Skip to content

Improved JsonApiClient and relationships into openapi-required-and-nullable #1231

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

Conversation

maurei
Copy link
Member

@maurei maurei commented Dec 19, 2022

This PR brings merges into #1185:

  • For resource attributes: JsonApiClient improvements when dealing with nullable/required/default-value properties when NRT is on/off. Also contains extensive test coverage.
  • For resource relationships: adjustment in JsonApiDotNetCore.OpenApi and extensive test coverage

For server-side testing (i.e. the generated OAS file), the table in #1111 provides a transparent overview for resource attributes of what is expected for required/nullable when NRT and/or MSV are on/off. This piece is already covered by #1185.

However, for client-side testing, that table doesn't clearly show which JsonApiClient behaviour during request is expected to fail and succeed. For that I've extended the overview in this PR to include relevant operations for which the client-side behaviour should be captured, see below. The overview is also extended to contain the same for relationships.

Screenshot 2022-12-29 at 10 25 31

For instance, from this overview it reads:

A client should be able to generate a request intentionally in which the json property

  • data.attributes.name is set to null (G9);
  • data.attributes.name is omitted from the request body (G9);
  • data.attributes.weight is set to its default value 0 (G12)

and should not be able to generate a request intentionally where

  • data.attributes.weight is omitted from the request body (H12)

where these paths correspond to the Name/Weight properties on the Chicken resource as declared server-side, for the scenario: NRT disabled and MSV enabled

Few more details:

  • Note that "NRT disabled" means: disabled both server-side and client-side. Therefore, the test cases in the PR are: 1 (server-on, client-on) and 2 (server-off, client-off). That implies we could for completeness also look into (server-on, client-off) and (server-off, client-on), but these scenarios don't generate any unique test cases. So I decided to leave it out for conciseness.
  • the scenario (NRT off, MSV on) is included but not (NRT off, MSV off), because it is identical. To see this, compare columns FGH with KLM
  • the scenario (NRT on, MSV on) is included but not (NRT on, MSV off), because it is nearly identical. Only I3 differs from C3, but that case doesn't deserve a new test suite because there are plenty of other cases already where omitting is not allowed in a similar circumstance.

@maurei maurei changed the title Rigorous SchemaProperty tests into openapi-required-and-nullable SchemaProperty tests into openapi-required-and-nullable Dec 19, 2022
@maurei maurei force-pushed the attributes-object-schema-property-tests branch from 14e6170 to 2f8d653 Compare December 20, 2022 13:04
@codecov
Copy link

codecov bot commented Dec 20, 2022

Codecov Report

Merging #1231 (affc187) into openapi-required-and-nullable-properties (f5bb7fc) will increase coverage by 0.00%.
The diff coverage is 94.84%.

@@                            Coverage Diff                            @@
##           openapi-required-and-nullable-properties    #1231   +/-   ##
=========================================================================
  Coverage                                     92.38%   92.39%           
=========================================================================
  Files                                           303      304    +1     
  Lines                                          8855     8910   +55     
=========================================================================
+ Hits                                           8181     8232   +51     
- Misses                                          674      678    +4     
Impacted Files Coverage Δ
...NetCore.OpenApi.Client/UnreachableCodeException.cs 0.00% <0.00%> (ø)
.../JsonApiDotNetCore.OpenApi.Client/JsonApiClient.cs 96.00% <96.59%> (ø)
...ggerComponents/ResourceFieldObjectSchemaBuilder.cs 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@maurei maurei requested a review from bkoelman December 20, 2022 15:46
@maurei maurei marked this pull request as ready for review December 20, 2022 15:46
@maurei maurei changed the title SchemaProperty tests into openapi-required-and-nullable Improved JsonApiClient and testing into openapi-required-and-nullable Dec 20, 2022
@maurei maurei force-pushed the openapi-required-and-nullable-properties branch from 589ad48 to 49108f3 Compare December 23, 2022 10:12
@maurei maurei force-pushed the attributes-object-schema-property-tests branch from b9c2601 to 2f2f664 Compare December 23, 2022 16:39
@maurei maurei force-pushed the openapi-required-and-nullable-properties branch from 49108f3 to f5bb7fc Compare December 23, 2022 16:57
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
@maurei maurei force-pushed the attributes-object-schema-property-tests branch from 2f2f664 to 932367e Compare December 23, 2022 17:02
@maurei maurei changed the title Improved JsonApiClient and testing into openapi-required-and-nullable Improved JsonApiClient and relationships into openapi-required-and-nullable Dec 24, 2022
@maurei
Copy link
Member Author

maurei commented Dec 25, 2022

@bkoelman your two cents on the following please.

Compare the current RequestTests#Can_exclude_optional_relationships with the newly proposed AlternativeFormRequestTests#Can_exclude_optional_relationship.

In the first form (current form), it isn't clear by reading the test how many or which relationships are being omitted. Also it isn't clear that every omitted relationship represents a distinct and unrelated test case. So I realised that this test too should actually be a Theory. In the second format it is now immediately clear that OldestChicken and AllChickens are the relevant omitted properties and that they represent individual test cases.

It comes at the cost that the json assertion string is not as readable anymore. But given that before it didn't illuminate what was relevant either, even when it was readable, I argue that this isn't a problem. And when the test fails, it will still be readable in the log anyway.

Same would apply to the following remaining tests:

Can_exclude_optional_attributes
Can_exclude_attributes_that_are_required_for_POST_when_performing_PATCH
Can_clear_nullable_attributes
Can_set_default_value_to_ValueType_attributes
Can_exclude_relationships_that_are_required_for_POST_when_performing_PATCH

These tests only have one omitted property so technically don't need a theory, but could stil consider doing so to highlight which property is omitted:
Cannot_clear_required_attribute_when_performing_POST
Can_clear_nullable_relationship

@maurei maurei merged commit d4f1d69 into openapi-required-and-nullable-properties Dec 27, 2022
@maurei maurei deleted the attributes-object-schema-property-tests branch December 27, 2022 16:15
maurei added a commit that referenced this pull request 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 pull request 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant