Skip to content

Nullable reference types #1095

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

Merged
merged 32 commits into from
Nov 3, 2021
Merged

Nullable reference types #1095

merged 32 commits into from
Nov 3, 2021

Conversation

bart-degreed
Copy link
Contributor

@bart-degreed bart-degreed commented Oct 18, 2021

Closes #1029.

This PR annotates the codebase for use with nullable reference types (NRT).

This does not mean your API project that uses JADNC must do so too. But once you do, additional warnings will light up for possible NullReferenceExceptions and unneeded null checks in your code.

Overview of changes:

  • Breaking: ResourceType (formerly ResourceContext) now provides Get methods that throw when not found and Find methods that return null when not found. For example: GetAttributeByPublicName() vs FindAttributeByPublicName(). The same applies to methods on IResourceGraph. For example: GetResourceType() vs FindResourceType().
  • Breaking: An injectable dependency of type IResourceGraph was added to Controller constructors.
  • Fixed: JADNC now renders the correct path in error.source.pointer for required relationships that are missing in a post/patch request body when ASP.NET ModelState validation is enabled. And it takes JsonPropertyNameAttribute from System.Text.Json into account when properties on complex attributes are named differently. For details, see ./test/JsonApiDotNetCoreTests/UnitTests/ModelStateValidation/ModelStateValidationTests.cs.
  • Breaking: ASP.NET ModelState validation is now enabled by default. It can still be turned off in options. The rationale is that when using NRT, resource properties and relationships become required by default where they were optional before. When ModelState is enabled, this produces validation errors with reliable source pointers, which is more helpful to API clients than JADNC catching downstream exceptions (where the source path is lost) or even HTTP 500 errors due to SQL foreign key constraints. And even if your project is not using NRT, putting [Required] on attributes and relationships provides the same benefits.
  • Fixed: JADNC now respects configured MaxModelValidationErrors (https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.mvcoptions.maxmodelvalidationerrors) in atomic:operations requests, which can be set in services.AddMvcCore() (defaults to 200).
  • Documentation was added on how to deal with nullable value/reference types in resource models. Also added notes on how to map required one-to-one relationships correctly.
  • Made serialization of response bodies ~10% faster by caching resource graph information that doesn't change per request.
  • Fixed: Improved validation of JSON request bodies when various elements are explicitly set to null.

QUALITY CHECKLIST

  • Changes implemented in code
  • Complies with our contributing guidelines
  • Adapted tests
  • Documentation updated
  • N/A: Created issue to update Templates: {ISSUE_NUMBER}

@bart-degreed bart-degreed force-pushed the nullable-reference-types branch from 7fbd4dd to 8663e30 Compare October 18, 2021 11:27
Bart Koelman added 3 commits October 28, 2021 12:08
@bart-degreed bart-degreed force-pushed the nullable-reference-types branch 2 times, most recently from b9db066 to 5176b8f Compare October 28, 2021 15:15
Bart Koelman added 23 commits October 31, 2021 16:31
- Removed LinkBuilderGetNamespaceFromPathBenchmarks, we don't have such code anymore
- Cleanup query string benchmark
Due to recent enhancements, this produces better error messages for missing required relationships and avoids 500 errors due to foreign key constraint violations.
…d. This relies on the fact that the resource model is frozen after building the resource graph.

|                               Method |     Mean |   Error |  StdDev |
| ------------------------------------ | -------- | ------- | ------- |
| SerializeOperationsResponse (before) | 168.6 us | 1.74 us | 1.63 us |
| SerializeOperationsResponse (after)  | 148.5 us | 1.06 us | 0.99 us |
| SerializeResourceResponse   (before) | 123.7 us | 1.12 us | 1.05 us |
| SerializeResourceResponse   (after)  | 119.6 us | 1.05 us | 0.98 us |

This makes SerializeOperationsResponse 12% faster and SerializeResourceResponse 3% faster. What the benchmark does not show is the performance improvement on subsequent requests, so in practice the gain in higher.
@bart-degreed bart-degreed force-pushed the nullable-reference-types branch from 4b4d898 to 1a57ea0 Compare October 31, 2021 15:33
@bart-degreed bart-degreed force-pushed the nullable-reference-types branch from ba26952 to a1dcf87 Compare October 31, 2021 18:33
@codecov
Copy link

codecov bot commented Nov 1, 2021

Codecov Report

Merging #1095 (4f23fc2) into master (4a2abe9) will increase coverage by 0.32%.
The diff coverage is 87.47%.

