feat(app): Change FCE MediaTypes #1729
Conversation
|
Warning Rate limit exceeded@oskogstad has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis pull request updates the media type definitions used in the system. It introduces two new constants in the Changes
Possibly related issues
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: 2
🧹 Nitpick comments (7)
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs (2)
285-285: Consider using a constant from test helpers.The media type constant is used in test data creation. Consider moving this to a test helper class to maintain consistency across test files.
- MediaType = MediaTypes.LegacyHtml, + MediaType = TestConstants.MediaTypes.LegacyHtml,
Line range hint
390-425: Comprehensive test coverage with database verification.The test effectively verifies:
- Dialog creation with different media types
- Proper conversion of deprecated media types in the database
- Correct association with dialog ID
Consider adding a test case for
MediaTypes.EmbeddableMarkdownto ensure complete coverage of supported media types.[Theory] [InlineData(MediaTypes.LegacyEmbeddableHtml)] #pragma warning disable CS0618 // Type or member is obsolete [InlineData(MediaTypes.LegacyEmbeddableHtmlDeprecated)] +[InlineData(MediaTypes.EmbeddableMarkdown)] #pragma warning restore CS0618 // Type or member is obsoletesrc/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationPolicy.cs (1)
21-21: Add XML documentation for the new scope constant.Consider adding XML documentation to explain:
- The purpose of this scope
- When it should be used
- Any migration plans (since it's for legacy content)
+ /// <summary> + /// Authorization scope for accessing legacy HTML content. + /// Part of the media type migration process - see issue #1728. + /// </summary> public const string LegacyHtmlScope = "digdir:dialogporten.serviceprovider.legacyhtml";src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs (2)
71-95: Consider extracting temporary media type additions to a configuration.The temporary media type additions (referencing issue #1749) are hardcoded. Consider extracting these to a configuration that can be easily updated when the migration is complete.
+private static readonly string[] TemporaryMediaTypes = [ + MediaTypes.EmbeddableMarkdown, + MediaTypes.EmbeddableMarkdownDeprecated +]; private static string[] GetAllowedDialogContentMediaTypes(DialogContentType contentType, IUser? user) => contentType.Id switch { DialogContentType.Values.AdditionalInfo when UserHasLegacyHtmlScope(user) => contentType.AllowedMediaTypes.Append(MediaTypes.LegacyHtml).ToArray(), DialogContentType.Values.MainContentReference when UserHasLegacyHtmlScope(user) => contentType.AllowedMediaTypes .Append(MediaTypes.LegacyEmbeddableHtmlDeprecated) .Append(MediaTypes.LegacyEmbeddableHtml) - .Append(MediaTypes.EmbeddableMarkdown) - .Append(MediaTypes.EmbeddableMarkdownDeprecated) + .Concat(TemporaryMediaTypes) .ToArray(), DialogContentType.Values.MainContentReference => contentType.AllowedMediaTypes - .Append(MediaTypes.EmbeddableMarkdown) - .Append(MediaTypes.EmbeddableMarkdownDeprecated) + .Concat(TemporaryMediaTypes) .ToArray(), _ => contentType.AllowedMediaTypes };
97-111: Maintain consistency in media type handling.The transmission content media types method follows the same pattern. Consider applying the same configuration-based approach for temporary media types.
private static string[] GetAllowedTransmissionContentMediaTypes(DialogTransmissionContentType contentType) => contentType.Id switch { DialogTransmissionContentType.Values.ContentReference => contentType.AllowedMediaTypes - .Append(MediaTypes.EmbeddableMarkdown) - .Append(MediaTypes.EmbeddableMarkdownDeprecated) + .Concat(TemporaryMediaTypes) .ToArray(), _ => contentType.AllowedMediaTypes };src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/DialogContentInputConverter.cs (1)
Line range hint
13-15: Address the TODO comment regarding team discussion.The TODO comment indicates that this implementation needs team discussion. Consider scheduling a team review to address this technical debt.
Would you like me to create an issue to track this technical debt and schedule the team discussion?
src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/TransmissionContentInputConverter.cs (1)
Line range hint
12-17: Consider combining with DialogContentInputConverter.The TODO comment suggests combining this class with
DialogContentInputConverter. This would reduce code duplication and improve maintainability.Would you like me to:
- Create an issue to track this refactoring task?
- Generate a proof-of-concept implementation that combines both converters?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs(3 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/DialogContentInputConverter.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/MappingProfile.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/MediaTypeExtensions.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/TransmissionContentInputConverter.cs(1 hunks)src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/Contents/DialogContentType.cs(1 hunks)src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/Transmissions/Contents/DialogTransmissionContentType.cs(1 hunks)src/Digdir.Domain.Dialogporten.Domain/MediaTypes.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationPolicy.cs(1 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs(6 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Transmissions/Commands/CreateTransmissionTests.cs(4 hunks)
🔇 Additional comments (15)
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs (4)
8-8: LGTM!The addition of the
Contentsnamespace import is appropriate for accessing dialog content entities.
303-305: LGTM!The assertion correctly verifies the media type validation error.
351-353: LGTM!The assertion correctly verifies the media type validation error.
Line range hint
356-388: Well-structured test parameterization.Good use of the
[Theory]attribute with[InlineData]to test both current and deprecated media types. The pragma warnings are appropriately used to suppress obsolete warnings for deprecated types.src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationPolicy.cs (1)
21-21: Verify the security implications of the new legacy HTML scope.Since this scope grants access to legacy HTML content, we should ensure:
- The scope is properly validated in all relevant endpoints
- There are appropriate security measures for handling legacy HTML content
✅ Verification successful
Legacy HTML scope implementation is properly secured ✅
The scope is correctly validated through proper authorization checks and is limited to specific content types (AdditionalInfo and MainContentReference). The implementation includes appropriate security measures and test coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of the new scope in authorization checks and content handling echo "Searching for LegacyHtmlScope usage in authorization requirements..." rg "LegacyHtmlScope" -A 5 echo "Searching for potential HTML content handling..." ast-grep --pattern 'RequireAuthorization($$$)'Length of output: 7861
src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/Contents/DialogContentType.cs (2)
68-70: LGTM! Clear migration plan with issue reference.The TODO comment provides good context about the migration strategy and includes a link to the tracking issue #1749. This aligns well with the PR's objective of implementing the first step in accommodating new media types.
71-72: Verify the scope of warning suppression.The warning suppression for the obsolete media type is properly scoped to just the specific usage. However, please ensure that:
- The new media types are ready to be used in the next phase
- The migration plan in issue Create migration for FCE MediaTypes #1749 includes steps to remove these warning suppressions
Let me verify the readiness of new media types and migration plans:
✅ Verification successful
Warning suppression is properly managed as part of the media type transition plan
The warning suppression is correctly scoped and is part of a well-documented transition plan:
- New media types are implemented and ready (PR feat(app): Change FCE MediaTypes #1729)
- Backward compatibility is maintained with automatic type conversion
- Database migration is planned as a separate phase (issue Create migration for FCE MediaTypes #1749)
- Warning suppressions will be removed after the full migration is complete
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for the definition and usage of new media types echo "Searching for new media type definitions..." rg "EmbeddableMarkdown[^D]" -A 5 echo "Checking migration issue details..." gh issue view 1749 --json title,body,labelsLength of output: 8209
Script:
#!/bin/bash # Check the blocking issue details echo "Checking blocking issue #1728..." gh issue view 1728 --json title,body,state,labels # Check for any related PRs echo -e "\nChecking related PRs..." gh pr list --search "1728 in:title,body"Length of output: 3865
src/Digdir.Domain.Dialogporten.Domain/MediaTypes.cs (2)
7-8: LGTM! Proper deprecation notices.The [Obsolete] attributes with clear migration messages help guide developers to use the new constants.
Also applies to: 11-12
8-9: Verify the impact of changing media type format from '+json' to '-url'.The change from
+jsonsuffix to-urlwith explicit type parameters represents a significant architectural change in how content is delivered.Please ensure that:
- All consumers of these media types are updated to handle the new format
- Documentation is updated to reflect this change
- API contracts and OpenAPI specifications are updated
✅ Verification successful
Media type format change is properly handled with backward compatibility
The codebase has comprehensive support for the transition from '+json' to '-url' format:
- Automatic conversion from deprecated to new format via
MediaTypeExtensions.ConvertIfDeprecatedMediaType()- Validation rules accepting both formats
- Proper deprecation notices with [Obsolete] attributes
- Extensive test coverage for both formats
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usages of the old media type format rg -l "\+json;type=(markdown|html)" --type csLength of output: 3556
Script:
#!/bin/bash # Check the actual content of DTO files and look for media type usage echo "=== Checking DTO implementations ===" rg -A 5 -B 5 "\+json;type=(markdown|html)" \ src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogTransmissions/Queries/Get/TransmissionDto.cs \ src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogDto.cs \ src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs echo -e "\n=== Checking for media type parsing or comparison logic ===" rg -A 5 "MediaType" --type csLength of output: 66105
src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/MappingProfile.cs (1)
14-17: LGTM! Consistent mapping configuration.The mapping profiles correctly handle media type conversion for both transmission and dialog content.
src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/Transmissions/Contents/DialogTransmissionContentType.cs (1)
39-43: LGTM! Clear migration plan with issue reference.The temporary use of deprecated media type is well-documented with a TODO comment and issue reference (#1749).
To track the migration completion:
✅ Verification successful
Migration for FCE MediaTypes is still pending (issue #1749 is open)
The current implementation using deprecated media type is a valid temporary solution with proper documentation and tracking. No immediate action required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the issue is still open gh issue view 1749 --json state,titleLength of output: 128
Script:
#!/bin/bash # Check if the issue is still open with explicit repository gh issue view 1749 --repo Altinn/dialogporten --json state,titleLength of output: 129
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Transmissions/Commands/CreateTransmissionTests.cs (2)
40-44: LGTM! Good test coverage for both current and deprecated media types.The conversion from
[Fact]to[Theory]with multiple test cases ensures that both the currentEmbeddableMarkdownand deprecatedEmbeddableMarkdownDeprecatedmedia types are properly tested.
85-89: LGTM! Consistent test coverage for error cases.The error case test has been properly updated to verify that HTTPS validation works consistently for both current and deprecated media types.
src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/DialogContentInputConverter.cs (1)
53-53: LGTM! Proper handling of deprecated media types.The use of
ConvertIfDeprecatedMediaType()ensures consistent handling of deprecated media types during conversion.src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/TransmissionContentInputConverter.cs (1)
56-56: LGTM! Consistent handling of deprecated media types.The use of
ConvertIfDeprecatedMediaType()aligns with the implementation inDialogContentInputConverter.
src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/MediaTypeExtensions.cs
Outdated
Show resolved
Hide resolved
...Integration.Tests/Features/V1/ServiceOwner/Transmissions/Commands/CreateTransmissionTests.cs
Outdated
Show resolved
Hide resolved
oskogstad
changed the title
oskogstad
force-pushed
the
feat/change-fce-media-types
branch
from
aeb435b to
27bc81b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20250203085824_ChangeFCEMediaTypes.cs (1)
27-51: Consider adding safety measures to the content updates.The SQL updates are well-structured but consider adding:
- Explicit transaction handling if not managed by EF Core.
- Error logging for failed updates.
- A count of affected rows for verification.
Here's a suggested enhancement:
migrationBuilder.Sql(""" -- Update MediaType in DialogContent table + DO $$ + DECLARE + affected_rows INTEGER; + BEGIN UPDATE public."DialogContent" SET "MediaType" = CASE WHEN "MediaType" = 'application/vnd.dialogporten.frontchannelembed+json;type=html' THEN 'application/vnd.dialogporten.frontchannelembed-url;type=text/html' WHEN "MediaType" = 'application/vnd.dialogporten.frontchannelembed+json;type=markdown' THEN 'application/vnd.dialogporten.frontchannelembed-url;type=text/markdown' ELSE "MediaType" END WHERE "MediaType" IN ('application/vnd.dialogporten.frontchannelembed+json;type=html', 'application/vnd.dialogporten.frontchannelembed+json;type=markdown'); + GET DIAGNOSTICS affected_rows = ROW_COUNT; + RAISE NOTICE 'Updated % rows in DialogContent', affected_rows; -- Update MediaType in DialogTransmissionContent table UPDATE public."DialogTransmissionContent" SET "MediaType" = CASE WHEN "MediaType" = 'application/vnd.dialogporten.frontchannelembed+json;type=html' THEN 'application/vnd.dialogporten.frontchannelembed-url;type=text/html' WHEN "MediaType" = 'application/vnd.dialogporten.frontchannelembed+json;type=markdown' THEN 'application/vnd.dialogporten.frontchannelembed-url;type=text/markdown' ELSE "MediaType" END WHERE "MediaType" IN ('application/vnd.dialogporten.frontchannelembed+json;type=html', 'application/vnd.dialogporten.frontchannelembed+json;type=markdown'); + GET DIAGNOSTICS affected_rows = ROW_COUNT; + RAISE NOTICE 'Updated % rows in DialogTransmissionContent', affected_rows; + END $$; """);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20250203085824_ChangeFCEMediaTypes.Designer.csis excluded by!**/Migrations/**/*Designer.cssrc/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/DialogDbContextModelSnapshot.csis excluded by!**/Migrations/DialogDbContextModelSnapshot.cs
📒 Files selected for processing (2)
src/Digdir.Domain.Dialogporten.Domain/MediaTypes.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20250203085824_ChangeFCEMediaTypes.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Digdir.Domain.Dialogporten.Domain/MediaTypes.cs
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Dry run deploy apps / Deploy service to test
- GitHub Check: Dry run deploy apps / Deploy graphql to test
- GitHub Check: Dry run deploy apps / Deploy web-api-so to test
- GitHub Check: Dry run deploy apps / Deploy web-api-eu to test
- GitHub Check: build / build-and-test
🔇 Additional comments (2)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20250203085824_ChangeFCEMediaTypes.cs (2)
1-9: LGTM! Migration setup follows best practices.The migration class is properly structured with appropriate naming and attributes.
13-26: Verify ID assumptions and document breaking changes.The migration assumes specific IDs exist in the tables. While this is likely correct based on the PR context, consider:
- Adding a safety check for the existence of these IDs.
- Documenting this as a breaking change in the API documentation since media types are changing.
Let's verify the ID assumptions:
✅ Verification successful
ID Existence & Breaking Change Documentation
- The investigation shows that IDs 6 (for DialogContentType) and 3 (for DialogTransmissionContentType) are used repeatedly in our migration seed data. This confirms that the migration’s assumptions are in line with the established data.
- Although the IDs appear to be stable across migrations, consider adding a safeguard (or explicit check) in the migration or at the DB level to ensure these records exist at runtime.
- Finally, document the breaking change (specifically the media type update) clearly in API release notes and any related documentation to alert consumers of the change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the IDs exist in the tables and check for any references # Search for ID definitions and references rg -A 5 "Id = 6|Id = 3" --type cs # Search for any existing documentation about these IDs fd -e md -e txt . | xargs rg "DialogContentType.*6|DialogTransmissionContentType.*3"Length of output: 79491
...ain.Dialogporten.Infrastructure/Persistence/Migrations/20250203085824_ChangeFCEMediaTypes.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs (2)
24-26: Consider caching the allowed media types list.While the backwards compatibility approach is correct, using
Append()creates a new sequence on each validation. Consider caching the combined list as a field to improve performance.+ private readonly string[] _allowedMediaTypes; + + public ContentValueDtoValidator(DialogTransmissionContentType contentType) + { + _allowedMediaTypes = contentType.AllowedMediaTypes + .Append(MediaTypes.EmbeddableMarkdownDeprecated) + .ToArray(); + RuleFor(x => x.MediaType) .NotEmpty() - .Must(value => value is not null && contentType.AllowedMediaTypes - // Manually adding this for backwards compatibility, until correspondence is updated and deployed - .Append(MediaTypes.EmbeddableMarkdownDeprecated).Contains(value)) + .Must(value => value is not null && _allowedMediaTypes.Contains(value))
78-84: Consider caching media type lists and documenting removal timeline.The implementation correctly handles backwards compatibility but has some areas for improvement:
- Multiple
Append()calls create new sequences on each validation- The temporary nature of these changes should be documented with a timeline or tracking issue
+ // Cache commonly used media type combinations + private static readonly string[] _legacyMainContentReferenceTypes = DialogContentType + .Values.MainContentReference.AllowedMediaTypes + .Append(MediaTypes.LegacyEmbeddableHtml) + .Append(MediaTypes.EmbeddableMarkdownDeprecated) + .Append(MediaTypes.LegacyEmbeddableHtmlDeprecated) + .ToArray(); + + private static readonly string[] _mainContentReferenceTypes = DialogContentType + .Values.MainContentReference.AllowedMediaTypes + .Append(MediaTypes.EmbeddableMarkdownDeprecated) + .ToArray(); [SuppressMessage("Style", "IDE0072:Add missing cases")] private static string[] GetAllowedMediaTypes(DialogContentType contentType, IUser? user) => contentType.Id switch { DialogContentType.Values.AdditionalInfo when UserHasLegacyHtmlScope(user) => contentType.AllowedMediaTypes.Append(MediaTypes.LegacyHtml).ToArray(), DialogContentType.Values.MainContentReference when UserHasLegacyHtmlScope(user) - => contentType.AllowedMediaTypes.Append(MediaTypes.LegacyEmbeddableHtml) - // Manually adding this for backwards compatibility, until correspondence is updated and deployed - .Append(MediaTypes.EmbeddableMarkdownDeprecated) - .Append(MediaTypes.LegacyEmbeddableHtmlDeprecated).ToArray(), + => _legacyMainContentReferenceTypes, DialogContentType.Values.MainContentReference - => contentType.AllowedMediaTypes.Append(MediaTypes.EmbeddableMarkdownDeprecated).ToArray(), + => _mainContentReferenceTypes, _ => contentType.AllowedMediaTypes };Also, consider adding a TODO comment with more details:
// TODO: Remove deprecated media types after correspondence service is updated // Tracking issue: #1728 // Expected removal: Q2 2025
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs(2 hunks)src/Digdir.Domain.Dialogporten.Domain/MediaTypes.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Digdir.Domain.Dialogporten.Domain/MediaTypes.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dry run deploy apps / Deploy graphql to test
- GitHub Check: build / build-and-test
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/DialogContentInputConverter.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/TransmissionContentInputConverter.cs(1 hunks)src/Digdir.Domain.Dialogporten.Domain/MediaTypes.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs
- src/Digdir.Domain.Dialogporten.Domain/MediaTypes.cs
🔇 Additional comments (5)
src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/DialogContentInputConverter.cs (3)
13-13: Address the TODO comment during this media type refactoring.Since this PR involves significant changes to media types and content handling, it would be a good time to discuss and resolve this TODO with the team. This could help ensure the new media type implementation aligns with the team's long-term vision.
Would you like me to create an issue to track this discussion item?
24-68: LGTM! The converter implementation is robust and performant.The implementation demonstrates good practices:
- Proper null handling
- Efficient property caching
- Clean separation of concerns
53-53: Verify the MapDeprecatedMediaType extension method.The change correctly handles the migration of deprecated media types, which aligns with the PR's objective of implementing a two-step transition process.
Let's verify the extension method implementation:
✅ Verification successful
The MapDeprecatedMediaType extension method is verified.
- The extension method is defined in
src/Digdir.Domain.Dialogporten.Domain/MediaTypes.cs(line 24) with the expected signature.- Its usage in both
DialogContentInputConverter.csandTransmissionContentInputConverter.csaligns with the intended functionality for handling deprecated media types.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the MapDeprecatedMediaType extension method exists and check its implementation # Search for the extension method definition ast-grep --pattern 'public static string MapDeprecatedMediaType(this string mediaType)' # Search for any tests covering this method rg -l "MapDeprecatedMediaType.*Test"Length of output: 128
Script:
#!/bin/bash # Description: Search the entire repository for any occurrence of "MapDeprecatedMediaType" # to verify if the extension method is defined somewhere or used elsewhere in the code. rg -n "MapDeprecatedMediaType"Length of output: 520
src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/TransmissionContentInputConverter.cs (2)
31-70: Implementation looks solid.The media type mapping change is well-integrated into the existing conversion logic. The code maintains proper null handling, immutability, and uses efficient property caching.
13-17: Consider addressing the TODO comment as part of this PR.Since both this converter and
DialogContentInputConverternow handle media type mapping similarly, this might be an opportune time to combine them as suggested in the comment. This would help maintain consistency in media type handling across the codebase.Let's check if
DialogContentInputConverterhas similar structure:✅ Verification successful
Please consider addressing the existing TODO comment in both converters.
Both the
TransmissionContentInputConverterand theDialogContentInputConvertercontain nearly identical mapping logic—including handling the mapping for deprecated media types—and both feature a TODO suggesting a potential merge. This consistency indicates that there is an opportunity to consolidate the shared logic, which would help maintain consistency and reduce duplicate code in the project.
- Verify if combining these converters is acceptable within the current architecture.
- If so, refactor and merge the common logic for media type mapping to streamline future maintenance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find and examine DialogContentInputConverter implementation rg -l "DialogContentInputConverter" | xargs catLength of output: 15658
...ain.Dialogporten.Application/Features/V1/Common/Content/TransmissionContentInputConverter.cs
Show resolved
Hide resolved
🤖 I have created a release *beep* *boop* --- ## [1.48.0](v1.47.8...v1.48.0) (2025-02-03) ### Features * **app:** Change FCE MediaTypes ([#1729](#1729)) ([ef4e0a4](ef4e0a4)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Changing FCE media types (breaks the API)
This PR is step 1 of 2(?), the app needs to allow for the new media types before we yank out the old ones in the DB lookup type.Tests have been modified to [Theory] and check both types, and checks for conversion from old to new when storing in DB.
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)