feat(webapi): Add ETag to response headers #1645
Conversation
📝 WalkthroughWalkthroughThis pull request introduces comprehensive changes to the Dialogporten API, focusing on enhancing response handling for dialog-related operations. The key modification is the addition of an 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/return-revision-header
branch
from
176b864 to
0fff9b7
Compare
oskogstad
changed the title
oskogstad
force-pushed
the
feat/return-revision-header
branch
from
394a061 to
19d7fad
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/EndUser/Dialogs/Queries/DeletedDialogTests.cs (1)
Line range hint
19-29: Add ETag verification to deletion testSince we're implementing ETag support, this test should also verify the ETag behavior for deleted resources.
var dialogId = createDialogResponse.AsT0.DialogId; +// Store the ETag from creation response +var originalETag = createDialogResponse.AsT0.ETag; + var deleteDialogCommand = new DeleteDialogCommand { Id = dialogId }; -await Application.Send(deleteDialogCommand); +var deleteResponse = await Application.Send(deleteDialogCommand); + +// Verify ETag changes after deletion +deleteResponse.AsT0.ETag.Should().NotBeNull(); +deleteResponse.AsT0.ETag.Should().NotBe(originalETag); // Act var getDialogQuery = new GetDialogQuery { DialogId = dialogId }; var getDialogResponse = await Application.Send(getDialogQuery); // Assert getDialogResponse.TryPickT2(out var entityDeleted, out _).Should().BeTrue(); entityDeleted.Should().NotBeNull(); entityDeleted.Message.Should().Contain("is removed"); entityDeleted.Message.Should().Contain(dialogId.ToString()); +// Verify ETag is not present in Gone response +entityDeleted.ETag.Should().BeNull();tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Transmissions/Commands/UpdateTransmissionTests.cs (1)
ETag-related test coverage is missing
The codebase lacks tests for the ETag functionality being added. Specifically:
- No tests verify ETag generation and validation
- No tests cover conditional request handling (If-Match/If-None-Match)
- The WebApi integration test suite, which should handle HTTP concerns, is minimal
Recommended locations for new tests:
tests/Digdir.Domain.Dialogporten.WebApi.Integration.Tests/Features/V1/- Add tests for HTTP-level ETag handlingtests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/UpdateDialogTests.cs- Add tests for ETag generation during updates🔗 Analysis chain
Line range hint
1-134: Consider adding ETag-related test cases.While the existing test coverage for transmission updates is good, considering this PR's objective of adding ETag support, it would be beneficial to add test cases that verify:
- ETag generation for transmission operations
- Conditional request handling using ETags
- Proper ETag updates when transmissions are modified
Let's check if ETag tests exist elsewhere:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ETag-related tests rg -g '*.cs' -i '(etag|if-match|if-none-match)' tests/Length of output: 56
Script:
#!/bin/bash # Search for HTTP-related test files and conditional request patterns echo "=== HTTP Test Files ===" fd -e cs -p 'http.*test' tests/ echo -e "\n=== Feature Test Files ===" fd -e cs -p 'features.*test' tests/ echo -e "\n=== Conditional Request Patterns ===" rg -g '*.cs' -i '(conditional|precondition|http.*request)' tests/Length of output: 3124
Script:
#!/bin/bash # Check content of potential ETag test locations echo "=== WebApi Tests ===" rg -A 5 -B 5 "class" tests/Digdir.Domain.Dialogporten.WebApi.Integration.Tests/Features/V1/SwaggerSnapshotTests.cs echo -e "\n=== Update Dialog Tests ===" rg -A 5 -B 5 "class" tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/UpdateDialogTests.cs echo -e "\n=== WebApi Directory Structure ===" fd . tests/Digdir.Domain.Dialogporten.WebApi.Integration.Tests/Length of output: 1863
🧹 Nitpick comments (15)
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Delete/DeleteDialogEndpoint.cs (2)
39-43: LGTM! The ETag header implementation aligns with HTTP/1.1 specifications.The implementation correctly adds the ETag header to the response before returning 204 No Content, which is essential for conditional requests and caching.
However, consider wrapping the revision GUID in quotes as per RFC 7232:
- HttpContext.Response.Headers.Append(Constants.ETag, success.Revision.ToString()); + HttpContext.Response.Headers.Append(Constants.ETag, $"\"{success.Revision}\"");
Line range hint
1-58: Excellent implementation of conditional requests and response caching!The endpoint implementation follows REST best practices with proper:
- HTTP method usage
- Status code handling
- Conditional request support via If-Match
- Authorization checks
Consider documenting the ETag behavior in the API documentation to help clients implement proper caching strategies.
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Update/UpdateDialogEndpointSummary.cs (1)
Line range hint
14-17: Update the "link TBD" placeholder in the documentation.The description contains a placeholder for documentation link. Please update this with the actual documentation URL to ensure complete API documentation.
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/GetDialogTests.cs (1)
Line range hint
53-53: Add ETag-specific test casesThe TODO comment presents an opportunity to add comprehensive ETag-related test cases.
Consider adding tests for:
- Conditional GET with matching If-None-Match header (should return 304 Not Modified)
- Conditional GET with non-matching If-None-Match header (should return 200 OK)
- ETag changes after dialog updates
- Weak vs strong ETag validation
Would you like me to help generate these test cases or create a GitHub issue to track this task?
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/EndUser/Dialogs/Queries/ActivityLogTests.cs (2)
76-76: LGTM! Consider adding ETag-related testsWhile the property name change is good, given that this PR adds ETag support to responses, consider adding tests to verify:
- ETag header presence in responses
- Proper ETag generation for different dialog states
- Conditional request handling using If-Match/If-None-Match headers
Would you like me to help generate the test cases for ETag verification?
Line range hint
1-100: Add comprehensive ETag testing suiteWhile the current tests cover activity log functionality well, the PR's main objective of adding ETag support isn't tested in this file. Consider adding a new test class
DialogEtagTests.csto verify:
- ETag generation and response headers
- Conditional requests with If-Match/If-None-Match
- ETag changes on resource modifications
- Proper 304 Not Modified responses
Would you like me to help create a new test class with comprehensive ETag test coverage?
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Delete/DeleteDialogEndpointSummary.cs (1)
Line range hint
13-20: Update the TBD documentation link.The description mentions "documentation (link TBD)". Please update this with the actual documentation link to ensure proper reference for API consumers.
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/SeenLogTests.cs (1)
Line range hint
1-124: Consider adding ETag-related test cases.While the existing tests thoroughly cover seen log functionality, consider adding test cases to verify the new ETag header implementation mentioned in the PR objectives. This could include:
- Verifying ETag presence in responses
- Testing conditional requests with If-Match/If-None-Match headers
- Validating ETag changes when dialog state is modified
Would you like me to help draft the additional test cases for ETag verification?
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/Common/Headers/HttpResponseHeaderExamples.cs (1)
8-12: Enhance ETag header description and add status code validationThe implementation looks good but could benefit from:
- More specific description about the ETag format being a UUID
- Validation for appropriate status code ranges (2xx/3xx)
public static ResponseHeader NewDialogETagHeader(int statusCode) { + if (statusCode < 200 || statusCode > 399) + throw new ArgumentException("ETag headers should only be used with 2xx and 3xx status codes", nameof(statusCode)); + return new(statusCode, Constants.ETag) { - Description = "The new UUID ETag of the dialog", + Description = "A UUID-formatted ETag representing the current version of the dialog", }; }src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Patch/ProducesResponseHeaderAttribute.cs (1)
6-11: Add parameter validation in the constructorThe attribute implementation looks good, but should validate the input parameters to prevent invalid usage.
public ProducesResponseHeaderAttribute(int statusCode, string headerName, string description) { + if (string.IsNullOrWhiteSpace(headerName)) + throw new ArgumentException("Header name cannot be empty", nameof(headerName)); + if (string.IsNullOrWhiteSpace(description)) + throw new ArgumentException("Description cannot be empty", nameof(description)); + if (statusCode < 100 || statusCode > 599) + throw new ArgumentException("Status code must be between 100 and 599", nameof(statusCode)); + HeaderName = headerName; StatusCode = statusCode; Description = description; }src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Create/CreateDialogEndpoint.cs (1)
40-40: Consider extracting ETag header setting to an extension method.To improve maintainability and reusability, consider creating an extension method for setting ETag headers. This would help maintain consistency across endpoints and reduce code duplication.
Example implementation:
+ // Add to HttpResponseExtensions.cs + public static class HttpResponseExtensions + { + public static void SetETagHeader(this HttpResponse response, string revision) + { + response.Headers.Append(Constants.ETag, revision.ToString()); + } + } - HttpContext.Response.Headers.Append(Constants.ETag, success.Revision.ToString()); + HttpContext.Response.SetETagHeader(success.Revision);src/Digdir.Domain.Dialogporten.WebApi/Common/Constants.cs (1)
6-6: Follow HTTP header naming convention.Consider using "ETag" instead of "Etag" to align with the conventional HTTP header capitalization.
- internal const string ETag = "Etag"; + internal const string ETag = "ETag";tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/DeleteDialogTests.cs (1)
62-81: Enhance test coverage for revision handling.While the test verifies basic revision change, consider adding test cases for:
- Concurrent modifications using If-Match header
- Invalid/outdated revision in If-Match header
- Missing If-Match header when required
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogTransmissions/Create/CreateDialogTransmissionEndpoint.cs (1)
69-75: Consider adding Cache-Control headers.Since you're implementing ETag for conditional requests, consider adding appropriate Cache-Control headers to guide client caching behavior.
success => { HttpContext.Response.Headers.Append(Constants.ETag, $"\"{success.Revision}\""); + HttpContext.Response.Headers.Append("Cache-Control", "private, must-revalidate"); return SendCreatedAtAsync<GetDialogTransmissionEndpoint>( new GetTransmissionQuery { DialogId = dialog.Id, TransmissionId = req.Id.Value }, req.Id, cancellation: ct);src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Patch/PatchDialogsController.cs (1)
93-93: Consider using weak ETags for this use case.Since the ETag is based on a revision number and doesn't guarantee byte-for-byte equality of responses, consider using weak ETags (W/ prefix) as per RFC 7232. This would better represent that two different representations might be semantically equivalent even if they're not byte-for-byte identical.
- HttpContext.Response.Headers.Append(Constants.ETag, success.Revision.ToString()); + HttpContext.Response.Headers.Append(Constants.ETag, $"W/\"{success.Revision}\"");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
docs/schema/V1/swagger.verified.json(6 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs(2 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/Common/Headers/HttpResponseHeaderExamples.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogActivities/Create/CreateDialogActivityEndpoint.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogActivities/Create/CreateDialogActivityEndpointSummary.cs(2 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogTransmissions/Create/CreateDialogTransmissionEndpoint.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogTransmissions/Create/CreateDialogTransmissionEndpointSummary.cs(2 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Create/CreateDialogEndpoint.cs(2 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Create/CreateDialogEndpointSummary.cs(2 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Delete/DeleteDialogEndpoint.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Delete/DeleteDialogEndpointSummary.cs(2 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Patch/PatchDialogsController.cs(2 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Patch/ProducesResponseHeaderAttribute.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Patch/ProducesResponseHeaderOperationProcessor.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Update/UpdateDialogEndpoint.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Update/UpdateDialogEndpointSummary.cs(2 hunks)src/Digdir.Domain.Dialogporten.WebApi/Program.cs(2 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/EndUser/Dialogs/Queries/ActivityLogTests.cs(2 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/EndUser/Dialogs/Queries/DeletedDialogTests.cs(1 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/EndUser/Dialogs/Queries/SeenLogTests.cs(4 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs(5 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/DeleteDialogTests.cs(3 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/UpdateDialogTests.cs(4 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/ActivityLogTests.cs(2 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/GetDialogTests.cs(2 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/SeenLogTests.cs(4 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/NotificationCondition/NotificationConditionTests.cs(3 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Transmissions/Commands/CreateTransmissionTests.cs(2 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Transmissions/Commands/UpdateTransmissionTests.cs(6 hunks)
🧰 Additional context used
📓 Learnings (4)
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/GetDialogTests.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.
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Create/CreateDialogEndpoint.cs (2)
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.
Learnt from: oskogstad
PR: digdir/dialogporten#1210
File: tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs:282-282
Timestamp: 2024-11-12T05:32:45.312Z
Learning: Using square brackets `[]` for collection initialization is acceptable in the codebase's C# code.
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs (2)
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.
Learnt from: oskogstad
PR: digdir/dialogporten#1210
File: tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs:282-282
Timestamp: 2024-11-12T05:32:45.312Z
Learning: Using square brackets `[]` for collection initialization is acceptable in the codebase's C# code.
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.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dry run deploy apps / Deploy job sync-resource-policy-information-job to test
- GitHub Check: Dry run deploy apps / Deploy job sync-subject-resource-mappings-job to test
🔇 Additional comments (39)
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Update/UpdateDialogEndpointSummary.cs (2)
4-4: LGTM! Using directive aligns with ETag implementation.The addition of the Headers namespace import is appropriate for implementing ETag support.
19-19: LGTM! ETag header documentation is properly configured.The ResponseHeaders initialization correctly documents the ETag header for the 204 No Content response, which aligns with REST best practices for resource updates.
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/EndUser/Dialogs/Queries/ActivityLogTests.cs (2)
22-22: LGTM! Property name change improves clarityThe change from
.Valueto.DialogIdmakes the code more semantically clear while maintaining the same functionality.
66-70: LGTM! Consistent property access patternThe DialogId property access follows the same pattern as other changes, maintaining consistency across the codebase.
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Delete/DeleteDialogEndpointSummary.cs (2)
4-4: LGTM! Import aligns with ETag implementation.The added using directive is correctly placed and necessary for the ETag header implementation.
22-22: Implementation matches API contract and documentation.The ETag header is correctly added with the appropriate 204 status code, aligning with the delete operation's behavior and the documented optimistic concurrency control strategy.
Let's verify the consistency of ETag implementation across other endpoints:
✅ Verification successful
ETag header implementation verified as consistent across endpoints
The ETag header is correctly implemented across all dialog-modifying endpoints with appropriate status codes (201 for creation, 204 for modifications/deletions) and proper optimistic concurrency control documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent ETag header implementation across endpoints # Expected: All endpoints that modify dialogs should implement ETag headers # Search for other endpoint summaries and verify ETag header implementation rg -l "EndpointSummary" | xargs rg "ResponseHeaders.*ETagHeader" # Verify If-Match header documentation in other endpoints rg -A 5 "If-Match header" "src/Digdir.Domain.Dialogporten.WebApi"Length of output: 6797
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/SeenLogTests.cs (5)
51-51: LGTM! Consistent with the DialogId property change.The change maintains consistency with the updated API contract.
79-83: LGTM! Consistent formatting and property access.The changes maintain consistency with the DialogId property pattern and improve readability through proper formatting.
89-89: LGTM! Consistent property access.The change maintains consistency with the DialogId property pattern.
110-110: LGTM! Completes consistent DialogId property usage.The changes complete the consistent pattern of using the DialogId property throughout all test methods.
Also applies to: 115-115
24-24: Verify API contract change for DialogId property.The change from
ValuetoDialogIdproperty looks good and the object initializer syntax improves readability. However, let's verify this API change is documented and consistent across the codebase.Also applies to: 27-30
✅ Verification successful
API change from Value to DialogId is consistent across the codebase
The change is verified to be consistently implemented across all test files, with no remaining instances of the old
.Valueproperty.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining uses of .Value property in dialog-related code rg -g '*.cs' 'createCommandResponse\.AsT0\.Value' # Search for the new DialogId property usage pattern rg -g '*.cs' 'createCommandResponse\.AsT0\.DialogId'Length of output: 6896
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Delete/DeleteDialogCommand.cs (3)
22-22: LGTM! Return type updated to support ETag.The change from
SuccesstoDeleteDialogSuccessin the OneOf type enables proper ETag support while maintaining all existing error cases.
24-24: LGTM! Well-designed success type for ETag support.The immutable record type with a
Revisionproperty is a clean way to carry the ETag information.
75-77: Verify ETag header integration in the API layer.The command handler now correctly returns the dialog revision for ETag support. Please ensure that the API endpoint properly sets the
ETagheader using this revision value.✅ Verification successful
ETag header integration is properly implemented
The DeleteDialogEndpoint correctly sets the ETag header using the revision from DeleteDialogSuccess and handles If-Match precondition checks appropriately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the corresponding endpoint to verify ETag header setting ast-grep --pattern 'class $_DialogEndpoint { $$$ public $_ $_($_, $_) { $$$ .WithHeader("ETag", $$$) $$$ } $$$ }'Length of output: 135
Script:
#!/bin/bash # Search for classes handling dialog deletion ast-grep --pattern 'class $_ { $$$ Delete$_Dialog($_) { $$$ } $$$ }' # Search for ETag header usage rg -l "ETag|If-Match" --type cs # Search for DeleteDialog related files fd "Delete.*Dialog.*\.cs$"Length of output: 2422
Script:
#!/bin/bash # Check the DeleteDialogEndpoint implementation cat "src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Delete/DeleteDialogEndpoint.cs"Length of output: 2228
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Create/CreateDialogEndpointSummary.cs (1)
4-4: LGTM! ETag header correctly configured for dialog creation response.The addition of the ETag header to the response documentation is well-implemented and aligns with the PR objectives.
Also applies to: 22-22
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogActivities/Create/CreateDialogActivityEndpointSummary.cs (1)
4-4: LGTM! ETag header correctly configured for dialog activity creation response.The implementation aligns with the optimistic concurrency strategy mentioned in the description.
Also applies to: 22-22
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogTransmissions/Create/CreateDialogTransmissionEndpointSummary.cs (1)
4-4: LGTM! ETag header correctly configured for dialog transmission creation response.The implementation maintains consistency with other endpoints and supports optimistic concurrency.
Also applies to: 22-22
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Create/CreateDialogEndpoint.cs (1)
38-43: LGTM! ETag header correctly implemented in the response handling.The implementation properly sets the ETag header using the revision value before sending the created response.
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/ActivityLogTests.cs (1)
23-23: LGTM!The property access updates maintain test functionality while improving code consistency.
Also applies to: 67-70, 77-77
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/EndUser/Dialogs/Queries/SeenLogTests.cs (1)
23-23: LGTM! Consistent DialogId access pattern.The changes standardize how DialogId is accessed across all test methods, improving code consistency and maintainability.
Also applies to: 45-45, 73-76, 83-83, 104-104, 109-109
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/NotificationCondition/NotificationConditionTests.cs (1)
43-43: LGTM! Consistent DialogId access pattern.The changes maintain a consistent pattern for accessing DialogId across notification condition tests, improving code clarity and maintainability.
Also applies to: 96-96, 98-98, 100-100, 109-109
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/UpdateDialogTests.cs (2)
19-47: LGTM! Well-structured test for revision updates.The new test method effectively verifies that dialog updates generate new revisions, which is crucial for proper ETag functionality. The test follows good practices with clear arrange-act-assert pattern.
54-54: LGTM! Consistent DialogId access pattern.The changes maintain a consistent pattern for accessing DialogId across update dialog tests, improving code clarity and maintainability.
Also applies to: 72-76, 93-93, 113-117
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Patch/PatchDialogsController.cs (1)
63-63: LGTM! Proper ETag implementation.The changes correctly implement ETag support for the PATCH endpoint, with proper documentation and header handling.
Also applies to: 91-95
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs (2)
24-24: LGTM! Good use of records for immutable data.The record type provides a clean way to encapsulate both the dialog ID and revision information.
27-27: LGTM! Improved type safety with dedicated success type.The change from
Success<Guid>toCreateDialogSuccessprovides better type safety and clearer intent.Also applies to: 98-98
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (2)
18-18: LGTM! Added required namespace.The new using directive is needed for the PATCH operations support.
129-131: LGTM! Enhanced OpenAPI documentation.Good addition of the response header processor to properly document ETag headers in Swagger.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs (2)
33-35: LGTM! Consistent pattern with CreateDialog.Good use of records and maintaining consistency with the CreateDialog implementation pattern.
171-171: LGTM! Proper revision handling.Correctly returning the dialog revision for ETag support.
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs (2)
86-86: LGTM! Updated assertions for new success type.Correctly updated all assertions to use
DialogIdinstead ofValuefollowing the type change.Also applies to: 101-101, 116-116, 139-139, 411-411
414-425: LGTM! Good test coverage for revision.Added test to verify that the revision is returned and non-empty.
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Update/UpdateDialogEndpoint.cs (1)
46-50: LGTM! Proper ETag implementation.The changes correctly implement ETag header addition using the dialog's revision value before sending the response.
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogActivities/Create/CreateDialogActivityEndpoint.cs (1)
68-77: LGTM! Consistent ETag implementation.The changes follow the same pattern as UpdateDialogEndpoint, correctly implementing ETag header addition using the dialog's revision value.
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Transmissions/Commands/CreateTransmissionTests.cs (1)
33-33: LGTM! Updated test assertions.The test assertions have been correctly updated to use
DialogIdinstead ofValue, aligning with the updated command response structures.Also applies to: 65-65
docs/schema/V1/swagger.verified.json (1)
5866-5871: LGTM! Comprehensive OpenAPI specification update.The OpenAPI specification has been thoroughly updated to document ETag headers across all relevant endpoints that modify dialog state. The documentation clearly indicates that the ETag contains the new UUID of the dialog.
Also applies to: 5940-5945, 6101-6106, 6201-6206, 6504-6509, 6872-6877
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Transmissions/Commands/UpdateTransmissionTests.cs (3)
61-61: LGTM! Property access change is consistent.The DialogId access pattern change is consistently applied here and the test logic remains intact.
Also applies to: 79-79
100-100: LGTM! Good test coverage for transmission updates.The DialogId access pattern change is consistently applied. The test suite thoroughly covers important scenarios including validation against reusing old transmission IDs.
Also applies to: 113-113
27-27: LGTM! Verify consistent DialogId access pattern across codebase.The property access change from
.Valueto.DialogIdimproves clarity. The implementation looks correct.Let's verify this pattern is consistently applied across the codebase:
Also applies to: 39-39
✅ Verification successful
Standardization of DialogId access pattern is correct
The change from
.Valueto.DialogIdis part of a consistent pattern across the codebase. Most files already use.DialogId, making this change align with the established standard.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of .Value being used to access DialogId rg -g '*.cs' '\.Value\s*[,);]' -A 2 -B 2Length of output: 44448
Script:
#!/bin/bash # Search for dialog ID access patterns rg -g '*.cs' 'DialogId.*=.*\.(Value|DialogId)' -A 2 -B 2 # Search for specific dialog-related value access ast-grep --pattern 'var $_ = $_.Value.DialogId' ast-grep --pattern 'DialogId = $_.Value'Length of output: 27098
| @@ -21,7 +21,7 @@ public async Task Get_ReturnsSimpleDialog_WhenDialogExists() | |||
| var createCommandResponse = await Application.Send(createDialogCommand); | |||
|
|
|||
| // Act | |||
| var response = await Application.Send(new GetDialogQuery { DialogId = createCommandResponse.AsT0.Value }); | |||
| var response = await Application.Send(new GetDialogQuery { DialogId = createCommandResponse.AsT0.DialogId }); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ETag verification to GetDialog tests
Since the PR adds ETag support to responses, these tests should verify that the GetDialog response includes the correct ETag header and that it matches the dialog's revision.
Consider adding assertions like:
var response = await Application.Send(new GetDialogQuery { DialogId = createCommandResponse.AsT0.DialogId });
+
+// Verify ETag presence and format
+response.AsT0.Revision.Should().NotBeEmpty("ETag should be present in the response");
+response.AsT0.Revision.Should().MatchRegex("^\"[a-f0-9]{32}\"$", "ETag should be a quoted MD5 hash");Also applies to: 44-44
| public bool Process(OperationProcessorContext context) | ||
| { | ||
| var headerAttribute = context.MethodInfo.GetCustomAttribute<ProducesResponseHeaderAttribute>(); | ||
| if (headerAttribute == null) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| var statusCode = headerAttribute.StatusCode.ToString(CultureInfo.InvariantCulture); | ||
| var response = context.OperationDescription.Operation.Responses[statusCode]; | ||
| var header = new OpenApiHeader | ||
| { | ||
| Description = headerAttribute.Description, | ||
| }; | ||
|
|
||
| response.Headers.Add(headerAttribute.HeaderName, header); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Support multiple header attributes and improve error handling
The processor needs to handle multiple attributes since ProducesResponseHeaderAttribute has AllowMultiple = true.
public bool Process(OperationProcessorContext context)
{
- var headerAttribute = context.MethodInfo.GetCustomAttribute<ProducesResponseHeaderAttribute>();
- if (headerAttribute == null)
+ var headerAttributes = context.MethodInfo.GetCustomAttributes<ProducesResponseHeaderAttribute>();
+ if (!headerAttributes.Any())
{
return true;
}
- var statusCode = headerAttribute.StatusCode.ToString(CultureInfo.InvariantCulture);
- var response = context.OperationDescription.Operation.Responses[statusCode];
- var header = new OpenApiHeader
+ foreach (var headerAttribute in headerAttributes)
{
- Description = headerAttribute.Description,
- };
+ var statusCode = headerAttribute.StatusCode.ToString(CultureInfo.InvariantCulture);
+
+ if (!context.OperationDescription.Operation.Responses.TryGetValue(statusCode, out var response))
+ {
+ throw new InvalidOperationException(
+ $"Response with status code {statusCode} not found for header {headerAttribute.HeaderName}");
+ }
+
+ var header = new OpenApiHeader
+ {
+ Description = headerAttribute.Description,
+ };
- response.Headers.Add(headerAttribute.HeaderName, header);
+ response.Headers.Add(headerAttribute.HeaderName, header);
+ }
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public bool Process(OperationProcessorContext context) | |
| { | |
| var headerAttribute = context.MethodInfo.GetCustomAttribute<ProducesResponseHeaderAttribute>(); | |
| if (headerAttribute == null) | |
| { | |
| return true; | |
| } | |
| var statusCode = headerAttribute.StatusCode.ToString(CultureInfo.InvariantCulture); | |
| var response = context.OperationDescription.Operation.Responses[statusCode]; | |
| var header = new OpenApiHeader | |
| { | |
| Description = headerAttribute.Description, | |
| }; | |
| response.Headers.Add(headerAttribute.HeaderName, header); | |
| return true; | |
| } | |
| public bool Process(OperationProcessorContext context) | |
| { | |
| var headerAttributes = context.MethodInfo.GetCustomAttributes<ProducesResponseHeaderAttribute>(); | |
| if (!headerAttributes.Any()) | |
| { | |
| return true; | |
| } | |
| foreach (var headerAttribute in headerAttributes) | |
| { | |
| var statusCode = headerAttribute.StatusCode.ToString(CultureInfo.InvariantCulture); | |
| if (!context.OperationDescription.Operation.Responses.TryGetValue(statusCode, out var response)) | |
| { | |
| throw new InvalidOperationException( | |
| $"Response with status code {statusCode} not found for header {headerAttribute.HeaderName}"); | |
| } | |
| var header = new OpenApiHeader | |
| { | |
| Description = headerAttribute.Description, | |
| }; | |
| response.Headers.Add(headerAttribute.HeaderName, header); | |
| } | |
| return true; | |
| } |
| success => SendCreatedAtAsync<GetDialogTransmissionEndpoint>(new GetTransmissionQuery { DialogId = dialog.Id, TransmissionId = req.Id.Value }, req.Id, cancellation: ct), | ||
| success => | ||
| { | ||
| HttpContext.Response.Headers.Append(Constants.ETag, success.Revision.ToString()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add strong ETag validator prefix.
Per RFC 7232, ETags should be prefixed with a strong validator (double quotes) or weak validator (W/).
- HttpContext.Response.Headers.Append(Constants.ETag, success.Revision.ToString());
+ HttpContext.Response.Headers.Append(Constants.ETag, $"\"{success.Revision}\"");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| HttpContext.Response.Headers.Append(Constants.ETag, success.Revision.ToString()); | |
| HttpContext.Response.Headers.Append(Constants.ETag, $"\"{success.Revision}\""); |
|
🤖 I have created a release *beep* *boop* --- ## [1.44.0](v1.43.0...v1.44.0) (2025-01-07) ### Features * **webapi:** Add ETag to response headers ([#1645](#1645)) ([7a32e60](7a32e60)) ### Bug Fixes * disable slack notifier ([#1655](#1655)) ([554fc8b](554fc8b)) --- 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)