Skip to content
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

Add support for handling of non-nullable reference types in required. #1111

Closed
maurei opened this issue Nov 16, 2021 · 4 comments · Fixed by #1185
Closed

Add support for handling of non-nullable reference types in required. #1111

maurei opened this issue Nov 16, 2021 · 4 comments · Fixed by #1185
Labels

Comments

@maurei
Copy link
Member

maurei commented Nov 16, 2021

For non-nullable reference types, a resource attribute should be included in the OpenAPI required property if model state validation is enabled, and should be excluded otherwise.

The following table displays a comprehensive overview of the behavior of required, nullable in an OpenAPI schema, depending on the property type, usage of [Required], configuration of NRT, and configuration of model state validation.

177394179-2943249b-f5f1-4a72-adab-1c1d7ae85fc6
@maurei
Copy link
Member Author

maurei commented Jul 5, 2022

An interesting case worth elaborating on is the OAS nullability on B6 and H6. One might expect TRUE here, after all within the scope an OAS description it is possible for the field here to be required (it cannot be omitted) while simultaneously allowing for null.

The caveat here, however, is that our implementation will not allow null because the model state validation will not allow it. If MSV is disabled, it will would still fail because of a null constraint violation in the database.

@bkoelman
Copy link
Member

bkoelman commented Jul 11, 2022

I'm not entirely sure about H6. The API may be running against a pre-existing database without column constraints in place, or not use EF Core at all. Is there a use case where someone intentionally declares a property as [Required] string?, or is this just a theoretical case?

I'm thinking of a database column that used to be optional, but has become required in vNext. The database column remains nullable (to preserve existing data), yet the API would not accept null as input anymore.

Also, note that [Required] means more than just not-null. Excerpt from https://docs.microsoft.com/en-us/dotnet/api/system.componentmodel.dataannotations.requiredattribute?view=net-6.0:

The RequiredAttribute attribute specifies that when a field on a form is validated, the field must contain a value. A validation exception is raised if the property is null, contains an empty string (""), or contains only white-space characters.

@maurei
Copy link
Member Author

maurei commented Sep 6, 2022

Another (theoretical) use case for having something like[Required] string? property could be to read null as a "pristine" state, but disallowing a client to write null. Eg. [Required] int? Grade with value null could mean "no grade (yet)".

I don't know if this is actually configurable with EF Core, because RequiredAttribute means not null, as far as it is concerns mapping to a database column using EF Core..

Given that JANDC is designed to work out-of-the-box with EF Core and these examples require to deviate from that, I would propose to leave these cases out of the picture.

@bkoelman
Copy link
Member

bkoelman commented Sep 6, 2022

C# allows to have different nullabilities for the getter and setter of a property, as described at https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/attributes/nullable-analysis#preconditions-allownull-and-disallownull. We could support that in OAS by emitting different models for read/write verbs. But I'd consider that a nice-to-have, probably best postponed until asked for.

However I disagree with leaving [Required] string? out of the picture. The use of EF Core is just a (pluggable) implementation detail that shouldn't drive the meaning of the exposed model. Even so, it's easy enough to deviate in EF Core by having [Required] with .Required(false) in mappings. My last comment describes the use case for doing that.

Explicitly adding [Required] wins over any C# nullability state when MSV is on, and I believe that should be the case when MSV is off as well. Because turning on MSV afterwards should not change the OAS model. Therefore I think H6 is correct, after all. In a broader sense, .NET attributes have always overridden/tweaked what's expressed at the C# language level, so it makes sense to me to adhere to that here as well. Examples: ComVisible, PrincipalPermission, MaybeNull, Out, DllExport, DebuggerVisible.

maurei added a commit that referenced this issue Dec 28, 2022
…1111

Organise tests such that they map directly to the tables in #1231 and #1111
bkoelman added a commit that referenced this issue Oct 9, 2023
* Added tests for nullable and required properties in schema generation

* Added handling of modelstate validation in setting required attributes

* Not all swagger documents need to be saved to disk; changes in OpenApiTestContext to allow for this

* Added OpenApi client tests for nullable and required properties

* Use NullabilityInfoContext for nullability information

