feat(webAPI): Make all lists nullable in OpenAPI schema #1359
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request modifies the serialization behavior in 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
|
oskogstad
force-pushed
the
feat/remove-filtering-of-empty-collections-when-serializing
branch
from
316478a to
33cf27a
Compare
oskogstad
changed the title
|
|
oskogstad
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/Digdir.Domain.Dialogporten.WebApi/OpenApiDocumentExtensions.cs (1)
84-90: Add XML documentation for consistency.Other public methods in this file have XML documentation explaining their purpose and behavior. Consider adding documentation to maintain consistency.
+ /// <summary> + /// Makes all collection properties in the OpenAPI document nullable to support serialization of empty collections. + /// </summary> + /// <param name="openApiDocument">The OpenAPI document to process.</param> public static void MakeCollectionsNullable(this OpenApiDocument openApiDocument)Also, consider adding null checks for defensive programming:
public static void MakeCollectionsNullable(this OpenApiDocument openApiDocument) { + ArgumentNullException.ThrowIfNull(openApiDocument); + ArgumentNullException.ThrowIfNull(openApiDocument.Components?.Schemas); + foreach (var schema in openApiDocument.Components.Schemas.Values)src/Digdir.Domain.Dialogporten.WebApi/Program.cs (3)
Line range hint
166-173: Update serializer configuration to remove empty collection filtering.The current configuration still includes the
IgnoreEmptyCollectionsmodifier which contradicts the PR's objective of allowing empty collections in the response.Apply this diff to update the serializer configuration:
x.Serializer.Options.DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull; - // Do not serialize empty collections - x.Serializer.Options.TypeInfoResolver = new DefaultJsonTypeInfoResolver - { - Modifiers = { IgnoreEmptyCollections } - };
Line range hint
214-223: Remove unusedIgnoreEmptyCollectionsmethod.This method is no longer used after updating the serializer configuration and should be removed to maintain code cleanliness.
Remove the entire method:
-static void IgnoreEmptyCollections(JsonTypeInfo typeInfo) -{ - foreach (var property in typeInfo.Properties) - { - if (property.PropertyType.IsAssignableTo(typeof(ICollection))) - { - property.ShouldSerialize = (_, val) => val is ICollection collection && collection.Count > 0; - } - } -}
Line range hint
166-166: Consider usingnullinstead of empty arrays.As discussed in PR comments, since
DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNullis already configured, consider initializing empty collections asnullinstead of empty arrays ([]) in your models. This would achieve the same effect of omitting empty collections from serialization while being more memory efficient.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
docs/schema/V1/swagger.verified.json(74 hunks)src/Digdir.Domain.Dialogporten.WebApi/OpenApiDocumentExtensions.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Program.cs(1 hunks)tests/k6/tests/serviceowner/dialogSearch.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/k6/tests/serviceowner/dialogSearch.js
🔇 Additional comments (4)
src/Digdir.Domain.Dialogporten.WebApi/OpenApiDocumentExtensions.cs (2)
83-106: Verify coordination with JSON serialization settings.
Based on the PR discussion, consider if this OpenAPI documentation change should be coordinated with corresponding changes to the JSON serialization configuration (e.g., JsonIgnoreCondition.WhenWritingNull). This would ensure consistency between the API documentation and actual behavior.
✅ Verification successful
OpenAPI nullable collections align with JSON serialization configuration
The OpenAPI documentation changes are consistent with the existing JSON serialization configuration. The codebase already:
- Sets
DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNullin WebApi - Includes logic to ignore empty collections
- Uses
WhenWritingDefaultfor cloud events
This means the nullable collections in OpenAPI spec correctly reflect the actual runtime behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for JSON serialization configuration
# Test: Search for JsonIgnoreCondition configuration
rg -l "JsonIgnoreCondition"
# Test: Search for serialization setup in Program.cs or similar startup files
fd -e cs -x rg -l "AddJsonOptions|ConfigureJsonOptions"
Length of output: 468
Script:
#!/bin/bash
# Let's check the actual JSON serialization configuration in the identified files
# Check Program.cs for JSON configuration
echo "=== WebApi Program.cs JSON Configuration ==="
rg -A 5 -B 5 "JsonIgnoreCondition|AddJsonOptions|ConfigureJsonOptions" src/Digdir.Domain.Dialogporten.WebApi/Program.cs
echo "\n=== SerializerOptions.cs Configuration ==="
cat src/Digdir.Domain.Dialogporten.Infrastructure/Common/Serialization/SerializerOptions.cs
Length of output: 2757
92-106: Consider handling nested objects recursively.
The current implementation only processes array properties at the top level of each schema. Consider recursively processing nested objects to ensure all array properties throughout the schema hierarchy are made nullable.
private static void MakeCollectionsNullable(JsonSchema schema)
{
if (schema.Properties == null)
{
return;
}
- foreach (var property in schema.Properties.Values)
+ foreach (var property in schema.Properties.Values)
{
if (property.Type.HasFlag(JsonObjectType.Array))
{
property.IsNullableRaw = true;
}
+ // Recursively process nested objects
+ if (property.Type.HasFlag(JsonObjectType.Object))
+ {
+ MakeCollectionsNullable(property);
+ }
}
}Also, let's verify the impact of these changes on the OpenAPI specification:
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (1)
197-197: LGTM: Making collections nullable aligns with PR objectives.
The addition of MakeCollectionsNullable aligns with the PR's goal of modifying collection serialization behavior.
docs/schema/V1/swagger.verified.json (1)
Line range hint 1-7540: LGTM! Schema is well-documented and consistent
The OpenAPI specification is well-structured with:
- Clear descriptions for all properties
- Good examples
- Proper type definitions
- Consistent nullable array handling
| @@ -211,6 +211,7 @@ | |||
| "items": { | |||
| "$ref": "#/components/schemas/V1EndUserDialogsQueriesSearch_Dialog" | |||
| }, | |||
| "nullable": true, | |||
There was a problem hiding this comment.
Breaking Change: Arrays are now nullable across all DTOs
The change consistently makes all array properties nullable across the API contract. While this is a valid approach to distinguish between "no data" (null) and "empty set" ([]), this is a breaking change that requires client updates.
Consider the following recommendations:
-
Add a migration guide for clients explaining:
- The change from empty arrays to null
- How to handle both null and empty arrays
- Examples of proper null checking
-
Consider adding a version suffix to the API endpoints (e.g.,
/v2) to maintain backward compatibility during migration. -
Update API documentation to explicitly mention this change in array handling.
Also applies to: 243-243, 265-265, 326-326, 359-359, 612-612, 620-620, 628-628, 679-679, 725-725, 758-758, 784-784, 866-866, 947-947, 960-960, 1052-1052, 1120-1120, 1190-1190, 1203-1203, 1416-1416, 1467-1467, 1600-1600, 1613-1613, 1716-1716, 1791-1791, 1804-1804, 1907-1907, 2021-2021, 2041-2041, 2099-2099, 2213-2213, 2299-2299, 2365-2365, 2373-2373, 2470-2470, 2478-2478, 2486-2486, 2532-2532, 2568-2568, 2598-2598, 2664-2664, 2694-2694, 2762-2762, 2777-2777, 2879-2879, 2965-2965, 3045-3045, 3060-3060, 3164-3164, 3172-3172, 3180-3180, 3220-3220, 3234-3234, 3250-3250, 3317-3317, 3347-3347, 3415-3415, 3430-3430, 3658-3658, 3666-3666, 3674-3674, 3726-3726, 3780-3780, 3813-3813, 3846-3846, 3928-3928, 4009-4009, 4022-4022, 4115-4115, 4182-4182, 4253-4253, 4266-4266, 4495-4495, 4552-4552, 4680-4680, 4748-4748, 4761-4761, 4864-4864, 4935-4935, 4948-4948, 5051-5051, 5121-5121
oskogstad
deleted the
feat/remove-filtering-of-empty-collections-when-serializing
branch
🤖 I have created a release *beep* *boop* --- ## [1.29.0](v1.28.3...v1.29.0) (2024-11-06) ### Features * **webAPI:** Make all lists nullable in OpenAPI schema ([#1359](#1359)) ([920d749](920d749)) ### Bug Fixes * **graphql:** ensure gql has maskinporten environment set ([#1408](#1408)) ([152417a](152417a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
New Features
Bug Fixes
Tests
DateTimeOffsettypes.