❗ Current head 4f23fc2 differs from pull request most recent head bb426c6. Consider uploading reports for the commit bb426c6 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1095      +/-   ##
==========================================
+ Coverage   88.21%   88.54%   +0.32%     
==========================================
  Files         256      255       -1     
  Lines        6931     7086     +155     
==========================================
+ Hits         6114     6274     +160     
+ Misses        817      812       -5     
Impacted Files Coverage Δ
...NetCoreExample/Controllers/OperationsController.cs 0.00% <0.00%> (ø)
...iDotNetCoreExample/Controllers/PeopleController.cs 0.00% <0.00%> (ø)
...ApiDotNetCoreExample/Controllers/TagsController.cs 0.00% <0.00%> (ø)
...tNetCoreExample/Controllers/TodoItemsController.cs 0.00% <0.00%> (ø)
...ples/JsonApiDotNetCoreExample/Data/AppDbContext.cs 0.00% <0.00%> (ø)
...otNetCoreExample/Definitions/TodoItemDefinition.cs 0.00% <ø> (ø)
...Examples/JsonApiDotNetCoreExample/Models/Person.cs 0.00% <0.00%> (ø)
...rc/Examples/JsonApiDotNetCoreExample/Models/Tag.cs 0.00% <0.00%> (ø)
...amples/JsonApiDotNetCoreExample/Models/TodoItem.cs 0.00% <0.00%> (ø)
src/Examples/JsonApiDotNetCoreExample/Startup.cs 0.00% <ø> (ø)
... and 202 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a2abe9...bb426c6. Read the comment docs.

