chore: Change policies set to policyMap
#34566
Conversation
WalkthroughThe recent updates represent a significant overhaul in how policies are managed within the codebase. Transitioning from a Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
sharat87
changed the title
policies set to policyMap
github-actions
bot
added
the
skip-changelog
Failed server tests
|
sharat87
added
ok-to-test
Failed server tests
|
sharat87
added
ok-to-test
Failed server tests
|
sharat87
added
ok-to-test
sharat87
added
ok-to-test
![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
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (10)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration025RemoveUnassignPermissionFromUnnecessaryRoles.java (4 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration028TagUserManagementRolesWithoutDefaultDomainTypeAndId.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration029PopulateDefaultDomainIdInUserManagementRoles.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration031CreateUserManagementRolesForUsersTaggedIn030.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration047AddMissingFieldsInDefaultAppsmithAiDatasource.java (4 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration057PolicySetToPolicyMap.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PolicySolutionCEImpl.java (4 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java (3 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/NewPageServiceTest.java (1 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/PolicySolutionCEImplTest.java (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration025RemoveUnassignPermissionFromUnnecessaryRoles.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration028TagUserManagementRolesWithoutDefaultDomainTypeAndId.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration029PopulateDefaultDomainIdInUserManagementRoles.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration047AddMissingFieldsInDefaultAppsmithAiDatasource.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration057PolicySetToPolicyMap.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PolicySolutionCEImpl.java
Additional comments not posted (11)
app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/PolicySolutionCEImplTest.java (4)
35-44: Good job on the test coverage!The test method
testAddNewPoliciesToEmptyObjectis well-written and covers the scenario of adding new policies to an empty object.
46-65: Great work on testing policy merging!The test method
testMergePoliciesWithExistingOnesis well-written and covers the scenario of merging new policies with existing ones.
67-85: Excellent test for immutability!The test method
testOriginalPolicyMapNotModifiedis well-written and ensures that the original policy map is not modified.
87-96: Well done on ensuring object consistency!The test method
testReturnModifiedObjectis well-written and ensures that the modified object is returned.app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration031CreateUserManagementRolesForUsersTaggedIn030.java (3)
27-27: Good use of constant for deprecated field name.Using the constant
DeprecatedFieldName.POLICIESimproves code maintainability by centralizing the reference to the policies field.
54-54: Ensure backward compatibility with the new field.Including both
POLICIESandpolicyMapin the query ensures backward compatibility. This is a good practice during migrations.
71-71: Good practice to use constant for deprecated field name.Using the constant
DeprecatedFieldName.POLICIESin the update operation ensures consistency and maintainability.app/server/appsmith-server/src/test/java/com/appsmith/server/services/NewPageServiceTest.java (1)
290-294: Nice use of streams for immutability!The use of streams and the
peekmethod to set the permission enhances immutability and ensures that the policies are not modified after being set.app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java (3)
2160-2161: Good job enhancing the assertion logic!The new assertion logic using
containsExactlyInAnyOrderElementsOfensures that the order of elements is not considered when comparing the policies, making the test more robust.
4947-4952: Nice use of stream operations for filtering policies!The new filtering logic using streams and
collect(Collectors.toUnmodifiableSet())improves code clarity and ensures immutability of the resulting set.
4984-4989: Great job updating the filtering logic!The use of streams and
collect(Collectors.toUnmodifiableSet())enhances the readability and immutability of the code.
...psmith/server/migrations/db/ce/Migration031CreateUserManagementRolesForUsersTaggedIn030.java
Outdated
Show resolved
Hide resolved
...smith/server/migrations/db/ce/Migration047AddMissingFieldsInDefaultAppsmithAiDatasource.java
Outdated
Show resolved
Hide resolved
...ver/src/main/java/com/appsmith/server/migrations/db/ce/Migration057PolicySetToPolicyMap.java
Outdated
Show resolved
Hide resolved
...ver/src/main/java/com/appsmith/server/migrations/db/ce/Migration057PolicySetToPolicyMap.java
Outdated
Show resolved
Hide resolved
| // Migrate the policies field to the policyMap field only if the policies field exists and is not empty | ||
| new Query(where(dotted(POLICIES, "0")) | ||
| .exists(true) | ||
| .and(BaseDomain.Fields.deletedAt) |
There was a problem hiding this comment.
Does this mean that we will need to update the notDeleted() method, because it will end up giving this error at other places as well?
...h-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java
Show resolved
Hide resolved
...ver/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PolicySolutionCEImpl.java
Outdated
Show resolved
Hide resolved
abhvsn
added
ok-to-test
abhvsn
added
ok-to-test
abhvsn
requested review from
nsarupr
and removed request for
nidhi-nair,
sondermanish and
a team
abhvsn
added
ok-to-test
Migrate
policiesfrom being aSet<Policy>to apolicyMap, that's aMap<String, Policy>. The key defines the permission whereas the value is the actual policy object. This makes it easier to use this data in code, to query as a MongoDB nested document, and as a Postgresjsonbcolumn. Win-win!The migration runs for all collections in parallel, with following stats taken on 1-Jul-2024.
This PR is only part of the solution. We have backwards compatible
getPolicies()andsetPolicies()methods so the diff isn't too big, but a follow-up PR will remove those two, and migration all usages to use the map instead.This PR only (almost) strictly includes changes that were necessary for the build and tests to pass.
/test all
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10301601562
Commit: d58ac59
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 08 Aug 2024 12:46:36 UTC
Summary by CodeRabbit
Summary by CodeRabbit
New Features
policiestopolicyMap, enhancing data structure for policy management.Documentation
Refactor
policyMapstructure.Tests