* Post-merge fixes

* Post-merge fixes

* Fixed: do not share NullabilityInfoContext, it is not thread-safe

* Review feedback

OpenApiTests/SchemaProperties test collection:
* Allow for usage of OpenApiStartup directly
* Remove unneeded adding of JsonConverter
* Replace null checks with .ShouldHaveCount() calls
* Adjust test names
Misc:
* Reverted .editorconfig changes and fixed ApiException constructor instead
* Remove Debug statement

* remove redundant new lines in eof added by cleanupcode

* Improved naming in OpenApiTests/SchemaProperties

* Review feedback: NullabilityTests

* Improved JsonApiClient and testing

SchemaProperty Tests:
* More rigorous test suite, see PR description

IJsonApiClient:
* Renamed registration method to a more functionally descriptive name.
* Improved documentation to contain most relevant information on top instead of at the end, and removed ambiguigity in wording.

JsonApiClient
* Fix bug: disallow omitting members that are explicitly required by the OAS description
* Renamed AttributeNamesContainer to AttributesObjectContext because it was more than just a container
* Misc: better variable naming

* Fix test: should not omit required field in test request body

* Temp enable CI buid for current branch

* Rename test files: it no longer only concerns required attributes, but more generally request behaviour

* Changes and tests for support of nullable and required for relationships

* - Rename placeholder model names and properties to examples consisent with existing test suite
- Use existing DbContext instead of temporary one

* Move into consistent folder structure, remove bad cleanupcode eof linebreaks

* Organise tests such that they map directly to the tables in #1231 and #1111

Organise tests such that they map directly to the tables in #1231 and #1111

* Add two missing 'Act' comments

* More elaborate testing

-> in sync with latest version of nullability/required table
-> introduces ResourceFieldValidationMetadataProvider
-> Fix test in legacy projects
-> Reusable faker building block for OpenApiClient related concerns

* Remove non-sensical testcases. Add caching in ObjectExtensions.

* Remove overlooked code duplication in OpenApiTests, revert reflection caching in object extension

* Make AutoFakers deterministic; generate positive IDs

* Fix nameof

* Use On/Off naming, shorten type names by using Nrt+Msv

* Renamed EmptyResource to Empty to further shorten FK names

* Fixed invalid EF Core mappings, resulting in logged warnings and inability to clear optional to-one relationship when NRT off; fixed wrong public names

* Move misplaced Act comments

* Optimize and clarify ResourceFieldValidationMetadataProvider

* Rename method, provide error message

* Refactor JsonApiClient: simplified recursion by using two converters, clearer naming, separation of concerns, improved error message

* Add relationship nullability assertions in OpenAPI client tests

* Cleanup JsonElementExtensions

* Cleanup ObjectExtensions

* Make base type abstract, remove redundant TranslateAsync calls, inline relationship Data property name

* Simplify usings

* Sync up test names

* Fix invalid tests

* Fix assertion messages

* Sync up tests

* Revert change to pass full options instead of just the naming policy

* Fix casing in test names

* Simplify Cannot_exclude_Id tests

* Rename base type to avoid OpenApiClientTests.OpenApiClientTests

* Adapt to existing naming convention

* Remove redundant assertions, fix formatting

* Correct test names

* Centralize code for property assignment in tests

* Apply Resharper hint: convert switch statement to expression

* Simplify expressions

* Simplify exception assertions

* Use string interpolation

* Corrections in openapi documentation

* Simplify code

* Remove redundant suppression

* Combine OpenAPI client tests for create resource with null/default attribute

* Fixup OpenAPI example and docs

* Revert "Merge branch 'master' into openapi-required-and-nullable-properties"

This reverts commit 66a2dc4, reversing
changes made to c3c4844.

* Workaround for running OpenAPI tests on Windows

* Address failing InspectCode

* Remove redundant calls

* Remove redundant tests

* Move types out of the wrong namespace

* Remove redundant suppressions in openapi after update to CSharpGuidelinesAnalyzer v3.8.4

---------

Co-authored-by: Bart Koelman <[email protected]>
@bkoelman bkoelman closed this as completed Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

2 participants