Bart Koelman added 2 commits November 2, 2021 09:39
In the past, [cibuild hung after update to the latest version](#1045).
Since then, the codebase has changed and a new minor version was released. These result in the cibuild no longer hanging.

Stats from my laptop (old=v2021.1.4, new=v2021.2.2):

```
master, inspectcode (old):  2:59
master, inspectcode (new):  2:00
nrt, inspectcode (old):     3:04
nrt, inspectcode (new):     2:33

master, cleanupcode (old): 11:06
master, cleanupcode (new):  5:39
nrt, cleanupcode (old):    11:59
nrt, cleanupcode (new):     6:34
```

So the newer versions got faster. And cleanupcode still takes the most time.

In this PR, 722 of 1020 files have changed, which is 70% of the total codebase.
Because during PR cibuild, we run cleanupcode only on changed files (in chunks), this PR takes very long and sometimes times out.
This is an exceptional case, future PRs shouldn't touch so many files. So in the end, I believe this update is the best way forward.
@bart-degreed bart-degreed force-pushed the nullable-reference-types branch from d592bd2 to 8002a53 Compare November 2, 2021 08:56
@bart-degreed bart-degreed marked this pull request as ready for review November 2, 2021 09:23
@bart-degreed bart-degreed requested a review from maurei November 2, 2021 09:23
bart-degreed pushed a commit that referenced this pull request Nov 3, 2021
@bart-degreed bart-degreed merged commit f0d638f into master Nov 3, 2021
@bart-degreed bart-degreed deleted the nullable-reference-types branch November 3, 2021 13:30
bart-degreed pushed a commit that referenced this pull request Dec 3, 2021
* Renamed TryXXX resource graph lookup methods to FindXXX, throwing on incoming null (not empty). Made non-public TryXXX methods tolerant against incoming nulls.

* Added directive to disable NRT in all source files

* Turn on NRT globally

* Annotated JsonApiDotNetCore library

* Use alternate public name for attribute, to improve test coverage

* Annotated example projects

* Annotated benchmark project
- Removed LinkBuilderGetNamespaceFromPathBenchmarks, we don't have such code anymore
- Cleanup query string benchmark

* Enhanced rendering of error.source.pointer on ModelState validation errors

* Respect configured MaxModelValidationErrors in operations

* Added docs for nullable reference types

* Automated cleanup code

* Fixed: do not fail when clearing a required to-many relationship

* Annotated controllers in integration tests

* Annotated DbContexts in integration tests

* Annotated fakers in integration tests

* Annotated TestBuildingBlocks

* Use Should() replacements that flow nullability

* Re-align similar testsets

* Annotated test projects, use fakers for non-trivial models in integration tests, FluentAssertions everywhere

* Fixed: make faker dates deterministic
See https://giters.com/bchavez/Bogus/issues/247

* Enable ASP.NET ModelState validation by default
Due to recent enhancements, this produces better error messages for missing required relationships and avoids 500 errors due to foreign key constraint violations.

* Check off roadmap entry

* Documented required one-to-one relationships mapping

* Added missing HEAD routes

* Prefer IDictionary<,>.TryGetValue() over .Contains() followed by indexer lookup

* Optimized response rendering time when no sparse fieldset is requested. This relies on the fact that the resource model is frozen after building the resource graph.

|                               Method |     Mean |   Error |  StdDev |
| ------------------------------------ | -------- | ------- | ------- |
| SerializeOperationsResponse (before) | 168.6 us | 1.74 us | 1.63 us |
| SerializeOperationsResponse (after)  | 148.5 us | 1.06 us | 0.99 us |
| SerializeResourceResponse   (before) | 123.7 us | 1.12 us | 1.05 us |
| SerializeResourceResponse   (after)  | 119.6 us | 1.05 us | 0.98 us |

This makes SerializeOperationsResponse 12% faster and SerializeResourceResponse 3% faster. What the benchmark does not show is the performance improvement on subsequent requests, so in practice the gain in higher.

* Fixed: handle nulls in request body

* Workaround for crash in cibuild

* Update to latest JetBrains tools

In the past, [cibuild hung after update to the latest version](#1045).
Since then, the codebase has changed and a new minor version was released. These result in the cibuild no longer hanging.

Stats from my laptop (old=v2021.1.4, new=v2021.2.2):

```
master, inspectcode (old):  2:59
master, inspectcode (new):  2:00
nrt, inspectcode (old):     3:04
nrt, inspectcode (new):     2:33

master, cleanupcode (old): 11:06
master, cleanupcode (new):  5:39
nrt, cleanupcode (old):    11:59
nrt, cleanupcode (new):     6:34
```

So the newer versions got faster. And cleanupcode still takes the most time.

In this PR, 722 of 1020 files have changed, which is 70% of the total codebase.
Because during PR cibuild, we run cleanupcode only on changed files (in chunks), this PR takes very long and sometimes times out.
This is an exceptional case, future PRs shouldn't touch so many files. So in the end, I believe this update is the best way forward.

* Package updates

* Review non-public TryXXX methods

* Addressed review feedback
bart-degreed pushed a commit that referenced this pull request Dec 3, 2021
* Renamed TryXXX resource graph lookup methods to FindXXX, throwing on incoming null (not empty). Made non-public TryXXX methods tolerant against incoming nulls.

* Added directive to disable NRT in all source files

* Turn on NRT globally

* Annotated JsonApiDotNetCore library

* Use alternate public name for attribute, to improve test coverage

* Annotated example projects

* Annotated benchmark project
- Removed LinkBuilderGetNamespaceFromPathBenchmarks, we don't have such code anymore
- Cleanup query string benchmark

* Enhanced rendering of error.source.pointer on ModelState validation errors

* Respect configured MaxModelValidationErrors in operations

* Added docs for nullable reference types

* Automated cleanup code

* Fixed: do not fail when clearing a required to-many relationship

* Annotated controllers in integration tests

* Annotated DbContexts in integration tests

* Annotated fakers in integration tests

* Annotated TestBuildingBlocks

* Use Should() replacements that flow nullability

* Re-align similar testsets

* Annotated test projects, use fakers for non-trivial models in integration tests, FluentAssertions everywhere

* Fixed: make faker dates deterministic
See https://giters.com/bchavez/Bogus/issues/247

* Enable ASP.NET ModelState validation by default
Due to recent enhancements, this produces better error messages for missing required relationships and avoids 500 errors due to foreign key constraint violations.

* Check off roadmap entry

* Documented required one-to-one relationships mapping

* Added missing HEAD routes

* Prefer IDictionary<,>.TryGetValue() over .Contains() followed by indexer lookup

* Optimized response rendering time when no sparse fieldset is requested. This relies on the fact that the resource model is frozen after building the resource graph.

|                               Method |     Mean |   Error |  StdDev |
| ------------------------------------ | -------- | ------- | ------- |
| SerializeOperationsResponse (before) | 168.6 us | 1.74 us | 1.63 us |
| SerializeOperationsResponse (after)  | 148.5 us | 1.06 us | 0.99 us |
| SerializeResourceResponse   (before) | 123.7 us | 1.12 us | 1.05 us |
| SerializeResourceResponse   (after)  | 119.6 us | 1.05 us | 0.98 us |

This makes SerializeOperationsResponse 12% faster and SerializeResourceResponse 3% faster. What the benchmark does not show is the performance improvement on subsequent requests, so in practice the gain in higher.

* Fixed: handle nulls in request body

* Workaround for crash in cibuild

* Update to latest JetBrains tools

In the past, [cibuild hung after update to the latest version](#1045).
Since then, the codebase has changed and a new minor version was released. These result in the cibuild no longer hanging.

Stats from my laptop (old=v2021.1.4, new=v2021.2.2):

```
master, inspectcode (old):  2:59
master, inspectcode (new):  2:00
nrt, inspectcode (old):     3:04
nrt, inspectcode (new):     2:33

master, cleanupcode (old): 11:06
master, cleanupcode (new):  5:39
nrt, cleanupcode (old):    11:59
nrt, cleanupcode (new):     6:34
```

So the newer versions got faster. And cleanupcode still takes the most time.

In this PR, 722 of 1020 files have changed, which is 70% of the total codebase.
Because during PR cibuild, we run cleanupcode only on changed files (in chunks), this PR takes very long and sometimes times out.
This is an exceptional case, future PRs shouldn't touch so many files. So in the end, I believe this update is the best way forward.

* Package updates

* Review non-public TryXXX methods

* Addressed review feedback
bart-degreed pushed a commit that referenced this pull request Dec 3, 2021
* Renamed TryXXX resource graph lookup methods to FindXXX, throwing on incoming null (not empty). Made non-public TryXXX methods tolerant against incoming nulls.

* Added directive to disable NRT in all source files

* Turn on NRT globally

* Annotated JsonApiDotNetCore library

* Use alternate public name for attribute, to improve test coverage

* Annotated example projects

* Annotated benchmark project
- Removed LinkBuilderGetNamespaceFromPathBenchmarks, we don't have such code anymore
- Cleanup query string benchmark

* Enhanced rendering of error.source.pointer on ModelState validation errors

* Respect configured MaxModelValidationErrors in operations

* Added docs for nullable reference types

* Automated cleanup code

* Fixed: do not fail when clearing a required to-many relationship

* Annotated controllers in integration tests

* Annotated DbContexts in integration tests

* Annotated fakers in integration tests

* Annotated TestBuildingBlocks

* Use Should() replacements that flow nullability

* Re-align similar testsets

* Annotated test projects, use fakers for non-trivial models in integration tests, FluentAssertions everywhere

* Fixed: make faker dates deterministic
See https://giters.com/bchavez/Bogus/issues/247

* Enable ASP.NET ModelState validation by default
Due to recent enhancements, this produces better error messages for missing required relationships and avoids 500 errors due to foreign key constraint violations.

* Check off roadmap entry

* Documented required one-to-one relationships mapping

* Added missing HEAD routes

* Prefer IDictionary<,>.TryGetValue() over .Contains() followed by indexer lookup

* Optimized response rendering time when no sparse fieldset is requested. This relies on the fact that the resource model is frozen after building the resource graph.

|                               Method |     Mean |   Error |  StdDev |
| ------------------------------------ | -------- | ------- | ------- |
| SerializeOperationsResponse (before) | 168.6 us | 1.74 us | 1.63 us |
| SerializeOperationsResponse (after)  | 148.5 us | 1.06 us | 0.99 us |
| SerializeResourceResponse   (before) | 123.7 us | 1.12 us | 1.05 us |
| SerializeResourceResponse   (after)  | 119.6 us | 1.05 us | 0.98 us |

This makes SerializeOperationsResponse 12% faster and SerializeResourceResponse 3% faster. What the benchmark does not show is the performance improvement on subsequent requests, so in practice the gain in higher.

* Fixed: handle nulls in request body

* Workaround for crash in cibuild

* Update to latest JetBrains tools

In the past, [cibuild hung after update to the latest version](#1045).
Since then, the codebase has changed and a new minor version was released. These result in the cibuild no longer hanging.

Stats from my laptop (old=v2021.1.4, new=v2021.2.2):

```
master, inspectcode (old):  2:59
master, inspectcode (new):  2:00
nrt, inspectcode (old):     3:04
nrt, inspectcode (new):     2:33

master, cleanupcode (old): 11:06
master, cleanupcode (new):  5:39
nrt, cleanupcode (old):    11:59
nrt, cleanupcode (new):     6:34
```

So the newer versions got faster. And cleanupcode still takes the most time.

In this PR, 722 of 1020 files have changed, which is 70% of the total codebase.
Because during PR cibuild, we run cleanupcode only on changed files (in chunks), this PR takes very long and sometimes times out.
This is an exceptional case, future PRs shouldn't touch so many files. So in the end, I believe this update is the best way forward.

* Package updates

* Review non-public TryXXX methods

* Addressed review feedback
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.

Adopt nullable reference types
2 participants