feat(webapi): Option to include deleted dialogs in ServiceOwner dialog search #1816
Conversation
📝 WalkthroughWalkthroughThis pull request introduces modifications to the API schema and domain model to enhance dialog filtering based on deletion status. A new schema definition for Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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
🧹 Nitpick comments (2)
docs/schema/V1/swagger.verified.json (2)
4076-4081: Consider adding documentation for deletion behavior.The
deletedAtfield's description could be enhanced to provide more details about:
- Whether this is a soft delete implementation
- How it affects visibility in different API endpoints
- Any automatic cleanup/purge processes
- Impact on related entities (activities, transmissions, etc.)
Consider updating the description to be more comprehensive:
- "description": "If deleted, the date and time when the deletion was performed.", + "description": "If deleted, the date and time when the deletion was performed. This indicates a soft-deleted dialog that will be hidden from list endpoints but still accessible via direct ID lookup. Deleted dialogs retain their activities and transmissions history.",
4076-4081: Consider adding deletion-related query parameters.To fully support the ServiceOwner's ability to list deleted dialogs, consider adding query parameters to filter by deletion status.
Consider adding these query parameters to the search endpoint:
includeDeleted(boolean) - Whether to include deleted dialogsdeletedAfter(DateTime) - Only return dialogs deleted after this datedeletedBefore(DateTime) - Only return dialogs deleted before this date
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/schema/V1/swagger.verified.json(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/DialogDtoBase.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/SearchDialogQuery.cs(1 hunks)
🔇 Additional comments (6)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/DialogDtoBase.cs (1)
105-108: LGTM! Well-documented property addition.The
DeletedAtproperty is well-documented and follows the established pattern of using nullableDateTimeOffsetfor temporal properties.src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/SearchDialogQuery.cs (1)
195-195: LGTM! Verify soft-delete filter behavior.The addition of
.IgnoreQueryFilters()aligns with the PR objective to allow ServiceOwners to list deleted dialogs. The placement is logical, and security is maintained through existing resource authorization checks.Let's verify the soft-delete filter implementation:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for soft-delete filter implementation # Look for global query filter setup that might be related to soft-delete # Search for query filter setup in DbContext ast-grep --pattern 'modelBuilder.Entity<Dialog>().HasQueryFilter($_)' # Search for soft-delete related properties or interfaces rg -l "ISoftDelete|DeletedAt|IsDeleted" --type csLength of output: 5996
Soft-delete filter behavior verified
The addition of .IgnoreQueryFilters() is correctly implemented to allow ServiceOwners to retrieve soft-deleted dialogs. Our search confirms that the soft-delete functionality is based on the ISoftDeletable pattern (as evidenced by the presence of ISoftDeletableEntity and related SoftDeletableExtensions), and the global query filter setup (typically applied using HasQueryFilter on soft-deletable entities) is in place. Security and performance remain intact as the authorization checks continue to enforce resource access restrictions.
docs/schema/V1/swagger.verified.json (4)
4076-4081: Addition ofdeletedAtproperty for tracking dialog deletion.A new property
deletedAthas been added to theV1ServiceOwnerDialogsQueriesSearch_Dialogschema to track when a dialog is deleted. This is a nullable DateTime field that aligns with the PR objective of allowing ServiceOwners to list deleted dialogs.
4076-4081: Verify the impact on existing queries and filters.The addition of the
deletedAtfield may affect existing queries. Let's verify the search functionality:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if there are any query parameters or filters that need to be updated for the deletedAt field # Search for query parameters in the search endpoints echo "Checking search endpoints for query parameters..." rg -A 10 "V1ServiceOwnerDialogsSearch_SearchDialog|V1EndUserDialogsSearch_SearchDialog" # Search for any filter implementations echo "Checking for filter implementations..." rg -A 10 "DeletedAt.*filter|filter.*DeletedAt"Length of output: 1870
Review Conclusion: No Impact on Existing Queries or Filters
The search endpoints and filter implementations do not reference the new deletedAt field. Based on the shell output:
- No query parameter or filtering logic was found that uses the deletedAt field.
- The endpoints for V1EndUserDialogsSearch_SearchDialog and V1ServiceOwnerDialogsSearch_SearchDialog remain unchanged.
This indicates that adding the deletedAt field in the swagger did not impact the existing query functionality.
4076-4081: Verify schema consistency across related endpoints.Let's verify that the
deletedAtproperty is consistently implemented across related schemas:
4076-4081: Verify API versioning impact.Let's check if this change requires a new API version:
✅ Verification successful
🌐 Web query:
What are the best practices for versioning REST APIs when adding new nullable fields?💡 Result:
When adding new nullable fields to a REST API, follow these best practices to maintain compatibility and minimize client disruption: ### 1. **Versioning Strategy** - Use **Semantic Versioning (SemVer)** with `MAJOR.MINOR.PATCH` format: - Increment **MINOR** version for backward-compatible additions (e.g., `1.3` → `1.4`)[4][9] - No MAJOR version change required since nullable fields don't break existing contracts[5][9] ### 2. **Backward Compatibility** - New nullable fields should: - Be optional in requests/responses - Default to `null`/empty when unspecified[2][6] - Avoid altering existing field behavior[7][9] ### 3. **Implementation Approaches** | Method | Example | Null Handling | |--------|---------|---------------| | **URI Versioning** | `GET /api/v1.4/resource` | Add field to latest API version[1][5] | | **Content Negotiation** | `Accept: application/vnd.api+json; version=1.4` | Granular control without URI changes[1][9] | | **Default Values** | `{"new_field": null}` | Explicit null representation[2][6] | ### 4. **Documentation & Communication** - Update OpenAPI/Swagger specs with: ```yaml components: schemas: Resource: properties: new_field: type: string nullable: true default: null ``` - Publish changelogs detailing added fields[1][9] - Provide migration guides showing null handling examples[2][6] ### 5. **Client Guidance** - Instruct consumers to: - Handle `null` values gracefully - Use conditional checks for new fields ```javascript if (response.new_field !== null && response.new_field !== undefined) { // Handle new field } ``` - Maintain support for previous API versions during transition[1][4] For nullable field implementation: - Use explicit `null` values rather than omitting keys[2][6] - Test server-side null handling with payloads like: ```json {"existing_field": "value", "new_field": null} ``` - Consider edge cases like form-data submissions where `undefined` omits fields[2] These practices enable safe evolution of APIs while preserving existing functionality. Major providers like Stripe and GitHub follow similar patterns when extending their interfaces[5][9]. Citations: - 1: https://www.xmatters.com/blog/blog-four-rest-api-versioning-strategies - 2: https://community.retool.com/t/issue-with-handling-nullable-fields-in-rest-api-resource-with-form-data/33463 - 3: https://www.baeldung.com/rest-versioning - 4: https://opensource.zalando.com/restful-api-guidelines/ - 5: https://restfulapi.net/versioning/ - 6: https://community.esri.com/t5/arcgis-rest-apis-and-services-questions/update-field-with-null/td-p/607240 - 7: https://learn.microsoft.com/en-us/azure/architecture/best-practices/api-design - 8: https://schweizerischebundesbahnen.github.io/api-principles/restful/best-practices/ - 9: https://liblab.com/blog/api-versioning-best-practices - 10: https://www.apollographql.com/blog/using-nullability-in-graphql
API Versioning Impact – Nullable Field Addition
Based on best practices for REST APIs, adding an optional, nullable field—like "deletedAt"—is considered a backward-compatible change. This means a new API version (major release) is generally not required; if versioning is used, only a minor version increment might be appropriate to reflect the update without breaking existing contracts.
oskogstad
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/DeletedFilter.cs (1)
3-8: Consider adding XML documentation.Adding XML documentation for the enum and its members would improve the API's usability by providing clear descriptions in IDE tooltips and generated API documentation.
+/// <summary> +/// Specifies how to filter dialogs based on their deletion status. +/// </summary> public enum DeletedFilter { + /// <summary> + /// Excludes deleted dialogs from the results. + /// </summary> Exclude = 1, + /// <summary> + /// Includes both deleted and non-deleted dialogs in the results. + /// </summary> Include = 2, + /// <summary> + /// Returns only deleted dialogs. + /// </summary> Only = 3 }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/schema/V1/swagger.verified.json(3 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/DeletedFilter.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/SearchDialogQuery.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/SearchDialogQuery.cs
- docs/schema/V1/swagger.verified.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Dry run deploy apps / Deploy job sync-resource-policy-information-job to test
🔇 Additional comments (1)
src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/DeletedFilter.cs (1)
3-8: Well-structured enum implementation!The enum is well-designed with clear, descriptive names and appropriate values. Starting from 1 instead of 0 is a good practice as it makes the default value explicit.
🤖 I have created a release *beep* *boop* --- ## [1.49.0](v1.48.5...v1.49.0) (2025-02-10) ### Features * Restore dialog action ([#1702](#1702)) ([331d492](331d492)) * **webapi:** Option to include deleted dialogs in ServiceOwner dialog search ([#1816](#1816)) ([5403063](5403063)) ### Miscellaneous Chores * **ci:** Increase container app verification timeout ([#1819](#1819)) ([fe5377a](fe5377a)) * **deps:** update dependency fastendpoints.swagger to 5.34.0 ([#1822](#1822)) ([8ce7e0e](8ce7e0e)) * **deps:** update dependency uuidnext to 4.1.0 ([#1823](#1823)) ([dfa5ce0](dfa5ce0)) * **deps:** update masstransit monorepo to 8.3.6 ([#1821](#1821)) ([6fbb41f](6fbb41f)) * refactor and add filters for telemetry ([#1813](#1813)) ([fd10351](fd10351)) --- 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)