fix(webapi): Repeat delete requests should return 400 BAD REQUEST #1542
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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
|
![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 (4)
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Delete/DeleteDialogEndpoint.cs (1)
Line range hint
21-31: Add 410 GONE to API documentationThe endpoint configuration needs to be updated to document the new 410 GONE response for already deleted dialogs. This is important for maintaining accurate API documentation.
Apply this diff:
Description(b => b.ProducesOneOf( StatusCodes.Status204NoContent, StatusCodes.Status404NotFound, + StatusCodes.Status410Gone, StatusCodes.Status412PreconditionFailed));src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Delete/DeleteDialogCommand.cs (3)
47-47: Consider index implications of ignoring query filters.While
IgnoreQueryFilters()is necessary here to check for deleted dialogs, it might bypass database indexes that include the deletion status. This could impact query performance if there are many soft-deleted records.Consider adding a covering index that includes both the
IdandDeletedcolumns to optimize this query pattern.
55-60: Enhance error messaging for API consumers.While the logic is correct, consider adding more context to help API consumers understand why they're receiving a 410 GONE response and what actions they can take.
- return new EntityDeleted<DialogEntity>(request.Id); + return new EntityDeleted<DialogEntity>(request.Id, + "The requested dialog has been deleted. No further action is required.");
57-58: Make the TODO more specific.The TODO comment could be more specific about what needs to be updated when restoration is implemented.
- // TODO: When restoration is implemented, add a hint to the error message. - // https://github.com/digdir/dialogporten/pull/406 + // TODO: When dialog restoration is implemented (PR #406), + // update the error message to include instructions for restoration if applicable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Delete/DeleteDialogCommand.cs(2 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Delete/DeleteDialogEndpoint.cs(1 hunks)
🔇 Additional comments (4)
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Delete/DeleteDialogEndpoint.cs (2)
38-42: LGTM! Implementation correctly handles deleted dialog case
The addition of the deleted case in the result matching follows the existing pattern and correctly implements the 410 GONE response for deleted dialogs, fulfilling the PR objective.
38-42: Verify DeleteDialogCommand implementation
Let's verify that the command handler correctly identifies already deleted dialogs.
✅ Verification successful
Based on the search results, I can now generate the final response as we have sufficient information about the DeleteDialogCommand implementation and soft delete handling:
DeleteDialogCommand correctly handles already deleted dialogs
The implementation properly handles repeated delete requests through:
- Soft delete implementation via
ISoftDeletableEntityinterface onDialogEntity - Proper status checks in
DeleteDialogCommandHandlerthat returnsEntityDeletedresult when attempting to delete an already deleted dialog - Consistent handling in the endpoint that maps
EntityDeletedresult to HTTP 410 Gone response - Domain events (
DialogDeletedDomainEvent) are properly published when a dialog is deleted
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for DeleteDialogCommand handler implementation
ast-grep --pattern 'class DeleteDialogCommandHandler {
$$$
Handle($_) {
$$$
}
}'
# Search for related dialog entity definition to verify soft delete implementation
rg -A 5 "class Dialog.*{"
Length of output: 214
Script:
#!/bin/bash
# Search for DeleteDialogCommand handler implementation with a more precise pattern
ast-grep --pattern 'class DeleteDialogCommandHandler'
# Then search for Handle method implementation
rg -A 20 "Handle.*DeleteDialogCommand"
# Search for Dialog class definition with fixed regex
rg -A 5 "class Dialog\s*{"
# Search for any soft delete related properties or methods in Dialog
rg -A 3 "IsDeleted|DeletedAt|Delete"
Length of output: 100702
Script:
#!/bin/bash
# Search for DeleteDialogCommand handler with fixed pattern
rg -A 20 "class DeleteDialogCommandHandler"
# Search for Dialog class and related soft delete properties
rg -A 10 "class DialogEntity"
# Search for soft delete related methods in DialogEntity
rg -A 5 "Delete.*Dialog"
Length of output: 78824
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Delete/DeleteDialogCommand.cs (2)
22-22: LGTM! The OneOf type extension is well-structured.
The addition of EntityDeleted to the result types aligns with the PR objective and follows the existing pattern.
Line range hint 22-77: Verify consistent handling of deleted dialogs across endpoints.
The implementation looks good overall, but let's verify that other endpoints consistently handle deleted dialogs.
✅ Verification successful
Consistent handling of deleted dialogs confirmed across endpoints
The verification shows that all dialog-related endpoints consistently:
- Use
IgnoreQueryFilters()to include soft-deleted records in queries - Check for null dialog entities first
- Have proper access control via
WhereIfconditions for service owner endpoints
The implementation in DeleteDialogCommand aligns with the codebase's overall approach to soft deletion and follows the established pattern for handling deleted entities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent handling of deleted dialogs across endpoints
# and handlers to ensure uniform behavior.
# Look for other endpoints that might need similar handling
ast-grep --pattern 'class $_Endpoint {
$$$
HandleAsync($$$) {
$$$
}
}'
# Check for other uses of IgnoreQueryFilters
rg "IgnoreQueryFilters\(\)" -A 5
# Look for other dialog deletion related code
rg "Deleted\s*=\s*true" -A 5
Length of output: 19605
...ogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Delete/DeleteDialogCommand.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Delete/DeleteDialogCommand.cs (2)
47-47: Consider performance impact of ignoring query filters.While
IgnoreQueryFilters()is necessary to detect soft-deleted dialogs, it might impact query performance by scanning all records, including deleted ones. Consider adding an index on theDeletedcolumn if you expect a significant number of soft-deleted records.
55-60: Enhance error message for API consumers.While the logic is correct, consider enhancing the error message to provide more context to API consumers. This could include when the entity was deleted and potentially how to handle this situation.
- return new EntityDeleted<DialogEntity>(request.Id); + return new EntityDeleted<DialogEntity>(request.Id) + { + Message = $"Dialog {request.Id} was previously deleted. Please create a new dialog or contact support if you need to restore this dialog." + };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
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(1 hunks)
🔇 Additional comments (2)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Delete/DeleteDialogCommand.cs (2)
22-22: LGTM! Good use of OneOf for type-safe result handling.
The addition of EntityDeleted to the result types properly models the "already deleted" state, enabling correct HTTP status code mapping (410 GONE) at the endpoint level.
Line range hint 22-60: Verify consistent implementation across endpoints.
Let's ensure this pattern is consistently implemented across all delete operations and properly mapped to HTTP status codes.
✅ Verification successful
Implementation verified and consistent with best practices
The delete operation implementation is properly structured and follows consistent patterns:
- The
DeleteDialogEndpointcorrectly mapsEntityDeletedto HTTP 410 Gone status using theGoneAsyncextension method - The pattern is consistently used across the codebase, with
EntityDeleted<T>being used in multiple related operations - The endpoint properly documents its response codes in the OpenAPI description
- The implementation includes proper error handling for various scenarios (not found, forbidden, concurrency)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent implementation of delete operations and status code mapping
# Check for other delete operations that should follow this pattern
ast-grep --pattern 'class $_DeleteCommand : IRequest<$_Result>'
# Check for proper HTTP status code mapping
rg -A 5 "EntityDeleted.*GoneAsync"
Length of output: 102
Script:
#!/bin/bash
# Let's try a different approach to find delete operations and their mappings
# Find all delete command handlers
ast-grep --pattern 'class $_DeleteCommandHandler : IRequestHandler<$_DeleteCommand, $_>'
# Find all delete-related result types
ast-grep --pattern 'class $_DeleteResult : OneOfBase<$_>'
# Look for HTTP response mappings in controller endpoints
rg -l "Delete.*Controller"
Length of output: 179
Script:
#!/bin/bash
# Let's search for delete-related patterns more broadly
# Find files containing delete operations
rg -l "Delete.*Command"
# Search for EntityDeleted type usage
rg "EntityDeleted[<]"
# Look for HTTP response mapping patterns
rg "IResult.*Gone"
# Search for delete endpoints in controllers
rg "Delete.*async"
Length of output: 3063
Script:
#!/bin/bash
# Let's examine the delete endpoint implementation and response mapping
# Check the delete endpoint implementation
cat src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Delete/DeleteDialogEndpoint.cs
# Look for HTTP response mapping patterns in the Results folder
rg -l "Results"
Length of output: 3462
...ogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs
Show resolved
Hide resolved
...ogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs
Show resolved
Hide resolved
oskogstad
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Delete/DeleteDialogEndpointSummary.cs (1)
Line range hint
12-21: Update documentation to include repeated delete behaviorThe description thoroughly explains soft delete behavior and concurrency control but should be updated to mention that attempting to delete an already deleted dialog will result in a 400 Bad Request response.
Add this information to the description:
Note that the dialog will still be available on the single details endpoint, but will have a deleted status. It will not appear on the list endpoint for either service owners nor end users. If end users attempt to access the dialog via the details endpoint, they will get a 410 Gone response. + +Attempting to delete an already deleted dialog will result in a 400 Bad Request response. Optimistic concurrency control is implemented using the If-Match header.src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Delete/DeleteDialogCommand.cs (1)
55-60: Consider making the error message more user-friendly.While the logic is correct, the error message could be more user-friendly by focusing on the business scenario rather than technical details.
- return new BadRequest($"Entity '{nameof(DialogEntity)}' with key '{request.Id}' is already removed, and cannot be deleted again."); + return new BadRequest($"The dialog has already been deleted and cannot be deleted again.");docs/schema/V1/swagger.verified.json (1)
5922-5931: Consider enhancing the error documentation for repeated delete scenarios.While the 400 response definition is technically correct, consider adding more specific documentation about the error codes and messages that will be returned when attempting to delete an already deleted dialog. This would help API consumers better handle this specific case.
Add error example for repeated delete case:
"400": { "content": { "application/problem+json": { "schema": { "$ref": "#/components/schemas/ProblemDetails" - } + }, + "example": { + "type": "https://www.rfc-editor.org/rfc/rfc7231#section-6.5.1", + "title": "Validation error occurred", + "status": 400, + "detail": "Dialog is already deleted", + "errors": [ + { + "code": "DialogAlreadyDeleted", + "name": "dialogId", + "reason": "The dialog has already been deleted" + } + ] + } } }, "description": "Validation error occurred. See problem details for a list of errors." }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
docs/schema/V1/swagger.verified.json(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Delete/DeleteDialogCommand.cs(2 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)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Delete/DeleteDialogEndpoint.cs
🔇 Additional comments (4)
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Delete/DeleteDialogEndpointSummary.cs (1)
24-24: Consider using a more specific error message constant
The current implementation uses a generic ValidationError constant for the 400 Bad Request response. Consider creating a more specific constant that better describes the "already deleted" scenario.
Let's check the available constants:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Delete/DeleteDialogCommand.cs (2)
22-22: LGTM! Result type extension aligns with PR objective.
The addition of BadRequest to the DeleteDialogResult type properly supports the new requirement to handle repeated delete requests.
47-47: Verify consistent usage of IgnoreQueryFilters for soft-deleted dialogs.
The addition of IgnoreQueryFilters() is correct and necessary to check for already deleted dialogs.
✅ Verification successful
IgnoreQueryFilters is consistently used across dialog-related queries
The verification shows that IgnoreQueryFilters() is consistently used across all dialog-related queries where soft-deleted records need to be accessed. The codebase follows a global query filter pattern for soft deletion (via HasQueryFilter(x => !x.Deleted)), and IgnoreQueryFilters() is appropriately used to bypass this filter when needed. This is evident in:
- All dialog retrieval operations (Get/Search)
- Dialog management operations (Delete/Update/Purge)
- Related entity queries (Activities/Transmissions/SeenLogs)
- Both ServiceOwner and EndUser features
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if IgnoreQueryFilters is consistently used when checking deleted status
# Look for other places where we might need to check deleted status but might be missing IgnoreQueryFilters
# Search for dialog deletion related queries
rg -l "Deleted\s*[=|!]" | xargs rg "FirstOrDefaultAsync|SingleOrDefaultAsync|AnyAsync" -B 2
Length of output: 93
Script:
#!/bin/bash
# Let's try a different approach to find soft deletion patterns
# First, let's look for IgnoreQueryFilters usage
rg "IgnoreQueryFilters" -B 2 -A 2
# Then, let's search for Deleted property in entity configurations
ast-grep --pattern 'public bool Deleted { get; set; }'
# Also search for global query filters that might be applying soft deletion
rg "HasQueryFilter" -B 2 -A 2
Length of output: 17164
docs/schema/V1/swagger.verified.json (1)
5922-5931: LGTM! The 400 response definition is well structured.
The added 400 response for the delete endpoint follows OpenAPI best practices:
- Uses proper content type (application/problem+json)
- References the standard ProblemDetails schema
- Includes clear description of the error scenario
|
🤖 I have created a release *beep* *boop* --- ## [1.40.1](v1.40.0...v1.40.1) (2024-11-29) ### Bug Fixes * **webapi:** Repeat delete requests should return 400 BAD REQUEST ([#1542](#1542)) ([f14861d](f14861d)) --- 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