Skip to content

Implement new .NET8 specific attributes to entry-format validation #516

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 19 commits into
base: dev
Choose a base branch
from

Conversation

marierin
Copy link

No description provided.

1nf0rmagician and others added 8 commits November 23, 2023 06:21
Bumps [Moq](https://github.com/moq/moq) from 4.20.69 to 4.20.70.
- [Release notes](https://github.com/moq/moq/releases)
- [Changelog](https://github.com/devlooped/moq/blob/main/CHANGELOG.md)
- [Commits](moq/moq.spikes@v4.20.69...v4.20.70)

---
updated-dependencies:
- dependency-name: Moq
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
…oq-4.20.70

Bump Moq from 4.20.69 to 4.20.70
Bumps `efCoreVersion` from 8.0.0 to 8.0.4.

Updates `Microsoft.EntityFrameworkCore` from 8.0.0 to 8.0.4
- [Release notes](https://github.com/npgsql/efcore.pg/releases)
- [Commits](npgsql/efcore.pg@v8.0.0...v8.0.4)

Updates `Microsoft.EntityFrameworkCore.Design` from 8.0.0 to 8.0.4
- [Release notes](https://github.com/npgsql/efcore.pg/releases)
- [Commits](npgsql/efcore.pg@v8.0.0...v8.0.4)

Updates `Microsoft.EntityFrameworkCore.Relational` from 8.0.0 to 8.0.4
- [Release notes](https://github.com/dotnet/efcore/releases)
- [Commits](dotnet/efcore@v8.0.0...v8.0.4)

Updates `Microsoft.EntityFrameworkCore.InMemory` from 8.0.0 to 8.0.4
- [Release notes](https://github.com/npgsql/efcore.pg/releases)
- [Commits](npgsql/efcore.pg@v8.0.0...v8.0.4)

Updates `Microsoft.EntityFrameworkCore.Proxies` from 8.0.0 to 8.0.4
- [Release notes](https://github.com/npgsql/efcore.pg/releases)
- [Commits](npgsql/efcore.pg@v8.0.0...v8.0.4)

Updates `Microsoft.EntityFrameworkCore.Sqlite` from 8.0.0 to 8.0.4
- [Release notes](https://github.com/npgsql/efcore.pg/releases)
- [Commits](npgsql/efcore.pg@v8.0.0...v8.0.4)

Updates `Npgsql.EntityFrameworkCore.PostgreSQL` from 8.0.0 to 8.0.4
- [Release notes](https://github.com/npgsql/efcore.pg/releases)
- [Commits](npgsql/efcore.pg@v8.0.0...v8.0.4)

---
updated-dependencies:
- dependency-name: Microsoft.EntityFrameworkCore
  dependency-type: direct:production
  update-type: version-update:semver-patch
- dependency-name: Microsoft.EntityFrameworkCore.Design
  dependency-type: direct:production
  update-type: version-update:semver-patch
- dependency-name: Microsoft.EntityFrameworkCore.Relational
  dependency-type: direct:production
  update-type: version-update:semver-patch
- dependency-name: Microsoft.EntityFrameworkCore.InMemory
  dependency-type: direct:production
  update-type: version-update:semver-patch
- dependency-name: Microsoft.EntityFrameworkCore.Proxies
  dependency-type: direct:production
  update-type: version-update:semver-patch
- dependency-name: Microsoft.EntityFrameworkCore.Sqlite
  dependency-type: direct:production
  update-type: version-update:semver-patch
- dependency-name: Npgsql.EntityFrameworkCore.PostgreSQL
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [Microsoft.NET.Test.Sdk](https://github.com/microsoft/vstest) from 17.8.0 to 17.10.0.
- [Release notes](https://github.com/microsoft/vstest/releases)
- [Changelog](https://github.com/microsoft/vstest/blob/main/docs/releases.md)
- [Commits](microsoft/vstest@v17.8.0...v17.10.0)

---
updated-dependencies:
- dependency-name: Microsoft.NET.Test.Sdk
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
…icrosoft.NET.Test.Sdk-17.10.0

Bump Microsoft.NET.Test.Sdk from 17.8.0 to 17.10.0
…fCoreVersion-8.0.4

Bump efCoreVersion from 8.0.0 to 8.0.4
Copy link
Member

@1nf0rmagician 1nf0rmagician left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two additional remarks

  1. Please update the documentation. Fell free to not only add your new additions but also correct any issues you encountered when using the documentation to understand the Entry concept. As I told you at the beginning, it would be nice if the next person getting into it could understand what it is and what it can be used for without to much additional help from us personally.
  2. Did you decide against the new Base64String validation attribute for a specific reason? Coincidently, we could probably make good use of that validation attribute with the new File System we want to provide😬

@1nf0rmagician
Copy link
Member

1nf0rmagician commented Jan 22, 2025

One more comment to add: Please check this site, it explains the two different attributes to validate collection length and string length. I think it would be alright to share the field in the validation class, as we also have the IsCollection flag on the entry. Bt you need to make sure in the serialization (and on the consuming site as well) to ignore the wrong attributes and to check for the right things when validating 😊

Copy link
Member

@1nf0rmagician 1nf0rmagician left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry but I don't get the optimization you did in the code since the last review, could we please have a quick chat about it 🙈

}
else if (attribute is RegularExpressionAttribute regexAttribute)
validation.Regex = regexAttribute.Pattern;
else if (attribute is StringLengthAttribute strLength)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I missing something or are the RegularExpressionAttribute and the StringLengthAttribute missing? 🤔

#if NET8_0
var validationAttribute = attributeProvider.GetCustomAttribute<AllowedValuesAttribute>();
var allowedValues = validationAttribute?.Values.Select(o => o.ToString()) ?? Enumerable.Empty<string>();
if (allowedValues.All(value => !string.IsNullOrEmpty(value)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the intention behind this if? From my understanding this would cause the allowed values to be used only if the empty string is not part of the AllowedValues, is that intended?
Also this method will throw a NullRef exception, if someone in a .net 8 application uses the possible values attribute for a list of primitive types or an enum.
Also the PossibleValues attribute is ignored, if the AllowedValues attribute is used (This is the only thing i'm fine with, if you intended it)

This is what i would expect, but please check if i missunderstood something 🙈

if( allowedValues is null)
{
    return allowedValues?.Distinct().ToArray();
}

@1nf0rmagician
Copy link
Member

Also please check the unit test, they should be resolved by the changes necessary. But you might want to check them if not

<PackageReference Update="Microsoft.NET.Test.Sdk" Version="17.8.0" />
<PackageReference Update="Moq" Version="4.20.69" />
<PackageReference Update="Microsoft.NET.Test.Sdk" Version="17.*" />
<PackageReference Update="Moq" Version="4.*" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not use a generic version here. Make it explict.

@@ -18,8 +18,8 @@
<!-- Package versions for package references across all projects -->
<ItemGroup>
<!--3rd party dependencies-->
<PackageReference Update="Microsoft.NET.Test.Sdk" Version="17.8.0" />
<PackageReference Update="Moq" Version="4.20.69" />
<PackageReference Update="Microsoft.NET.Test.Sdk" Version="17.*" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here - i would use an explict version here. In the past there were a lot of changes between the SDKs, also in minors.

@@ -54,33 +54,109 @@ The recursive `SubEntries` structure represents properties of a `EntryValueType.
Example class:

````cs
public class Root
public class PackagingResource
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Root sounds better than this

[DisplayName("Bob")]
public Foo Blah { get; set; }
[EntrySerialize]
[isRequiered]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isnt it RequiredAttribute?

public Foo Blah { get; set; }
[EntrySerialize]
[isRequiered]
public Product Product_1 { get; set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No underscores in property-names

public regex: string;
public isRequired: boolean;
public isPassword: boolean;
public deniedValue: string[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why this is not plural here? deniedValues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants