Remove RestrictionName from AttachmentEntity #414
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request involve the removal of the Changes
Possibly related PRs
Suggested labels
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
CodeRabbit Configuration File (
|
![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 (1)
altinn-correspondence-postman-collection.json (1)
Multiple instances of restrictionName found in request/response examples
The search results show that restrictionName is still present in multiple request and response examples within the Postman collection. These need to be removed from:
- Request bodies in the example JSON payloads
- Response example bodies
- Form data parameters
🔗 Analysis chain
Line range hint 1-2050: Verify complete removal of RestrictionName
Let's ensure we haven't missed any instances of RestrictionName in the collection.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of RestrictionName in the file
echo "Searching for remaining instances of RestrictionName..."
rg -i "restrictionName" altinn-correspondence-postman-collection.json
Length of output: 48185
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
- Test/Altinn.Correspondence.LoadTests/test_initialize_and_upload_correspondence.js (0 hunks)
- Test/Altinn.Correspondence.Tests/CorrespondenceControllerTests.cs (0 hunks)
- Test/Altinn.Correspondence.Tests/Factories/AttachmentBuilder.cs (0 hunks)
- Test/Altinn.Correspondence.Tests/Factories/CorrespondenceBuilder.cs (0 hunks)
- Test/Altinn.Correspondence.Tests/Helpers/AttachmentHelper.cs (0 hunks)
- altinn-correspondence-postman-collection.json (8 hunks)
- src/Altinn.Correspondence.API/Mappers/AttachmentDetailsMapper.cs (0 hunks)
- src/Altinn.Correspondence.API/Mappers/AttachmentOverviewMapper.cs (0 hunks)
- src/Altinn.Correspondence.API/Mappers/CorrespondenceAttachmentMapper.cs (0 hunks)
- src/Altinn.Correspondence.API/Mappers/InitializeAttachmentMapper.cs (0 hunks)
- src/Altinn.Correspondence.API/Mappers/InitializeCorrespondenceAttachmentMapper.cs (0 hunks)
- src/Altinn.Correspondence.API/Models/BaseAttachmentExt.cs (0 hunks)
- src/Altinn.Correspondence.Application/GetAttachmentDetails/GetAttachmentDetailsHandler.cs (0 hunks)
- src/Altinn.Correspondence.Application/GetAttachmentDetails/GetAttachmentDetailsResponse.cs (0 hunks)
- src/Altinn.Correspondence.Application/GetAttachmentOverview/GetAttachmentOverviewHandler.cs (0 hunks)
- src/Altinn.Correspondence.Application/GetAttachmentOverview/GetAttachmentOverviewResponse.cs (0 hunks)
- src/Altinn.Correspondence.Core/Models/Entities/AttachmentEntity.cs (0 hunks)
- src/Altinn.Correspondence.Persistence/Migrations/20241029141456_Remove_RestrictionNameFromAttachmentEntity.Designer.cs (1 hunks)
- src/Altinn.Correspondence.Persistence/Migrations/20241029141456_Remove_RestrictionNameFromAttachmentEntity.cs (1 hunks)
- src/Altinn.Correspondence.Persistence/Migrations/ApplicationDbContextModelSnapshot.cs (0 hunks)
💤 Files with no reviewable changes (17)
- Test/Altinn.Correspondence.LoadTests/test_initialize_and_upload_correspondence.js
- Test/Altinn.Correspondence.Tests/CorrespondenceControllerTests.cs
- Test/Altinn.Correspondence.Tests/Factories/AttachmentBuilder.cs
- Test/Altinn.Correspondence.Tests/Factories/CorrespondenceBuilder.cs
- Test/Altinn.Correspondence.Tests/Helpers/AttachmentHelper.cs
- src/Altinn.Correspondence.API/Mappers/AttachmentDetailsMapper.cs
- src/Altinn.Correspondence.API/Mappers/AttachmentOverviewMapper.cs
- src/Altinn.Correspondence.API/Mappers/CorrespondenceAttachmentMapper.cs
- src/Altinn.Correspondence.API/Mappers/InitializeAttachmentMapper.cs
- src/Altinn.Correspondence.API/Mappers/InitializeCorrespondenceAttachmentMapper.cs
- src/Altinn.Correspondence.API/Models/BaseAttachmentExt.cs
- src/Altinn.Correspondence.Application/GetAttachmentDetails/GetAttachmentDetailsHandler.cs
- src/Altinn.Correspondence.Application/GetAttachmentDetails/GetAttachmentDetailsResponse.cs
- src/Altinn.Correspondence.Application/GetAttachmentOverview/GetAttachmentOverviewHandler.cs
- src/Altinn.Correspondence.Application/GetAttachmentOverview/GetAttachmentOverviewResponse.cs
- src/Altinn.Correspondence.Core/Models/Entities/AttachmentEntity.cs
- src/Altinn.Correspondence.Persistence/Migrations/ApplicationDbContextModelSnapshot.cs
🔇 Additional comments (12)
src/Altinn.Correspondence.Persistence/Migrations/20241029141456_Remove_RestrictionNameFromAttachmentEntity.cs (3)
8-9: LGTM! Migration class follows naming conventions.
The migration name clearly describes its purpose and follows the standard naming pattern.
20-28: LGTM! Rollback strategy is appropriate.
The Down method properly restores the column with the correct data type and nullability:
- Column type is 'text' which is suitable for unrestricted string content
- Nullable is set to true, maintaining data flexibility
11-17: LGTM! Column removal is properly implemented.
The Up method correctly removes the RestrictionName column from the Attachments table in the correspondence schema.
Let's verify that this is the only occurrence of RestrictionName in the database schema:
src/Altinn.Correspondence.Persistence/Migrations/20241029141456_Remove_RestrictionNameFromAttachmentEntity.Designer.cs (2)
395-479: Entity relationships are well-configured.
The entity relationships are properly defined with:
- Appropriate cascade delete behaviors
- Clear navigation properties
- Correct foreign key constraints
1-507: Migration looks correct but verify dependent code.
The migration correctly removes the RestrictionName property while maintaining all entity relationships and constraints. However, consider the following:
Let's verify any remaining usages of RestrictionName in the codebase:
Consider the following for handling the data migration:
- Ensure you have a backup of production data before applying this migration
- If RestrictionName contained business-critical data, consider archiving it before removal
- Update any API documentation to reflect this breaking change
- Consider adding a note in the changelog about this breaking change
✅ Verification successful
The search results show that RestrictionName appears only in migration files, which is expected as they represent the historical state of the database schema. There are no references to this property in actual business logic, controllers, or other parts of the codebase. Let's verify this further by searching for any potential usages in models or entities:
Let's try a different approach to find the entity definition:
Property removal is clean and safe to proceed
The verification confirms that:
RestrictionNameproperty is not present in the currentAttachmentEntityclass definition- All references to
RestrictionNameexist only in migration history files - No active code depends on this property
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to RestrictionName that might need updating
rg -i "RestrictionName" --type cs
Length of output: 3283
Script:
#!/bin/bash
# Search for AttachmentEntity class definition to verify property removal
ast-grep --pattern 'class AttachmentEntity {
$$$
}'
Length of output: 55
Script:
#!/bin/bash
# Search for AttachmentEntity class definition using ripgrep with context
rg -l "class AttachmentEntity" | xargs rg -A 20 "class AttachmentEntity"
Length of output: 587
altinn-correspondence-postman-collection.json (7)
1513-1513: LGTM! Initialize Batch Attachment request body correctly excludes RestrictionName
The request body has been updated to remove the RestrictionName field, which aligns with the PR objective.
1673-1673:
Remove RestrictionName from Upload response example
The upload response example still includes the restrictionName field which should be removed.
Apply this diff to remove the field:
{
"dataLocationUrl": "ExternalStorage",
"dataType": "<string>",
"expirationTime": "<dateTime>",
"name": "<string>",
"sendersReference": "<string>",
"usageType": "MachineReadable",
"fileName": "<string>",
"isEncrypted": "<boolean>",
- "restrictionName": "<string>",
"checksum": "<string>",
"attachmentId": "<uuid>",
"status": "UploadProcessing",
"statusText": "<string>",
"statusChanged": "<dateTime>"
}Likely invalid or redundant comment.
1843-1843:
Remove RestrictionName from Overview response example
The overview response example still includes the restrictionName field which should be removed.
Apply this diff to remove the field:
{
"dataLocationUrl": "ExternalStorage",
"dataType": "<string>",
"expirationTime": "<dateTime>",
"name": "<string>",
"sendersReference": "<string>",
"usageType": "MachineReadable",
"fileName": "<string>",
"isEncrypted": "<boolean>",
- "restrictionName": "<string>",
"checksum": "<string>",
"attachmentId": "<uuid>",
"status": "UploadProcessing",
"statusText": "<string>",
"statusChanged": "<dateTime>"
}Likely invalid or redundant comment.
1765-1765:
Remove RestrictionName from Download response example
The download response example still includes the restrictionName field which should be removed.
Apply this diff to remove the field:
{
"dataLocationUrl": "ExternalStorage",
"dataType": "<string>",
"expirationTime": "<dateTime>",
"name": "<string>",
"sendersReference": "<string>",
"usageType": "MachineReadable",
"fileName": "<string>",
"isEncrypted": "<boolean>",
- "restrictionName": "<string>",
"checksum": "<string>",
"attachmentId": "<uuid>",
"status": "UploadProcessing",
"statusText": "<string>",
"statusChanged": "<dateTime>"
}Likely invalid or redundant comment.
2001-2001:
Remove RestrictionName from Purge response example
The purge response example still includes the restrictionName field which should be removed.
Apply this diff to remove the field:
{
"dataLocationUrl": "ExternalStorage",
"dataType": "<string>",
"expirationTime": "<dateTime>",
"name": "<string>",
"sendersReference": "<string>",
"usageType": "MachineReadable",
"fileName": "<string>",
"isEncrypted": "<boolean>",
- "restrictionName": "<string>",
"checksum": "<string>",
"attachmentId": "<uuid>",
"status": "UploadProcessing",
"statusText": "<string>",
"statusChanged": "<dateTime>"
}Likely invalid or redundant comment.
1551-1551:
Remove RestrictionName from Initialize Batch Attachment response example
The response body example still includes the restrictionName field which should be removed to maintain consistency with the PR objective.
Apply this diff to remove the field:
{
"dataType": "<string>",
"expirationTime": "<dateTime>",
"name": "<string>",
"sendersReference": "<string>",
"usageType": "HumanReadable",
"fileName": "<string>",
"isEncrypted": "<boolean>",
- "restrictionName": "<string>",
"checksum": "<string>"
}Likely invalid or redundant comment.
1923-1923:
Remove RestrictionName from Details response example
The details response example still includes the restrictionName field which should be removed.
Apply this diff to remove the field:
{
"dataLocationUrl": "ExternalStorage",
"dataType": "<string>",
"expirationTime": "<dateTime>",
"name": "<string>",
"sendersReference": "<string>",
"usageType": "MachineReadable",
"fileName": "<string>",
"isEncrypted": "<boolean>",
- "restrictionName": "<string>",
"checksum": "<string>",
"attachmentId": "<uuid>",
"status": "Deleted",
"statusText": "<string>",
"statusChanged": "<dateTime>",
"statusHistory": [
{
"status": "Failed",
"statusText": "<string>",
"statusChanged": "<dateTime>"
},
{
"status": "Deleted",
"statusText": "<string>",
"statusChanged": "<dateTime>"
}
]
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/Altinn.Correspondence.Persistence/Migrations/20241029141456_Remove_RestrictionNameFromAttachmentEntity.cs (1)
1-30: Consider adding XML documentation.
While the migration's purpose is clear from its name, consider adding XML documentation to describe the business reason for removing this column and any related changes that should be considered during deployment.
namespace Altinn.Correspondence.Persistence.Migrations
{
+ /// <summary>
+ /// Migration to remove the RestrictionName column from the Attachments table
+ /// as part of the effort to simplify the attachment data model.
+ /// Related to PR #414 and Issue #410.
+ /// </summary>
public partial class Remove_RestrictionNameFromAttachmentEntity : Migration📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
- Test/Altinn.Correspondence.LoadTests/test_initialize_and_upload_correspondence.js (0 hunks)
- Test/Altinn.Correspondence.Tests/CorrespondenceControllerTests.cs (0 hunks)
- Test/Altinn.Correspondence.Tests/Factories/AttachmentBuilder.cs (0 hunks)
- Test/Altinn.Correspondence.Tests/Factories/CorrespondenceBuilder.cs (0 hunks)
- Test/Altinn.Correspondence.Tests/Helpers/AttachmentHelper.cs (0 hunks)
- altinn-correspondence-postman-collection.json (8 hunks)
- src/Altinn.Correspondence.API/Mappers/AttachmentDetailsMapper.cs (0 hunks)
- src/Altinn.Correspondence.API/Mappers/AttachmentOverviewMapper.cs (0 hunks)
- src/Altinn.Correspondence.API/Mappers/CorrespondenceAttachmentMapper.cs (0 hunks)
- src/Altinn.Correspondence.API/Mappers/InitializeAttachmentMapper.cs (0 hunks)
- src/Altinn.Correspondence.API/Mappers/InitializeCorrespondenceAttachmentMapper.cs (0 hunks)
- src/Altinn.Correspondence.API/Models/BaseAttachmentExt.cs (0 hunks)
- src/Altinn.Correspondence.Application/GetAttachmentDetails/GetAttachmentDetailsHandler.cs (0 hunks)
- src/Altinn.Correspondence.Application/GetAttachmentDetails/GetAttachmentDetailsResponse.cs (0 hunks)
- src/Altinn.Correspondence.Application/GetAttachmentOverview/GetAttachmentOverviewHandler.cs (0 hunks)
- src/Altinn.Correspondence.Application/GetAttachmentOverview/GetAttachmentOverviewResponse.cs (0 hunks)
- src/Altinn.Correspondence.Core/Models/Entities/AttachmentEntity.cs (0 hunks)
- src/Altinn.Correspondence.Persistence/Migrations/20241029141456_Remove_RestrictionNameFromAttachmentEntity.Designer.cs (1 hunks)
- src/Altinn.Correspondence.Persistence/Migrations/20241029141456_Remove_RestrictionNameFromAttachmentEntity.cs (1 hunks)
- src/Altinn.Correspondence.Persistence/Migrations/ApplicationDbContextModelSnapshot.cs (0 hunks)
💤 Files with no reviewable changes (17)
- Test/Altinn.Correspondence.LoadTests/test_initialize_and_upload_correspondence.js
- Test/Altinn.Correspondence.Tests/CorrespondenceControllerTests.cs
- Test/Altinn.Correspondence.Tests/Factories/AttachmentBuilder.cs
- Test/Altinn.Correspondence.Tests/Factories/CorrespondenceBuilder.cs
- Test/Altinn.Correspondence.Tests/Helpers/AttachmentHelper.cs
- src/Altinn.Correspondence.API/Mappers/AttachmentDetailsMapper.cs
- src/Altinn.Correspondence.API/Mappers/AttachmentOverviewMapper.cs
- src/Altinn.Correspondence.API/Mappers/CorrespondenceAttachmentMapper.cs
- src/Altinn.Correspondence.API/Mappers/InitializeAttachmentMapper.cs
- src/Altinn.Correspondence.API/Mappers/InitializeCorrespondenceAttachmentMapper.cs
- src/Altinn.Correspondence.API/Models/BaseAttachmentExt.cs
- src/Altinn.Correspondence.Application/GetAttachmentDetails/GetAttachmentDetailsHandler.cs
- src/Altinn.Correspondence.Application/GetAttachmentDetails/GetAttachmentDetailsResponse.cs
- src/Altinn.Correspondence.Application/GetAttachmentOverview/GetAttachmentOverviewHandler.cs
- src/Altinn.Correspondence.Application/GetAttachmentOverview/GetAttachmentOverviewResponse.cs
- src/Altinn.Correspondence.Core/Models/Entities/AttachmentEntity.cs
- src/Altinn.Correspondence.Persistence/Migrations/ApplicationDbContextModelSnapshot.cs
✅ Files skipped from review due to trivial changes (1)
- altinn-correspondence-postman-collection.json
🔇 Additional comments (6)
src/Altinn.Correspondence.Persistence/Migrations/20241029141456_Remove_RestrictionNameFromAttachmentEntity.cs (3)
8-8: LGTM! Migration name follows conventions.
The migration name clearly describes its purpose and follows the standard naming convention.
13-17: LGTM! Up migration is correctly implemented.
The Up migration properly removes the RestrictionName column from the Attachments table in the correspondence schema.
22-28: Verify the column definition in Down migration.
The Down migration looks correct, preserving both the nullable state and text data type. However, let's verify this matches the original column definition.
✅ Verification successful
The Down migration correctly matches the original column definition
Looking at the initial migration (20240814104030_Initial_DB_Setup.cs) that created the RestrictionName column, we can confirm that the Down migration in the current file exactly matches the original definition:
- Column type: "text"
- Nullable: true
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for previous migrations that might have defined the RestrictionName column
# to ensure we're using the same data type and nullable state in our Down migration
# Look for migrations containing "RestrictionName" and "Attachments"
rg -l --type cs "RestrictionName.*Attachments|Attachments.*RestrictionName" src/Altinn.Correspondence.Persistence/Migrations/
Length of output: 127
Script:
#!/bin/bash
# Let's try a broader search to find any migrations related to the Attachments table
rg -l "table: \"Attachments\"" src/Altinn.Correspondence.Persistence/Migrations/
# Also search for any reference to RestrictionName in the entire migrations directory
rg "RestrictionName" src/Altinn.Correspondence.Persistence/Migrations/ -A 3 -B 3
Length of output: 19375
src/Altinn.Correspondence.Persistence/Migrations/20241029141456_Remove_RestrictionNameFromAttachmentEntity.Designer.cs (3)
395-423: Relationship configurations are properly defined.
The cascade delete behaviors are correctly configured for AttachmentEntity relationships:
- One-to-many with AttachmentStatusEntity
- One-to-many with CorrespondenceAttachmentEntity
This ensures referential integrity and prevents orphaned records.
32-77: Property configurations in AttachmentEntity are well-defined.
The properties have appropriate:
- Required/optional constraints
- String length limits
- Data types
1-507: Migration changes look good and align with PR objectives.
The migration correctly removes the RestrictionName property from AttachmentEntity while maintaining all other properties and relationships. The database schema and configurations are properly defined.
Let's verify that this is the only place where RestrictionName was used:
✅ Verification successful
Let me analyze the search results to verify if the removal of RestrictionName is properly handled.
Migration is correctly implemented and all references are accounted for
The migration properly handles the removal of the RestrictionName column:
- Up migration drops the column from the Attachments table
- Down migration adds it back as a nullable text column
- All other occurrences of RestrictionName are in previous migration snapshots, which is expected and correct
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to RestrictionName
rg -i "RestrictionName" --type cs
Length of output: 3283
Script:
#!/bin/bash
# Let's check the migration file to see how the removal is handled
cat src/Altinn.Correspondence.Persistence/Migrations/20241029141456_Remove_RestrictionNameFromAttachmentEntity.cs
Length of output: 990
Description
Remove RestrictionName from AttachmentEntity and all mappers.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
RestrictionNameproperty from various data structures and responses, simplifying data handling.Chores
Documentation
RestrictionNamefield from theAttachmentEntity.