chore: Change 400 to 410 for update/delete on deleted dialogs #1552
Conversation
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications to the Dialogporten API's OpenAPI specification and related classes. Key changes include the addition of a Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
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
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 (7)
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogTransmissions/Create/CreateDialogTransmissionEndpointSummary.cs (2)
26-26: LGTM! Maintains consistency with other endpoints.The addition of
Status410Gonefollows the same pattern as other endpoints, maintaining consistency in API behavior. Since this change extends the PR's original scope (update/delete operations) to include create operations, consider updating the PR description to reflect this broader change.Consider adding a note in the PR description that the change has been extended to create operations for both activities and transmissions to maintain API consistency.
26-26: Overall Implementation ReviewThe changes consistently implement HTTP 410 Gone status across multiple endpoints:
- Update operations (as per original PR scope)
- Create operations for activities (extension)
- Create operations for transmissions (extension)
This consistent approach improves API clarity by properly distinguishing between validation errors (400) and operations on deleted resources (410). However, since the implementation extends beyond the original PR scope of "update/delete operations", please:
- Update the PR description to reflect the broader scope
- Verify that the corresponding endpoint handlers consistently implement the 410 Gone response
Also applies to: 26-26, 26-26
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Update/UpdateDialogEndpoint.cs (1)
30-30: LGTM! Consider updating API documentation.The changes correctly implement the 410 Gone status for deleted dialogs in the update endpoint, maintaining consistency with the delete endpoint. Consider updating the API documentation to reflect this change in behavior.
Consider adding a comment in the API documentation explaining when the 410 Gone status is returned, similar to the documentation in DeleteDialogEndpointSummary.
Also applies to: 48-48
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/DeleteDialogTests.cs (1)
Line range hint
13-61: Add test coverage for deleting an already deleted dialogThe test suite only verifies the EntityDeleted response for updating a deleted dialog. We should also test the same behavior when attempting to delete an already deleted dialog.
Add a new test method:
[Fact] public async Task Deleting_Already_Deleted_Dialog_Should_Return_EntityDeleted() { // Arrange var createDialogCommand = DialogGenerator.GenerateSimpleFakeDialog(); var createDialogResponse = await Application.Send(createDialogCommand); var dialogId = createDialogResponse.AsT0.Value; // Delete first time var deleteDialogCommand = new DeleteDialogCommand { Id = dialogId }; await Application.Send(deleteDialogCommand); // Act - Delete second time var secondDeleteResponse = await Application.Send(deleteDialogCommand); // Assert secondDeleteResponse.TryPickT2(out var entityDeleted, out _).Should().BeTrue(); entityDeleted.Should().NotBeNull(); entityDeleted.Message.Should().Contain(dialogId.ToString()); }src/Digdir.Domain.Dialogporten.WebApi/Common/Constants.cs (1)
21-21: Make message format consistent with other dialog messagesThe new
DialogDeletedmessage uses generic "Entity" terminology while other messages in this class specifically mention "dialog". Let's maintain consistency.-internal const string DialogDeleted = $"Entity with the given key(s) is removed."; +internal const string DialogDeleted = "The dialog {0} has been removed.";src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Delete/DeleteDialogCommand.cs (1)
57-59: Track restoration hint implementationThe TODO comment references issue #1543 for dialog restoration. When implemented, we should enhance the error message to guide users about potential restoration options.
Would you like me to create a GitHub issue to track the enhancement of the error message once restoration is implemented?
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs (1)
98-98: LGTM! Changed to return EntityDeleted for deleted dialogsThe change from returning BadRequest to EntityDeleted properly implements the new 410 Gone response for deleted dialogs.
I notice there's a TODO comment referencing issue #1543 regarding dialog restoration. Would you like me to create a tracking issue for implementing the restoration hint in the error message?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
docs/schema/V1/swagger.verified.json(4 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Delete/DeleteDialogCommand.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs(2 hunks)src/Digdir.Domain.Dialogporten.WebApi/Common/Constants.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogActivities/Create/CreateDialogActivityEndpoint.cs(2 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogActivities/Create/CreateDialogActivityEndpointSummary.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogTransmissions/Create/CreateDialogTransmissionEndpoint.cs(2 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogTransmissions/Create/CreateDialogTransmissionEndpointSummary.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Delete/DeleteDialogEndpoint.cs(2 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Delete/DeleteDialogEndpointSummary.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Update/UpdateDialogEndpoint.cs(2 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Update/UpdateDialogEndpointSummary.cs(1 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/DeleteDialogTests.cs(1 hunks)
🧰 Additional context used
📓 Learnings (1)
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/DeleteDialogTests.cs (1)
Learnt from: elsand
PR: digdir/dialogporten#1529
File: tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs:29-32
Timestamp: 2024-11-25T18:06:11.188Z
Learning: The end-to-end tests cover the `serviceprovider.admin-scope` scenario for dialog creation, so additional unit tests are not necessary.
🔇 Additional comments (11)
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogTransmissions/Create/CreateDialogTransmissionEndpoint.cs (2)
38-38: LGTM! Proper addition of 410 Gone status code.
The addition of Status410Gone to the endpoint configuration is well-placed and follows proper HTTP status code ordering.
71-71: LGTM! Proper handling of gone status.
The addition of the gone case in the result handling is consistent with the endpoint's error handling pattern. Let's verify that this change is consistently applied across related endpoints.
✅ Verification successful
Gone status handling is consistently implemented across dialog-related endpoints
The verification shows that the gone status (410) is properly handled across related endpoints:
- UpdateDialogEndpoint: Includes Status410Gone in response codes and handles
gone => this.GoneAsync(gone, ct) - DeleteDialogEndpoint: Includes Status410Gone in response codes and handles
gone => this.GoneAsync(gone, ct) - GetDialogEndpoint (EndUser): Handles
deleted => this.GoneAsync(deleted, ct)
The implementation in CreateDialogTransmissionEndpoint follows the same pattern as other endpoints in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent handling of gone status across related endpoints
# Expected: All dialog-related endpoints should handle gone status similarly
# Search for endpoints that might need similar gone status handling
rg -l "DialogEndpoint" | xargs rg -l "Configure|HandleAsync" | xargs rg -C 5 "Status410Gone|GoneAsync"
# Search for potential missing gone status handling in dialog-related endpoints
rg -l "DialogEndpoint" | xargs rg -l "Configure|HandleAsync" | xargs rg -C 5 "BadRequestAsync"
Length of output: 15725
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Update/UpdateDialogEndpointSummary.cs (1)
26-26: LGTM! Addition of 410 Gone status aligns with PR objectives.
The addition of Status410Gone response with DialogDeleted message is appropriate for indicating that the dialog being updated has been deleted. This change improves the API's clarity by distinguishing between validation errors (400) and attempts to update deleted resources (410).
Let's verify the consistency of this change across the API documentation:
✅ Verification successful
410 Gone status is consistently documented across related endpoints
The verification shows that Status410Gone with DialogDeleted message is consistently implemented across all relevant endpoints:
- Dialog operations (Update, Delete)
- Dialog-related operations (CreateDialogTransmission, CreateDialogActivity)
- End-user operations (SearchDialogLabelAssignmentLog, SetDialogSystemLabel)
In all cases, it's properly placed between other status codes and uses the same message constant (Constants.SwaggerSummary.DialogDeleted).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the 410 Gone status is consistently documented across related endpoints
# Expected: Find similar patterns in related endpoint documentation
rg -A 2 "Status410Gone" "src/Digdir.Domain.Dialogporten.WebApi/Endpoints/"
Length of output: 5554
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogActivities/Create/CreateDialogActivityEndpointSummary.cs (1)
26-26: Verify if 410 Gone is appropriate for create operations.
While the PR objectives specifically mention changing 400 to 410 for update/delete operations, this change extends to create operations. This is logically consistent since activities cannot be created for deleted dialogs. However, please verify if this extension aligns with the intended API behavior.
Let's check the related implementation:
✅ Verification successful
Based on the implementation details and error handling patterns found, I can now generate the final response:
410 Gone is appropriate for create operations on deleted dialogs
The implementation shows that the endpoint first checks for dialog existence using GetDialogQuery, which can return both "not found" and "deleted" states. The consistent use of 410 Gone across other endpoints (CreateDialogTransmission, UpdateDialog) for deleted dialogs shows this is an intentional pattern in the API design. This maintains a clear distinction between:
- 404 Not Found: Dialog never existed
- 410 Gone: Dialog existed but was deleted
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the create operation handler returns 410 Gone
# Expected: Find corresponding implementation in the endpoint handler
ast-grep --pattern 'class CreateDialogActivityEndpoint {
$$$
public async $_ $_($$_) {
$$$
GoneAsync($$$)
$$$
}
$$$
}'
Length of output: 140
Script:
#!/bin/bash
# Let's try a different approach to find the implementation
# First, let's locate the file containing CreateDialogActivityEndpoint
fd "CreateDialogActivityEndpoint" --type f
# Then let's examine the content for Gone status handling
rg -l "Status410Gone|GoneAsync" --type cs
# Finally, let's look at the actual implementation
rg -A 10 "CreateDialogActivityEndpoint" --type cs
Length of output: 7329
Script:
#!/bin/bash
# Let's check the implementation details of the endpoint
rg -A 20 "public override async Task HandleAsync" src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogActivities/Create/CreateDialogActivityEndpoint.cs
# Also check for any dialog-related exceptions that might trigger 410
rg -A 5 "DialogDeleted|DialogNotFound" --type cs
Length of output: 21656
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Delete/DeleteDialogEndpointSummary.cs (1)
26-26: LGTM! The addition of 410 Gone status is well documented.
The addition of the 410 Gone status code is consistent with the PR objective and aligns well with the existing documentation that already mentions this behavior for end users.
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Delete/DeleteDialogEndpoint.cs (1)
30-30: Verify consistent handling of deleted dialogs across the codebase.
The changes correctly implement the 410 Gone status for deleted dialogs. Let's verify the consistency of this implementation across related components.
Also applies to: 41-41
✅ Verification successful
Consistent handling of deleted dialogs is properly implemented across the codebase
The verification shows that:
- No instances of BadRequest responses for deleted dialogs were found
- GoneAsync is consistently used across all relevant endpoints (Delete, Update, CreateDialogTransmission, CreateDialogActivity)
- Status410Gone is properly documented in all related endpoint summaries and implementation files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent handling of deleted dialogs across the codebase
# Check for any remaining instances of BadRequest responses for deleted dialogs
ast-grep --pattern 'badRequest => this.BadRequestAsync($_, $_)'
# Check for consistent use of GoneAsync for deleted dialogs
ast-grep --pattern 'gone => this.GoneAsync($_, $_)'
# Check for consistent documentation of 410 Gone status
rg -l "Status410Gone"
Length of output: 2140
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Delete/DeleteDialogCommand.cs (1)
22-22: LGTM! Result type change aligns with PR objectives
The addition of EntityDeleted to the DeleteDialogResult type correctly implements the change from 400 to 410 status code.
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogActivities/Create/CreateDialogActivityEndpoint.cs (2)
38-38: LGTM! Added 410 Gone status code
The addition of Status410Gone to the ProducesOneOf method properly documents the new response status in the endpoint configuration.
70-70: LGTM! Added handling for Gone status
The addition of the gone => this.GoneAsync(gone, ct) case in the Match method properly handles the 410 Gone response from the command handler.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs (1)
34-34: LGTM! Added EntityDeleted to result types
The addition of EntityDeleted to the UpdateDialogResult OneOf type list properly supports the new 410 Gone response scenario.
docs/schema/V1/swagger.verified.json (1)
5948-5950: LGTM! Added 410 Gone response documentation
The OpenAPI specification has been properly updated to document the 410 Gone response for all relevant endpoints:
- DELETE /api/v1/serviceowner/dialogs/{dialogId}
- PUT /api/v1/serviceowner/dialogs/{dialogId}
- POST /api/v1/serviceowner/dialogs/{dialogId}/activities
- POST /api/v1/serviceowner/dialogs/{dialogId}/transmissions
The description "Entity with the given key(s) is removed" is clear and consistent across all endpoints.
Also applies to: 6199-6201, 6497-6499, 6842-6844
...Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/DeleteDialogTests.cs
Show resolved
Hide resolved
…te-deleted-dialog' into fix/change-response-code-on-update-deleted-dialog
oskogstad
deleted the
fix/change-response-code-on-update-deleted-dialog
branch
|
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
Release Notes
New Features
410 Goneresponse status forDELETE,PUT, andPOSToperations related to dialog management, enhancing clarity on resource deletion.Bug Fixes
Tests