fix(webapi): Explicit null on non-nullable lists no longer causes 500 INTERNAL SERVER ERROR #1602
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request focus on enhancing the configuration of FastEndpoints and Swagger components within the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (2)
Line range hint
178-185: Add tests to verify serialization behavior.The custom empty collection handling logic needs test coverage to ensure it works correctly with:
- Null collections
- Empty collections
- Non-empty collections
- Nested collections
Would you like me to help create comprehensive serialization tests for these scenarios?
Line range hint
208-213: Consider a more robust environment check for dialog prefix.The current development environment check might be too simplistic. Consider:
- Using a configuration value instead of environment check
- Handling staging/testing environments explicitly
-var dialogPrefix = builder.Environment.IsDevelopment() ? "" : "/dialogporten"; +var dialogPrefix = builder.Configuration + .GetValue<string>("SwaggerSettings:DialogPrefix") ?? + (builder.Environment.IsDevelopment() ? "" : "/dialogporten");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Digdir.Domain.Dialogporten.WebApi/Program.cs(1 hunks)
🔇 Additional comments (3)
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (3)
176-177: LGTM! This change fixes the 500 error for non-nullable lists.
The RespectNullableAnnotations = true setting ensures proper handling of nullable annotations during serialization, which directly addresses the issue mentioned in PR #1597.
Line range hint 186-188: Verify custom DateTime converter implementations.
While adding these converters is good practice, please ensure:
UtcDateTimeOffsetConverterproperly handles timezone conversionsDateTimeNotSupportedConverterprovides clear error messages
✅ Verification successful
Custom DateTime converters implementation verified successfully
The implementations are well-designed and handle the requirements appropriately:
-
UtcDateTimeOffsetConvertercorrectly enforces UTC by:- Requiring explicit offset notation (e.g., '+00:00', '-05:00', 'Z')
- Converting all values to Universal Time
-
DateTimeNotSupportedConverterprovides a clear and detailed error message:- Explicitly states that DateTime is not supported
- Directs users to use DateTimeOffset instead
- Includes guidance for support contact
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for converter implementations to verify their behavior
ast-grep --pattern 'class $_(DateTimeOffsetConverter|DateTimeNotSupportedConverter) {
$$$
}'
Length of output: 96
Script:
#!/bin/bash
# Let's try a broader search for converter classes and their implementations
rg -l "Converter" --type cs
# Then let's look for specific implementations
rg "class.*Converter.*DateTimeOffset|class.*Converter.*DateTime" --type cs -A 10
Length of output: 6918
176-177: Verify the completeness of the null handling fix.
While enabling RespectNullableAnnotations should fix the immediate issue, please verify:
- No other components need similar null handling configuration
- This change doesn't affect existing valid null cases
✅ Verification successful
Let me gather more information about the nullable properties and their usage to ensure we have a complete picture.
Let me search for any potential null-related issues in the models and configurations.
Based on the gathered information, I can now provide a final response:
The null handling configuration is complete and consistent across the codebase
The review has confirmed that:
- All JSON serialization configurations across the codebase consistently handle null values:
- WebApi uses
WhenWritingNullfor public API responses - Infrastructure clients use
WhenWritingDefaultfor internal communication
- WebApi uses
- The
RespectNullableAnnotations = truesetting in WebApi is appropriate as it's the public API layer where strict null handling is crucial - No other components require similar null handling configuration as they serve different purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other serialization configurations that might need similar changes
rg -l "JsonSerializerOptions|System.Text.Json" | xargs rg "RespectNullableAnnotations|DefaultIgnoreCondition"
# Search for existing null checks that might be affected
ast-grep --pattern 'if ($_ == null) {
$$$
}'
Length of output: 2020
Script:
#!/bin/bash
# Search for nullable properties and lists in the models
ast-grep --pattern 'public $_? $_? { get; set; }'
# Look for any other JSON serialization settings that might affect null handling
rg "JsonSerializer|[JsonIgnore]|[JsonProperty]" -A 2 -B 2
# Check for any custom model configurations that might affect serialization
rg "IModelConfiguration|ModelBuilder" -A 5
Length of output: 78361
Script:
#!/bin/bash
# Search for nullable list properties in models
ast-grep --pattern 'List<$_>? $_'
# Search for JsonSerializerOptions configurations in other files
rg "JsonSerializerOptions" -A 3 -B 3
# Look for any custom model configurations that might affect serialization
rg "OnModelCreating|protected override void" -A 5
Length of output: 65778
|
🤖 I have created a release *beep* *boop* --- ## [1.41.3](v1.41.2...v1.41.3) (2024-12-13) ### Bug Fixes * **azure:** adjust SKU and storage for staging ([#1601](#1601)) ([3fb9f95](3fb9f95)) * Collapse subject resource mappings before building sql query ([#1579](#1579)) ([b39c376](b39c376)) * **webapi:** Explicit null on non-nullable lists no longer causes 500 INTERNAL SERVER ERROR ([#1602](#1602)) ([2e8b3e6](2e8b3e6)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Setting
RespectNullableAnnotations = truemakes it so that explicit null on non-nullable list results in a 400 BAD REQUEST across the whole API.{ "type": "https://datatracker.ietf.org/doc/html/rfc7231#section-6.5.1", "title": "One or more validation errors occurred.", "status": 400, "instance": "/api/v1/serviceowner/dialogs", "errors": { "searchTags": [ "The property or field 'searchTags' on type 'Digdir.Domain.Dialogporten.Application.Features.V1.ServiceOwner.Dialogs.Commands.Create.CreateDialogCommand' doesn't allow setting null values. Consider updating its nullability annotation. Path: $.searchTags | LineNumber: 169 | BytePositionInLine: 22." ] }, "traceId": "00-958361cdc8411eee6b3e7f32f19d1acc-119627bfbb3b75f6-01" }Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)