chore: Migrate Tenant to Organization #38891
Conversation
trishaanand
requested review from
sharat87,
abhvsn,
AnaghHegde,
a team and
NilanshBansal
as code owners
WalkthroughThis pull request implements a comprehensive refactor that transitions the codebase from a tenant‐based model to an organization‐based model. The changes affect Java and TypeScript files on both server and client sides. Class names, DTO field names, constants, repository and service interfaces, API endpoints, Redux actions/selectors, sagas, and tests are updated accordingly. Obsolete “tenant” files are removed, and migration scripts have been added to facilitate data and configuration updates. The underlying business logic remains unchanged apart from renaming and dependency injection updates. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant API as API Gateway
participant OS as OrganizationService
participant DB as Database
C->>API: HTTP Request for Organization Config
API->>OS: Retrieve organization configuration
OS->>DB: Query organization data
DB-->>OS: Return organization config
OS-->>API: Send configuration response
API-->>C: Return organization config response
Suggested reviewers
Poem
✨ Finishing Touches
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
|
github-actions
bot
added
the
skip-changelog
Failed server tests
|
1 similar comment
Failed server tests
|
Failed server tests
|
| @@ -121,7 +121,7 @@ public enum AclPermission { | |||
| READ_PERMISSION_GROUPS("read:permissionGroups", PermissionGroup.class), | |||
|
|
|||
| // Manage tenant permissions | |||
| MANAGE_TENANT("manage:tenants", Tenant.class), | |||
| MANAGE_TENANT("manage:tenants", Organization.class), | |||
There was a problem hiding this comment.
@trishaanand curious are we going to create another PR to move the existing permission nomenclature with the corresponding migration changes required?
There was a problem hiding this comment.
Oh it would be part of this PR itself. Its just a WIP.
Failed server tests
|
Failed server tests
|
Failed server tests
|
Failed server tests
|
Failed server tests
|
1 similar comment
Failed server tests
|
TODO : Fix client side code to use organization instead of tenant in CE.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.java (1)
18-58: Update comments to use "organization" instead of "tenant".Comments still reference "tenant" terminology which should be updated to maintain consistency with the refactoring.
For example:
- Line 29: "tenant admin" should be "organization admin"
- Line 35: "tenant configuration" should be "organization configuration"
- Line 48: "tenant level feature flags" should be "organization level feature flags"
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/MigrationHelperMethods.java (2)
773-783: Enhance error handling in mapGoogleSheetsToNewFormData.The method throws an AppsmithException without proper context when formData is not null.
Consider providing more context in the error:
- throw new AppsmithException(AppsmithError.MIGRATION_ERROR); + throw new AppsmithException( + AppsmithError.MIGRATION_ERROR, + "Action has already been migrated: " + action.getName());
54-54: Consider splitting the class into smaller, focused classes.The
MigrationHelperMethodsclass has grown too large and handles multiple types of migrations. This violates the Single Responsibility Principle.Consider splitting into specialized classes:
GoogleSheetsMigrationHelperMongoMigrationHelperFirestoreMigrationHelperCacheEvictionHelperapp/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration003AddInstanceNameToTenantConfiguration.java (1)
21-22: Update class name and ChangeUnit id to reflect Organization terminology.For consistency with the codebase migration from Tenant to Organization, consider renaming:
- The class to
Migration003AddInstanceNameToOrganizationConfiguration- The ChangeUnit id to
add-instance-name-env-variable-organization-configuration-@ChangeUnit(order = "003", id = "add-instance-name-env-variable-tenant-configuration") -public class Migration003AddInstanceNameToTenantConfiguration { +@ChangeUnit(order = "003", id = "add-instance-name-env-variable-organization-configuration") +public class Migration003AddInstanceNameToOrganizationConfiguration {app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java (1)
203-218: Rename 'tenantId' fields to align with 'Organization'.
Although the code now queries the organization, it still uses 'tenantId' in the user record. This can be confusing.
🧹 Nitpick comments (38)
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/OrganizationConfiguration.java (1)
7-9: LGTM! Clean implementation following the CE pattern.The class is properly structured with Lombok annotations and correctly extends the CE base class. The
callSuper = truein@EqualsAndHashCodeensures proper equality comparisons.This empty implementation serves as an extension point for potential enterprise features, maintaining a clean separation from the community edition.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/OrganizationRepositoryCE.java (1)
8-10: Consider adding deprecation version and since tags.The deprecation notice and alternative solution are well documented. However, to better track the deprecation lifecycle, consider adding
@Deprecated.sinceand including the version when this will be removed.// Use organizationService.getDefaultOrganization() instead of this method as it is cached to redis. -@Deprecated(forRemoval = true) +@Deprecated(since = "1.0", forRemoval = true) Mono<Organization> findBySlug(String slug);app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Organization.java (1)
37-37: Consider creating a tracking issue for the SSO configuration TODO.The TODO comment indicates pending work for SSO configuration migration.
Would you like me to create an issue to track this TODO item for SSO and other configuration migrations?
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/MigrationHelperMethods.java (2)
213-231: Consider using non-blocking operations for cache eviction.The blocking call at line 227 could impact performance in high-throughput scenarios.
Consider refactoring to use reactive patterns consistently:
- cacheableRepositoryHelper - .evictPermissionGroupsUser(user.getEmail(), user.getOrganizationId()) - .block(); + cacheableRepositoryHelper + .evictPermissionGroupsUser(user.getEmail(), user.getOrganizationId()) + .subscribe();
661-675: Extract magic numbers to constants.The mapping uses hardcoded indices which could lead to maintenance issues.
Consider defining constants for these indices:
+ private static final int COMMAND_INDEX = 0; + private static final int SHEET_URL_INDEX = 1; + private static final int RANGE_INDEX = 2; final Map<Integer, List<String>> googleSheetsMigrationMap = Map.ofEntries( - Map.entry(0, List.of("command.data", "entityType.data")), - Map.entry(1, List.of("sheetUrl.data")), - Map.entry(2, List.of("range.data")), + Map.entry(COMMAND_INDEX, List.of("command.data", "entityType.data")), + Map.entry(SHEET_URL_INDEX, List.of("sheetUrl.data")), + Map.entry(RANGE_INDEX, List.of("range.data")),app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration065_CopyTenantIdToOrganizationId.java (1)
48-50: Consider providing an actual rollback strategy or clarifying why it's empty.
The empty rollback method might lead to orphaned or partially migrated data if the migration partially fails. Consider at least logging or providing a clear rationale for an empty rollback.app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/OrganizationConfig.java (2)
25-30: Improve method name and documentation.The method name is verbose and contains "Refresh" in the middle. Consider renaming to
refreshDefaultOrganizationPolicies()for better readability.
32-41: Consider extracting policy update logic.The policy update logic could be more readable by extracting the update operation into a separate method.
private Mono<Organization> cleanupAndUpdateRefreshDefaultOrganizationPolicies() { log.debug("Cleaning up and updating default organization policies on server startup"); - return organizationRepository.findBySlug(FieldName.DEFAULT).flatMap(organization -> { - if (CollectionUtils.isNullOrEmpty(organization.getPolicyMap())) { - organization.setPolicyMap(policySetToMap(organization.getPolicies())); - return cacheableRepositoryHelper - .evictCachedOrganization(organization.getId()) - .then(organizationRepository.save(organization)); - } - return Mono.just(organization); - }); + return organizationRepository.findBySlug(FieldName.DEFAULT) + .flatMap(this::updateOrganizationPoliciesIfNeeded); +} + +private Mono<Organization> updateOrganizationPoliciesIfNeeded(Organization organization) { + if (CollectionUtils.isNullOrEmpty(organization.getPolicyMap())) { + organization.setPolicyMap(policySetToMap(organization.getPolicies())); + return cacheableRepositoryHelper + .evictCachedOrganization(organization.getId()) + .then(organizationRepository.save(organization)); + } + return Mono.just(organization); }app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/InMemoryCacheableRepositoryHelper.java (1)
11-11: Consider adding @deprecated annotations for backward compatibility.To ensure a smooth migration, consider adding
@Deprecatedannotations with the old method names that delegate to the new ones.private String defaultOrganizationId = null; public String getDefaultOrganizationId() { return defaultOrganizationId; } public void setDefaultOrganizationId(String defaultOrganizationId) { this.defaultOrganizationId = defaultOrganizationId; } +@Deprecated(since = "1.0", forRemoval = true) +public String getDefaultTenantId() { + return getDefaultOrganizationId(); +} + +@Deprecated(since = "1.0", forRemoval = true) +public void setDefaultTenantId(String defaultTenantId) { + setDefaultOrganizationId(defaultTenantId); +}Also applies to: 23-25, 27-29
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CacheableFeatureFlagHelperCEImpl.java (1)
215-215: Field name mismatch.
You're setting tenantId to the organizationId. For consistency, consider renaming tenantId to organizationId in FeaturesRequestDTO.- featuresRequestDTO.setTenantId(organizationId); + featuresRequestDTO.setOrganizationId(organizationId);app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCEImpl.java (1)
35-37: Consider making the cache duration externally configurable.
Currently, the cache time is hardcoded to 115 minutes. Making it configurable via environment variables or app configuration would enhance flexibility.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java (1)
105-107: Use standard string equality checks instead of '=='.
In Java, 'mailHost == ""' should be replaced with 'mailHost.equals("")' or 'mailHost.isEmpty()' for clarity and correctness.- if (mailHost == null || mailHost == "") { + if (mailHost == null || mailHost.isEmpty()) {app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java (2)
180-180: @Cache annotation uses organizationId, but only returns the default org.This may cause confusion; consider renaming to reflect that the code always retrieves the default organization.
182-182: Method name 'fetchDefaultOrganization'.Consider removing the unused organizationId parameter or supporting a truly parameterized lookup for clarity.
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationSuccessHandlerCE.java (1)
73-75: Consider using Optional for configuration access.The nested method calls could lead to NullPointerException if the organization configuration is null.
- Mono<Boolean> emailVerificationEnabledMono = organizationService - .getOrganizationConfiguration() - .map(organization -> organization.getOrganizationConfiguration().isEmailVerificationEnabled()) + Mono<Boolean> emailVerificationEnabledMono = organizationService + .getOrganizationConfiguration() + .map(organization -> Optional.ofNullable(organization.getOrganizationConfiguration()) + .map(config -> config.isEmailVerificationEnabled()) + .orElse(false))app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCE.java (2)
11-33: Consider adding Javadoc for method documentation.While the method names are descriptive, adding Javadoc would help clarify:
- Parameters and return values
- Expected behavior
- Potential error conditions
Example for one method:
/** * Restarts the organization and its associated resources. * * @return Mono<Void> Completes when restart is finished * @throws OrganizationException if restart fails */ Mono<Void> restartOrganization();
28-28: Consider breaking down the migration method.The method name
checkAndExecuteMigrationsForOrganizationFeatureFlagssuggests it might be doing too much. Consider splitting into separate methods for checking and executing migrations.app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration055AddIsAtomicPushAllowedEnvVarToTenantConfiguration.java (1)
32-33: Rename "tenantQuery" to reflect Organization-based naming.
This aligns the query variable name with the updated type and avoids confusion.Proposed diff:
- Query tenantQuery = new Query(); - tenantQuery.addCriteria(where(Organization.Fields.slug).is("default")); - Organization defaultOrganization = mongoTemplate.findOne(tenantQuery, Organization.class); + Query organizationQuery = new Query(); + organizationQuery.addCriteria(where(Organization.Fields.slug).is("default")); + Organization defaultOrganization = mongoTemplate.findOne(organizationQuery, Organization.class);app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomOrganizationRepositoryCEImpl.java (1)
7-9: Add class-level documentation.Consider adding Javadoc to describe the purpose and responsibilities of this repository implementation.
@Slf4j +/** + * Repository implementation for Organization domain operations. + * Extends base repository functionality for Organization entities. + */ public class CustomOrganizationRepositoryCEImpl extends BaseAppsmithRepositoryImpl<Organization> implements CustomOrganizationRepositoryCE {}app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration061TenantPolicySetToPolicyMap.java (3)
21-21: Update migration ID to reflect Organization terminology.The migration ID still contains "tenant" which should be updated to maintain consistency with the organization terminology.
-@ChangeUnit(order = "061", id = "migrate-policy-set-to-map-tenant", author = " ") +@ChangeUnit(order = "061", id = "migrate-policy-set-to-map-organization", author = " ")
37-45: Update error message to use Organization terminology.The comment and error message still reference "tenant" which should be updated for consistency.
- // Fetch default tenant and verify earlier migration has updated the policyMap field + // Fetch default organization and verify earlier migration has updated the policyMap field Query tenantQuery = new Query(); tenantQuery.addCriteria(where(Organization.Fields.slug).is(DEFAULT)); Organization defaultOrganization = mongoTemplate.findOne(tenantQuery, Organization.class); if (defaultOrganization == null) { log.error( - "No default tenant found. Aborting migration to update policy set to map in tenant Migration061TenantPolicySetToPolicyMap."); + "No default organization found. Aborting migration to update policy set to map in organization Migration061TenantPolicySetToPolicyMap."); return; }
46-46: Update comment to use Organization terminology.The comment still references "tenant" which should be updated for consistency.
- // Evict the tenant to avoid any cache inconsistencies + // Evict the organization to avoid any cache inconsistenciesapp/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration059PolicySetToPolicyMap.java (2)
89-97: Update comments and assertion message to use consistent terminology.The code has been updated to use "organization", but comments and assertion messages still reference "tenant".
Apply this diff to maintain consistency:
- // Evict the default tenant from the cache to ensure that the updated tenant object is fetched from the database + // Evict the default organization from the cache to ensure that the updated organization object is fetched from the database Query tenantQuery = new Query(); tenantQuery.addCriteria(where(Organization.Fields.slug).is(DEFAULT)); Organization defaultOrganization = mongoTemplate.findOne(tenantQuery, Organization.class).block(); - assert defaultOrganization != null : "Default tenant not found"; + assert defaultOrganization != null : "Default organization not found"; cacheableRepositoryHelper .evictCachedOrganization(defaultOrganization.getId()) .block();
90-90: Rename variable to use consistent terminology.The variable name still uses "tenant" terminology.
Apply this diff:
- Query tenantQuery = new Query(); + Query organizationQuery = new Query();app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/OrganizationControllerCE.java (1)
24-26: Consider renaming the field for clarity.
Renaming “service” to “organizationService” improves readability.- private final OrganizationService service; + private final OrganizationService organizationService;app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java (2)
41-41: Potential concurrency consideration.
The cachedOrganizationFeatureFlags field could become stale under concurrent access.
161-164: Validate multi-organization scenarios.
Caching might produce an issue if multiple organizations exist.Also applies to: 166-166
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java (2)
589-630: Method name mismatch with the new Organization domain.
“addTenantAdminPermissionsToInstanceAdmin” should be updated to reflect “Organization” in its name for clarity.
664-664: Index naming: consider renaming 'tenantId_deleted'.
You might want to rename the index to “organizationId_deleted” to match the organizational pivot.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java (1)
403-405: Automatic association with the default organization.
Assigning the user to the default organization works well. For multi-org scenarios, consider an expanded approach.app/server/appsmith-server/src/main/java/com/appsmith/server/domains/UsagePulse.java (1)
18-24: LGTM! Consider adding @SInCE and @deprecated javadoc tags.The migration pattern looks good with proper deprecation handling. Consider enhancing the documentation with standard javadoc tags:
+ /** + * @since 1.x.x + */ private String organizationId; + /** + * @deprecated Use organizationId instead + */ @Deprecated // TODO: Remove this field once we have migrated the data to use organizationId instead of tenantId private String tenantId;app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/ConsolidatedApiSpanNamesCE.java (1)
18-18: Consider updating the span value to match the new terminology.The constant name has been updated to ORGANIZATION_SPAN but still uses the value "tenant". This might cause confusion.
Either:
- Update the value to match the new terminology:
- public static final String ORGANIZATION_SPAN = "tenant"; + public static final String ORGANIZATION_SPAN = "organization";
- Or add a comment explaining why the value remains as "tenant" for backward compatibility.
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Workspace.java (1)
50-56: Well-structured deprecation and migration approach.The changes properly handle the transition with:
- Clear deprecation marking
- Documented TODO for cleanup
- Consistent field visibility
Consider creating a tracking issue for the data migration and subsequent removal of the deprecated field.
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/ConsolidatedAPIResponseCE_DTO.java (3)
38-39: Update API endpoint documentation.The comment still references the old endpoint
v1/tenants/currentwhile the field has been migrated to use Organization.- /* v1/tenants/current */ + /* v1/organizations/current */
38-39: Update API endpoint comment to reflect the new organization endpoint.The comment still references the old endpoint
v1/tenants/currentwhile the field has been migrated to use Organization.- /* v1/tenants/current */ + /* v1/organizations/current */
38-39: Update API endpoint comment to reflect organization terminology.The comment
/* v1/tenants/current */should be updated to match the new organization-based endpoint.- /* v1/tenants/current */ + /* v1/organizations/current */app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UsagePulseServiceCEImpl.java (1)
57-57: Review and update TODO comment.The TODO comment references multi-tenancy which may need to be updated given the migration to organization-based model.
app/server/appsmith-server/src/main/java/com/appsmith/server/acl/AclPermission.java (1)
124-124: Consider updating permission name for consistency.While the class reference has been updated to
Organization.class, the permission string still uses "manage:tenants". Consider updating it to "manage:organizations" for complete consistency.- MANAGE_TENANT("manage:tenants", Organization.class), + MANAGE_TENANT("manage:organizations", Organization.class),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (107)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/OrganizationSpan.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/TenantSpan.java(0 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/ConsolidatedApiSpanNamesCE.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/OrganizationSpanCE.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/TenantSpanCE.java(0 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/dtos/ExecuteActionDTO.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/enums/FeatureFlagEnum.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/TriggerRequestDTO.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java(1 hunks)app/server/appsmith-plugins/appsmithAiPlugin/src/main/java/com/external/plugins/constants/AppsmithAiConstants.java(1 hunks)app/server/appsmith-plugins/appsmithAiPlugin/src/main/java/com/external/plugins/dtos/SourceDetails.java(2 hunks)app/server/appsmith-plugins/appsmithAiPlugin/src/main/java/com/external/plugins/utils/HeadersUtil.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/acl/AclPermission.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/FeatureFlaggedMethodInvokerAspect.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/AuthenticationSuccessHandler.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationSuccessHandlerCE.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/InstanceConfig.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/OrganizationConfig.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/TenantConfig.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/git/GitConfigCECompatibleImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/git/GitConfigCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/git/GitConfigImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/OrganizationController.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/OrganizationControllerCE.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Organization.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/domains/OrganizationConfiguration.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/domains/PermissionGroup.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/domains/PricingPlan.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/domains/TenantConfiguration.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/domains/UsagePulse.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/domains/User.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Workspace.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/UserSessionDTO.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/ConsolidatedAPIResponseCE_DTO.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/featureflags/FeatureFlagIdentityTraits.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/EmailServiceHelperImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/InMemoryCacheableRepositoryHelper.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/EmailServiceHelperCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCEImpl.java(7 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/InstanceConfigHelperCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/InstanceConfigHelperCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/UserUtilsCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java(10 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/MigrationHelperMethods.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration003AddInstanceNameToTenantConfiguration.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration021MoveGoogleMapsKeyToTenantConfiguration.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration055AddIsAtomicPushAllowedEnvVarToTenantConfiguration.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration059PolicySetToPolicyMap.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration061TenantPolicySetToPolicyMap.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration065_CopyTenantIdToOrganizationId.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/solutions/ce/UpdateSuperUserMigrationHelperCE.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/solutions/PluginTriggerSolutionCEImpl.java(5 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/solutions/PluginTriggerSolutionImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomOrganizationRepository.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomOrganizationRepositoryImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomTenantRepository.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomTenantRepositoryImpl.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/OrganizationRepository.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/TenantRepository.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java(5 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomOrganizationRepositoryCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomOrganizationRepositoryCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomPermissionGroupRepositoryCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomPermissionGroupRepositoryCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomTenantRepositoryCE.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomTenantRepositoryCEImpl.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomWorkspaceRepositoryCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomWorkspaceRepositoryCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/OrganizationRepositoryCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/TenantRepositoryCE.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/UserRepositoryCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/CacheableFeatureFlagHelperImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/FeatureFlagServiceImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/OrganizationService.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/OrganizationServiceImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/PermissionGroupServiceImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/TenantService.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/UsagePulseServiceImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserDataServiceImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserServiceImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserWorkspaceServiceImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CacheableFeatureFlagHelperCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CacheableFeatureFlagHelperCEImpl.java(7 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java(5 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java(5 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/PermissionGroupServiceCEImpl.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCE.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UsagePulseServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserDataServiceCEImpl.java(6 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java(12 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCEImpl.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCE.java(1 hunks)
⛔ Files not processed due to max files limit (33)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/ConsolidatedAPIServiceCECompatibleImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/PermissionGroupServiceCECompatibleImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/UserServiceCECompatibleImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ActionExecutionSolutionImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/DatasourceTriggerSolutionImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/EnvManagerImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ScheduledTaskImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/UserSignupImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceTriggerSolutionCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ScheduledTaskCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/FeatureFlaggedMethodInvokerAspectTest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/component/TestComponentImpl.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/configurations/SeedMongoData.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/domains/EqualityTest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/FeatureFlagMigrationHelperTest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/MockCacheableFeatureFlagHelper.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/EmailServiceHelperCETest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/UsagePulseServiceTest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserWorkspaceServiceUnitTest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/WorkspaceServiceTest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/EmailServiceCEImplTest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/FeatureFlagServiceCETest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/OrganizationServiceCETest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/TenantServiceCETest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/EnvManagerTest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/UserSignupTest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImplTest.java
💤 Files with no reviewable changes (13)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/TenantRepository.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomTenantRepositoryImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/TenantRepositoryCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomTenantRepositoryCE.java
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/TenantSpan.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/TenantService.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomTenantRepository.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomTenantRepositoryCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/TenantConfig.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/domains/TenantConfiguration.java
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/TenantSpanCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java
✅ Files skipped from review due to trivial changes (6)
- app/server/appsmith-server/src/main/java/com/appsmith/server/domains/PricingPlan.java
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/OrganizationSpan.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomOrganizationRepository.java
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/OrganizationSpanCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java
👮 Files not reviewed due to content moderation or server errors (4)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/ConsolidatedAPIResponseCE_DTO.java
- app/server/appsmith-plugins/appsmithAiPlugin/src/main/java/com/external/plugins/constants/AppsmithAiConstants.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UsagePulseServiceCEImpl.java
🧰 Additional context used
📓 Learnings (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java (1)
Learnt from: NilanshBansal
PR: appsmithorg/appsmith#33641
File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java:23-25
Timestamp: 2024-11-12T08:11:25.416Z
Learning: The new methods `fetchDefaultTenant` and `evictCachedTenant` in the `CacheableRepositoryHelperCE` interface are covered by existing and new tests, as confirmed by NilanshBansal.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java (1)
Learnt from: NilanshBansal
PR: appsmithorg/appsmith#33641
File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java:23-25
Timestamp: 2024-11-12T08:11:25.416Z
Learning: The new methods `fetchDefaultTenant` and `evictCachedTenant` in the `CacheableRepositoryHelperCE` interface are covered by existing and new tests, as confirmed by NilanshBansal.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (197)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/CacheableFeatureFlagHelperImpl.java (2)
5-5: LGTM! Constructor parameter updated correctly.The change from TenantRepository to OrganizationRepository is consistent with the PR objectives of migrating from Tenant to Organization terminology.
Also applies to: 16-16
23-23: LGTM! Super constructor call updated correctly.The super constructor parameter has been updated to match the new OrganizationRepository parameter.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/UsagePulseServiceImpl.java (1)
15-15: LGTM! Clean migration from tenant to organization.The changes correctly implement the tenant to organization migration by updating both the constructor parameter and the super call.
Also applies to: 18-18
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomPermissionGroupRepositoryCEImpl.java (2)
64-67: LGTM! Parameter rename aligns with organization-based model.The parameter rename from
tenantIdtoorganizationIdis consistent with the broader refactoring effort.
69-72: LGTM! Parameter rename maintains consistency.The parameter rename and delegation to
evictPermissionGroupsUsermaintains the existing behavior while aligning with the organization-based model.app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/git/GitConfigCECompatibleImpl.java (1)
4-4: LGTM! Clean migration from TenantService to OrganizationService.The changes correctly implement the tenant to organization migration while maintaining the component's functionality.
Also applies to: 9-10
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java (2)
22-40: LGTM! Constructor parameter updated correctly.The constructor parameter has been properly updated from
TenantServicetoOrganizationServiceas part of the tenant to organization migration.
41-60: LGTM! Super constructor call aligned with parameter changes.The super constructor call correctly reflects the parameter change, maintaining consistency with the constructor signature.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/OrganizationRepositoryCE.java (1)
7-7: LGTM! Clean interface hierarchy.The interface properly extends both
BaseRepositoryandCustomOrganizationRepositoryCE, following the repository pattern best practices.app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/solutions/PluginTriggerSolutionImpl.java (3)
7-7: LGTM! Import statement correctly updated.The import change aligns with the tenant to organization migration.
18-18: LGTM! Constructor parameter updated correctly.The constructor signature change maintains consistency with the organization-based architecture.
25-25: LGTM! Super constructor call updated appropriately.The super constructor invocation properly forwards the organizationService parameter.
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Organization.java (2)
21-21: LGTM! Class name change aligns with migration objectives.The rename from Tenant to Organization maintains proper inheritance and interface implementation.
35-35: LGTM! Configuration field rename maintains consistency.The field type and name change aligns with the organization-based terminology.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/EmailServiceHelperCEImpl.java (2)
3-4: LGTM! Clean migration from Tenant to Organization.The service dependency and imports have been correctly updated to use organization-based components.
Also applies to: 26-26
29-36: Verify the default instance name behavior.The method has been properly updated to use organization configuration. However, ensure that the default value "Appsmith" aligns with the application's branding requirements across all instances.
Consider externalizing the default instance name to a configuration constant for easier maintenance.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomWorkspaceRepositoryCEImpl.java (3)
32-33: LGTM: Method signature updated for organization context.The parameter change from
tenantIdtoorganizationIdaligns with the PR objective of migrating from tenant to organization terminology.
35-36: LGTM: Criteria field updated for organization filtering.The criteria field update from
tenantIdtoorganizationIdmaintains consistency with the organization-based filtering approach.
44-45: LGTM: User context updated to use organization ID.The change from
getTenantId()togetOrganizationId()correctly reflects the new organizational context.app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/InstanceConfig.java (1)
64-64: LGTM! Consistent tenant to organization migration.The changes correctly reflect the migration from tenant to organization terminology while maintaining the same functionality.
Also applies to: 74-74
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/InstanceConfigHelperCEImpl.java (1)
229-231: LGTM! Clean method rename with consistent implementation.The method has been properly renamed and its implementation updated to use the organization-focused feature flag service method.
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.java (2)
1-17: LGTM! Import changes align with the refactoring.The package structure and imports are correctly updated to reflect the tenant to organization migration.
66-85: LGTM! Method changes are consistent and robust.The copyNonSensitiveValues method:
- Correctly handles null checks
- Uses ObjectUtils for safe defaults
- Properly copies all configuration values
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration065_CopyTenantIdToOrganizationId.java (3)
63-66: Verify handling of multiple tenant documents.
The approach strictly assumes there's only one tenant. If multiple tenant documents exist, only the first will be migrated, leaving the rest unmigrated.
76-77: Double-check ID reuse when creating the new organization document.
Reusing the tenant’s ObjectId can create collisions if an organization with the same ID already exists (although you catch DuplicateKeyException later, it might block future insertions).
116-117: Confirm correctness of the update pipeline logic for partial or null tenantId.
If tenantId is null or empty in some documents, the update pipeline may overwrite organizationId incorrectly. Ensure data consistency is maintained.app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/OrganizationConfig.java (1)
17-24: LGTM! Well-structured Spring configuration class.The class follows Spring best practices with proper dependency injection and appropriate use of Lombok annotations.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/PermissionGroupServiceCEImpl.java (5)
21-21: LGTM!Import statement correctly added for the new OrganizationService.
48-48: LGTM!Field declaration correctly migrated from TenantService to OrganizationService.
62-62: LGTM!Constructor parameter and initialization correctly updated to use OrganizationService.
Also applies to: 70-70
275-280: LGTM!Method implementation correctly migrated to use organizationService.getDefaultOrganizationId() instead of tenantService.getDefaultTenantId().
284-284: Verify cache eviction behavior with organization IDThe cache eviction method now uses organizationId instead of tenantId. Ensure that this change doesn't affect existing cached entries.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other cache eviction methods to verify consistent usage rg -A 2 "evictAllPermissionGroupCachesForUser"Length of output: 2540
Cache eviction consistently uses organization IDs
- Confirmed that all invocations of evictAllPermissionGroupCachesForUser pass an organization ID as the second parameter.
- Both the service implementation and the helper methods use organization-related values, ensuring consistency with the updated behavior.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomOrganizationRepositoryCE.java (1)
1-6: LGTM! Clean interface definition.The interface follows Spring repository patterns and proper naming conventions.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomOrganizationRepositoryImpl.java (1)
1-6: LGTM! Implementation follows Spring patterns.Clean implementation extending the CE class and implementing the custom interface.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/OrganizationRepository.java (1)
1-7: LGTM! Verify tenant removal completeness.The repository interface is correctly defined with proper Spring annotations and inheritance.
Let's verify that all tenant references have been removed from the repository layer:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining "tenant" references in repository files rg -i "tenant" "app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/"Length of output: 89
Tenant references have been successfully removed from the repository layer.
The grep command returned no output, confirming that there are no stray "tenant" references remaining in the files under the repositories directory.
app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/solutions/PluginTriggerSolutionCEImpl.java (4)
13-13: LGTM! Import and field changes align with the migration.The changes correctly replace tenant-related dependencies with organization-based ones.
Also applies to: 31-31
39-39: LGTM! Constructor changes are consistent.The constructor properly initializes the organizationService field.
Also applies to: 45-45
84-84: LGTM! Feature flag retrieval updated correctly.The method now retrieves organization-level feature flags instead of tenant-level flags.
94-94: Verify the impact of using default organization ID.The method has been correctly renamed and updated to use organization-based calls. However, please verify that using
getDefaultOrganizationId()instead of the previous tenant ID retrieval method maintains the expected behavior in all scenarios.Let's check for any potential edge cases:
Also applies to: 118-127
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other usages of getDefaultOrganizationId to understand the context rg "getDefaultOrganizationId" -A 3 # Search for any remaining tenant-related calls that might need migration rg "getTenantId|setTenantId" -A 3Length of output: 17154
Impact of Using Default Organization ID Verified
Based on the search outputs, the migration from tenant-based methods to organization-based ones is consistently implemented. The usage of getDefaultOrganizationId appears throughout the codebase, and the few remaining tenant-related calls seem confined to migration-specific code (e.g., in DatabaseChangelog2 and within a helper that is likely part of the migration process).
• The PluginTriggerSolutionCEImpl, along with other core services, now relies on getDefaultOrganizationId, ensuring consistency.
• Legacy tenant calls exist only in migration and helper areas, not affecting runtime behavior.No edge cases were found that would unintentionally change the expected behavior.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/InMemoryCacheableRepositoryHelper.java (1)
11-11: Verify all callers have been updated.The renaming from tenant to organization looks good, but we should ensure all callers have been updated to use the new method names.
Also applies to: 23-25, 27-29
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining references to the old method names echo "Searching for old method references..." rg "getDefaultTenantId|setDefaultTenantId" -t java # Search for usage of new method names to confirm migration echo "Verifying new method usage..." rg "getDefaultOrganizationId|setDefaultOrganizationId" -t javaLength of output: 4836
All method references have been successfully updated.
- No remaining references to “getDefaultTenantId” or “setDefaultTenantId” were found.
- All call sites now reference the new organization-based methods.
- Consider adding deprecation annotations to the legacy methods if backward compatibility is desired.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserWorkspaceServiceImpl.java (1)
20-20: LGTM! Verify dependent service updates.The constructor parameter and super call changes correctly align with the tenant to organization migration.
Run this script to verify that all dependent services have been updated:
Also applies to: 30-30
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining tenant references in service implementations # and verify organization service injection in workspace-related services # Check for any remaining tenant references in service implementations echo "Checking for remaining tenant references in services..." rg -l "TenantService|tenantService" "app/server/appsmith-server/src/main/java/com/appsmith/server/services" # Check workspace-related services for organization service injection echo "Checking workspace services for organization service..." rg -l "class.*WorkspaceService.*|class.*WorkspacePermission.*" "app/server/appsmith-server/src/main/java/com/appsmith/server/services" | xargs rg "OrganizationService"Length of output: 1152
LGTM – TenantService successfully replaced with OrganizationService.
The shell script output confirms that:
- There are no remaining references to TenantService.
- The workspace services now correctly inject and use OrganizationService.
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/solutions/ce/UpdateSuperUserMigrationHelperCE.java (3)
6-6: LGTM! Import statement updated correctly.The import statement has been properly updated to reflect the migration from Tenant to Organization.
27-27: LGTM! Method parameter type updated correctly.The parameter type change from Tenant to Organization aligns with the migration objectives.
48-48: LGTM! User creation logic updated consistently.The changes in createNewUser method maintain consistency with the Organization migration:
- Parameter type updated
- Organization ID setting replaced tenant ID
- Method call parameter updated
Let's verify that all tenant references have been properly migrated:
Also applies to: 56-56, 63-63
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining 'tenant' references in the codebase rg -i "tenant" --type javaLength of output: 29572
Verified: User creation logic updates are consistent and expected tenant references remain only where legacy migration or test scenarios require them.
The update in UpdateSuperUserMigrationHelperCE.java (and similarly in lines 56 and 63) correctly replaces tenant-based parameters with organization-based ones. The shell script's output shows numerous occurrences of "tenant" still present elsewhere in the codebase—mostly in constants, ACL configurations, migration scripts, and tests. These references are part of legacy handling and migration support, not the user creation logic itself.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CacheableFeatureFlagHelperCEImpl.java (9)
7-7: Good job aligning with the new organization model.
Revising from TenantRepository to OrganizationRepository cleanly reflects the new domain logic.Also applies to: 19-19, 47-47
91-91: Confirm that each user can reliably have a single organizationId.
If a user can belong to multiple organizations, storing only one ID might be insufficient. Please verify.
165-166: Documentation aligns with the new parameter.
The updated comments correctly describe the organization-based approach.
169-172: New caching method is consistent.
Fetching organization features and caching them is logically aligned with the domain model.
186-189: Update operation for cached features appears straightforward.
Logging the update is a good practice for observability.
194-195: Cache eviction is well-defined.
The new methods for evicting organization features look correct and maintain clarity around cache management.Also applies to: 198-200
205-206: Method systematically retrieves remote features for an organization.
Implementation matches the pattern of forcing a refresh from the remote service.Also applies to: 209-209
221-221: Chain call for retrieving features.
The flatMap call to the new organization-focused method is consistent with the updated flow.
228-228: Documentation and method name are consistent.
Renaming from getRemoteFeaturesForTenant to getRemoteFeaturesForOrganization clarifies the domain shift.Also applies to: 233-233
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomPermissionGroupRepositoryCE.java (1)
26-26: LGTM! Parameter renaming aligns with organization migration.The parameter renaming from
tenantIdtoorganizationIdis consistent with the broader migration effort.Please verify that corresponding cache implementations have been updated to use the new parameter name. Run this to find related cache configurations:
Also applies to: 28-28
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for cache configurations related to permission groups rg -l "cache" --type java | xargs rg "PermissionGroup.*Cache"Length of output: 2850
LGTM – Cache configurators now consistently use "organizationId".
The shell script output confirms that cache eviction implementations and calls (e.g., in CustomPermissionGroupRepositoryCEImpl and UserUtilsCE) have been updated to use the new parameter naming.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCEImpl.java (5)
5-6: Renamed imports are consistent.
These two import lines correctly replace tenant references with organization references, aligning with the migration goal.
40-43: getUpdatedFlagsWithPendingMigration logic looks good.
The forced refresh logic and migration from tenant to organization are coherent. No issues spotted.Also applies to: 48-50, 54-55, 62-65, 69-74
79-80: refreshOrganizationFeatures changes are well-structured.
Switching to organization-focused caching and fallback behavior is implemented neatly.Also applies to: 82-82, 85-86, 93-93, 95-96, 102-104
203-203: Migration methods updated correctly.
The rename from tenant to organization is comprehensive, and the documentation changes match the new parameters. Just ensure organizationConfiguration is checked before use, as done in some places but not everywhere.Also applies to: 207-208, 210-210, 212-213, 216-216, 222-222, 231-231, 237-238, 241-243, 248-248, 265-266, 270-271
132-135: 🛠️ Refactor suggestionPotential null check needed for organizationConfiguration.
Accessing dbOrganization.getOrganizationConfiguration() without verifying it's non-null could lead to NPE. Consider guarding these references or defaulting to an empty config.Also applies to: 156-157, 160-160, 162-162
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/UserUtilsCE.java (2)
112-114: LGTM! Maintains consistency with organization terminology.The change mirrors the one in
makeSuperUser, maintaining consistency across the codebase.
93-95: LGTM! Consistent with tenant to organization migration.The change from
getTenantId()togetOrganizationId()aligns with the PR's objective.Let's verify the migration's consistency:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check for any remaining tenant references in the codebase rg -i 'tenant' --type javaLength of output: 29572
LGTM – Changes in UserUtilsCE.java are consistent with the tenant-to-organization migration.
The switch from user.getTenantId() to user.getOrganizationId() in lines 93–95 (and in a similar change at line 113) is correct for this migration. Note that the shell script output shows additional "tenant" references elsewhere in the codebase (e.g., in migration scripts, tests, and constants). These remaining references are expected during an incremental migration and do not impact the correctness of the changes in UserUtilsCE.java.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/OrganizationServiceImpl.java (2)
14-15: LGTM! Clean migration from Tenant to Organization.The class declaration and inheritance hierarchy have been properly updated to reflect the new Organization terminology.
17-37: Constructor implementation looks good.The constructor parameters and super call are properly aligned with the Organization migration. All dependencies are correctly injected.
Let's verify that all Tenant references have been consistently replaced across the codebase:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining "tenant" references that might have been missed rg -i "tenant" --type javaLength of output: 29572
Tenant Reference Consistency Verification:
The search results show that “tenant” references still appear in various parts of the codebase—primarily within migration classes, tests, configuration, and constants used for logging or instrumentation. This usage is consistent with our migration strategy (e.g., in Migration065_CopyTenantIdToOrganizationId and related migration files) where legacy “tenant” identifiers are intentionally referenced as part of the conversion process. In production service code (like in OrganizationServiceImpl), the injection and usage align with the new “organization” nomenclature, and the remaining “tenant” references are expected for backward compatibility and transitional logic.No inconsistencies or issues were detected regarding how tenant references are used.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java (2)
35-35: Suspected leftover from tenant naming.
There is a static import of MANAGE_TENANT at line 35, which might be inconsistent with the organization's naming migration effort. Verify if this should be renamed (e.g., MANAGE_ORGANIZATION) to maintain naming consistency.
308-322: Confirm consistency of cache eviction.
Unlike the “save” method, “update” correctly uses the organizationId parameter for cache eviction. Verify that both methods consistently evict the appropriate organization from the cache to prevent stale data issues.app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java (20)
7-8: Imports updated for Organization.
These changes coherently replace Tenant references with Organization references.
32-32: New import for 'FETCH_ORGANIZATION_FROM_DB_SPAN'.
The static import is aligned with the transition to OrganizationSpan references.
48-48: User organization-based cache key.
Using user.email + user.organizationId ensures better scoping for the cache entry.
130-131: Evicting permission groups by organization.
Transition from tenantId to organizationId is consistent with the refactor.
132-132: Method signature for evictPermissionGroupsUser.
Parameters now reflect the organizational context.
137-139: getDefaultOrganizationId method.
The logic to return from cached value if present looks correct and straightforward.
143-144: Criteria for default organization.
Replacing tenant slug logic with organization slug aligns well with the migration.
146-146: Adding criteria to query.
Extends the existing query approach for organization retrieval.
148-148: MongoDB query for default organization.
The flow to fetch from MongoDB if cache is empty is sound.
149-151: Caching the defaultOrganizationId.
A sensible approach to reduce DB lookups going forward.
174-174: Documentation updated for organizational context.
Accurately reflects the new references to Organization.
176-177: Docstring clarifying the default Organization fetch.
Useful for understanding fallback behavior when cache is empty.
183-184: Dynamic query checks 'slug' against 'DEFAULT'.
Correctly implements a default lookup.
186-186: Combining the notDeleted criteria.
Appropriately filters out removed entries.
189-189: Log statement.
Helpful for debugging when the cache is not found.
191-191: findOne on Organization collection.
Query aligns with the bridging logic.
192-196: Ensuring OrganizationConfiguration is non-null.
Safeguarding the domain object is a good practice.
198-198: Naming the reactive chain for micrometer.
Facilitates better telemetry.
202-202: Cache eviction for 'organization'.
Aligns with the updated entity.
204-204: Mono return type.
Eviction is effectively a no-op at the moment, but the contract is clear.app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/AuthenticationSuccessHandler.java (1)
11-11: LGTM! Verify complete migration from Tenant to Organization.The changes correctly replace TenantService with OrganizationService in imports, constructor parameters, and super constructor call.
Let's verify the complete migration of Tenant to Organization across the codebase:
Also applies to: 35-35, 49-49
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining references to "Tenant" that might have been missed rg -i "tenant" --type java # Search for new Organization references to ensure consistent naming rg -i "organization(service|config|span)" --type javaLength of output: 65903
Migration in AuthenticationSuccessHandler.java is correctly implemented.
The updates in this file—including replacing TenantService with OrganizationService in the import, constructor, and the super call—are consistent with the migration objectives. While our broader codebase search shows several remaining occurrences of “tenant” (in constants, migrations, and tests), these appear to be legacy or transitional references that don’t impact the changes in this handler. Please ensure that any non-transitional “tenant” usages are addressed as part of the overall migration plan.
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationSuccessHandlerCE.java (3)
20-20: LGTM! Service migration looks good.The TenantService has been correctly replaced with OrganizationService.
Also applies to: 68-68
109-113: Documentation update looks good.The comment block clearly explains the behavior when email verification is enabled after signup.
72-125: Verify error handling for email verification state changes.The method handles the transition from tenant to organization well, but we should verify the error handling when organization configuration changes while a user is in the verification process.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCE.java (2)
5-5: LGTM! Import statement correctly updated.The import statement has been properly updated to use the Organization domain class.
12-13: LGTM! Method signatures consistently updated.All method signatures have been properly updated to use Organization instead of Tenant, maintaining consistent parameter naming across the interface.
Let's verify that all implementations of this interface have been updated accordingly:
Also applies to: 15-16, 18-18, 20-20
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for implementations of FeatureFlagMigrationHelperCE ast-grep --pattern 'class $_ implements FeatureFlagMigrationHelperCE { $$$ }'Length of output: 81
Below is an additional verification script to search for all implementations of the interface, the usage of the updated method signature, and ensure the old "Tenant" type is no longer present in the interface file. This should help us gather more confidence that all implementations have been updated as intended.
Once these results are available, we can conclusively verify if the interface and all its implementations have been updated accordingly.
🏁 Script executed:
#!/bin/bash echo "Searching for implementations of FeatureFlagMigrationHelperCE interface:" rg "implements FeatureFlagMigrationHelperCE" -n echo "" echo "Searching for occurrences of 'getUpdatedFlagsWithPendingMigration' (to check method signatures):" rg "getUpdatedFlagsWithPendingMigration" -n echo "" echo "Verifying that the FeatureFlagMigrationHelperCE interface file does not mention the old type 'Tenant':" rg "Tenant" -n app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCE.javaLength of output: 7759
All implementations correctly updated to use Organization and no Tenant references found.
The shell script outputs confirm:
• Implementation in FeatureFlagMigrationHelperCEImpl.java exists and uses the updated method signature.
• Numerous references to getUpdatedFlagsWithPendingMigration consistently use defaultOrganization.
• The interface file shows no occurrence of the old type Tenant.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCE.java (2)
1-8: Clean and well-organized imports!The package structure and imports are properly organized, following Java conventions.
9-9: Interface extension looks good!The interface properly extends CrudService with appropriate type parameters.
Let's verify the consistency of CE suffix usage across the codebase:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check for consistent CE suffix usage in service interfaces rg -t java "interface \w+CE" "app/server/appsmith-server/src/main/java/com/appsmith/server/services"Length of output: 6604
Interface extension and CE suffix consistency verified!
The search showed that the CE naming convention is applied consistently across the service interfaces in our codebase, including the OrganizationServiceCE that extends CrudService with Organization and String.
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/FeatureFlaggedMethodInvokerAspect.java (2)
68-68: LGTM!Comment updated correctly to reflect the organization terminology.
111-111: Verify feature flag data migration.The change from
getCachedTenantFeatureFlags()togetCachedOrganizationFeatureFlags()looks correct, but we should ensure that:
- All existing feature flags are migrated correctly
- The new method is properly implemented in
FeatureFlagService✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for the new method implementation and any migration scripts echo "Searching for the new method implementation..." ast-grep --pattern 'getCachedOrganizationFeatureFlags() { $$$ }' echo "Searching for migration scripts..." fd -e java -e sql -e xml migration | xargs rg -l "tenant.*feature.*flag|organization.*feature.*flag"Length of output: 463
Feature Flag Migration Verification
The new method is referenced by its usage, and we located a migration helper file:
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCEImpl.java
This indicates that the migration from tenant to organization feature flags is being handled. Please verify that:
- All feature flags are correctly migrated in FeatureFlagMigrationHelperCEImpl.java.
- The implementation of getCachedOrganizationFeatureFlags() in FeatureFlagService returns the expected flags.
Overall, the changes appear correctly implemented.
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/git/GitConfigImpl.java (1)
4-4: Verify parent class compatibility with OrganizationService.The changes align with the tenant to organization migration. Let's verify the parent class compatibility.
Also applies to: 9-10
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if GitConfigCECompatibleImpl constructor accepts OrganizationService ast-grep --pattern 'class GitConfigCECompatibleImpl { $$$ GitConfigCECompatibleImpl(OrganizationService $_) { $$$ } $$$ }'Length of output: 136
We'll generate a new shell script to dig a bit deeper into the parent class. This script will locate the file containing the GitConfigCECompatibleImpl class and then search for constructor definitions and OrganizationService references to ensure the constructor signature has been updated.
🏁 Script executed:
#!/bin/bash # Locate the file containing GitConfigCECompatibleImpl file=$(fd -e java -HI "GitConfigCECompatibleImpl.java") echo "Found file: $file" if [ -n "$file" ]; then echo "----- Searching for constructor definitions using OrganizationService -----" rg "GitConfigCECompatibleImpl\\(.*OrganizationService" "$file" -n echo "----- Searching for any use of OrganizationService in the file -----" rg "OrganizationService" "$file" -n else echo "GitConfigCECompatibleImpl.java not found." fiLength of output: 1385
Parent Class Compatibility Confirmed
The updated GitConfigCECompatibleImpl now correctly defines a constructor with OrganizationService, consistent with the migration from TenantService. The findings show:
- The import for OrganizationService and the constructor accepting this type are present in GitConfigCECompatibleImpl (line 9).
- This aligns with the migration objective, and no additional issues were identified.
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration055AddIsAtomicPushAllowedEnvVarToTenantConfiguration.java (3)
3-4: Imports are aligned with the new Organization-based domain.
No issues spotted here; these imports correctly reflect the shift from Tenant to Organization.
41-42: Use the updated variable name for consistency.
This block still sets a value to “defaultTenantConfiguration,” which contradicts the new Organization naming. See comment above.
45-46: Use the updated variable name for consistency.
Same mismatch with “defaultTenantConfiguration” in the assignment.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/InstanceConfigHelperCE.java (1)
20-20: LGTM! Method rename aligns with organization migration.The method rename maintains consistency with the broader tenant-to-organization migration effort.
Let's verify the implementing classes have been updated accordingly:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for classes implementing InstanceConfigHelperCE ast-grep --pattern 'class $_ implements $$$InstanceConfigHelperCE$$$' # Search for any remaining references to updateCacheForTenantFeatureFlags rg "updateCacheForTenantFeatureFlags"Length of output: 109
Let's run a more comprehensive check to verify that the new method name is consistently used across implementations. We'll search for interface implementations and the new and old method references in Java files:
🏁 Script executed:
#!/bin/bash # List all Java files that implement InstanceConfigHelperCE rg -l "implements InstanceConfigHelperCE" --type java # Search for occurrences of the new method name in Java files rg "updateCacheForOrganizationFeatureFlags" --type java # Ensure no references to the old method name remain rg "updateCacheForTenantFeatureFlags" --type javaLength of output: 785
LGTM! Method rename is consistent across the codebase.
- The implementation in InstanceConfigHelperCEImpl.java reflects the new method name.
- No references to updateCacheForTenantFeatureFlags were found.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/OrganizationService.java (1)
1-5: LGTM! Clean interface implementation following the CE pattern.The interface correctly extends OrganizationServiceCE, maintaining the separation between community and enterprise editions.
Let's verify the impact on dependent services that might still be using TenantService:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining references to TenantService rg "TenantService" --type javaLength of output: 30
I'll generate another shell script that uses the "--stats" flag so we can see the statistics—even if there are no matches—which might help verify that dependent services aren't referencing TenantService.
🏁 Script executed:
#!/bin/bash # Run a search for "TenantService" with stats to show the total number of search hits rg --stats "TenantService" --type javaLength of output: 201
Dependency Verification: No Remaining TenantService References
The shell script confirmed that no references to TenantService exist in the Java codebase. The OrganizationService interface correctly extends OrganizationServiceCE and the migration from Tenant to Organization aligns with this update.
- The search across 1948 files yielded 0 matches for TenantService.
- This indicates no lingering dependency issues with TenantService in the codebase.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomWorkspaceRepositoryCE.java (1)
16-17: LGTM! Parameter rename aligns with organization migration.The change from
tenantIdtoorganizationIdis consistent with the PR's objective.Let's verify the parameter rename is consistently applied across implementations:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining references to tenantId in workspace-related files rg -i "tenantId.*workspace" --type java # Search for the implementation class to verify the parameter name change ast-grep --pattern 'class $_ implements CustomWorkspaceRepositoryCE { $$$ findByIdsIn($$$) { $$$ } }'Length of output: 155
LGTM! The parameter rename from tenantId to organizationId is consistently applied.
No instances of lingering tenantId references were detected in workspace-related Java files, and the repository method signature aligns with our migration strategy.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java (3)
456-457: LGTM! Method chain update is consistent.The update to use organization terminology in the method chain is correct and maintains the expected behavior.
480-485: LGTM! Consistent implementation in getStrictPermissionGroupsForUser.The changes mirror those in
getAllPermissionGroupsForUser, maintaining consistency across both methods.
449-454: LGTM! Organization ID check and setting logic is correct.The logic for checking and setting the organization ID is consistent with the migration from tenant to organization terminology.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Verify that all tenant references have been migrated to organization rg -i "tenant(?!.*organization)" -g "*.java"Length of output: 280
Let's re-run the verification using PCRE2 to support lookahead. We'll run a new shell script using the --pcre2 flag:
🏁 Script executed:
#!/bin/bash # Re-run the search using PCRE2 to support lookahead for verifying tenant references migration rg --pcre2 -i "tenant(?!.*organization)" -g "*.java"Length of output: 21234
LGTM – Organization ID check remains valid in BaseAppsmithRepositoryCEImpl.java
The changes in lines 449–454 correctly verify and set the organization ID. Although our PCRE2 search revealed several remaining "tenant" references, these occur in areas such as migration scripts, caching, or legacy constants and do not affect the logic of this refactored snippet.
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration003AddInstanceNameToTenantConfiguration.java (1)
34-36: Verify migration's backward compatibility.The query has been updated to use Organization.Fields.slug, but we need to ensure this won't break existing data migration paths.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if there are any existing migrations that might depend on the old tenant query rg -l "Tenant\.Fields\.slug" --type javaLength of output: 42
Migration backward compatibility verified:
The search for any reference to Tenant.Fields.slug did not yield any results from the codebase, and the migration file clearly uses Organization.Fields.slug. Thus, there is no indication that updating the query would break data migration paths.app/server/appsmith-server/src/main/java/com/appsmith/server/services/PermissionGroupServiceImpl.java (2)
27-37: LGTM! Super call updated consistently.The super call correctly passes the organizationService parameter, maintaining consistency with the constructor change.
16-25: LGTM! Constructor parameter updated correctly.The constructor signature has been properly updated to use OrganizationService, aligning with the tenant to organization migration.
Let's verify that the parent class
PermissionGroupServiceCECompatibleImplhas been updated to handleOrganizationService:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if the parent class constructor has been updated ast-grep --pattern 'class PermissionGroupServiceCECompatibleImpl { $$$ PermissionGroupServiceCECompatibleImpl($$$, OrganizationService $_, $$$) { $$$ } }'Length of output: 166
Let's run an additional shell script command to locate the parent class file and inspect its constructor signature for the presence of OrganizationService. This should help confirm that the parent class has been updated accordingly.
🏁 Script executed:
#!/bin/bash # Locate the parent class file and inspect its constructor file=$(fd --type f "PermissionGroupServiceCECompatibleImpl.java") if [ -n "$file" ]; then echo "Found file: $file" echo "Constructor signatures in the file:" rg -n "public PermissionGroupServiceCECompatibleImpl" "$file" echo "Occurrences of OrganizationService in the file:" rg -n "OrganizationService" "$file" else echo "Parent class file PermissionGroupServiceCECompatibleImpl.java not found." fiLength of output: 1318
LGTM! Parent Class Constructor Update Verified
The parent class file "PermissionGroupServiceCECompatibleImpl.java" correctly includes the OrganizationService parameter in its constructor (imported and used at line 23), confirming consistency with the migration from TenantService.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/EmailServiceHelperImpl.java (1)
4-4: LGTM! Verify parent class compatibility.The changes align with the tenant to organization migration. The constructor changes and import updates are correct.
Let's verify that the parent class
EmailServiceHelperCEImplhas been updated to useOrganizationService:Also applies to: 9-10
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if the parent class EmailServiceHelperCEImpl has been updated ast-grep --pattern 'class EmailServiceHelperCEImpl { $$$ EmailServiceHelperCEImpl($service) { $$$ } $$$ }'Length of output: 120
I'll generate a new shell script to inspect the parent class file "EmailServiceHelperCEImpl.java" to verify its constructor signature and confirm that it utilizes OrganizationService. This will help ensure the migration changes are consistent across the class hierarchy.
🏁 Script executed:
#!/bin/bash # Locate the parent class file and display its constructor signature along with occurrences of OrganizationService. # This should help confirm that the constructor now expects an OrganizationService. fd -e java EmailServiceHelperCEImpl.java | xargs rg "public EmailServiceHelperCEImpl\(" -n echo "----- Occurrences of OrganizationService in EmailServiceHelperCEImpl.java -----" fd -e java EmailServiceHelperCEImpl.java | xargs rg "OrganizationService" -nLength of output: 459
The parent class appears updated.
We confirmed that EmailServiceHelperCEImpl now imports and uses OrganizationService (line 4 import and line 26 declaring the field), which is consistent with the changes in EmailServiceHelperImpl. The migration to OrganizationService is properly reflected across the class hierarchy.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCE.java (1)
15-15: LGTM! Method signature updated correctly.The change from
findByEmailAndTenantIdtofindByEmailAndOrganizationIdaligns with the PR objective of migrating from Tenant to Organization.Let's verify consistent renaming across related files:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining references to 'tenantId' in repository interfaces and implementations rg -l "tenantId" --type java "app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/" # Search for the new method name to ensure it's implemented ast-grep --pattern 'findByEmailAndOrganizationId'Length of output: 2344
LGTM! All references have been updated consistently.
We verified the change across the codebase and found that:
- The new method signature is present in all relevant repository and service files.
- No remaining references to "tenantId" were detected in repository-related Java files.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomOrganizationRepositoryCEImpl.java (1)
8-9: Verify empty implementation.Please verify that all required methods from
CustomOrganizationRepositoryCEare either:
- Already implemented in
BaseAppsmithRepositoryImpl- Or there are no abstract methods to implement
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for abstract methods in CustomOrganizationRepositoryCE ast-grep --pattern 'interface CustomOrganizationRepositoryCE { $$$ }'Length of output: 73
Let's gather additional information by printing the contents of the interface file to check if it declares any abstract methods. This will help us verify if the empty implementation in CustomOrganizationRepositoryCEImpl is acceptable.
🏁 Script executed:
#!/bin/bash # Locate and display the content of the CustomOrganizationRepositoryCE interface definition fd "CustomOrganizationRepositoryCE.java$" | while read file; do echo "Displaying file: $file" cat "$file" doneLength of output: 703
Empty Implementation Verified.
The interface CustomOrganizationRepositoryCE doesn't declare any additional abstract methods beyond those inherited from AppsmithRepository, and as BaseAppsmithRepositoryImpl likely provides their implementation, the empty implementation in CustomOrganizationRepositoryCEImpl is correct.app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration061TenantPolicySetToPolicyMap.java (2)
47-55: LGTM! Policy map migration logic looks correct.The logic for migrating policies to policy map and cache eviction is well-implemented.
32-33: Consider implementing rollback logic.The rollback execution is empty. While this might be intentional, consider documenting why rollback is not supported or implement rollback logic if possible.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if other migrations in the codebase implement rollback logic rg -l "RollbackExecution.*{[\s\S]*?}" app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/Length of output: 244
🏁 Script executed:
#!/bin/bash # Display @RollbackExecution annotated methods with surrounding context from migration files rg -A 10 "@RollbackExecution" app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/Length of output: 67080
Subject: Clarify Rollback Intent in Migration061
This migration’s rollback method is intentionally left empty—as seen in several other migration classes in the codebase (e.g., Migration010UpdatePluginDocsLink and Migration012RemoveStructureFromWithinDatasource, which include comments explaining why no rollback logic is needed). If rollback is genuinely not supported or applicable for this migration, please add a brief comment to document this rationale. Otherwise, if rollback logic is required, consider implementing it similarly to other migrations that perform reversible operations.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CacheableFeatureFlagHelperCE.java (5)
19-23: LGTM! Documentation and method signature updated correctly.The fetch operation has been properly renamed from tenant to organization context.
25-25: LGTM! Method signature updated correctly.The update operation has been properly renamed from tenant to organization context.
28-32: LGTM! Documentation and method signature updated correctly.The evict operation has been properly renamed from tenant to organization context.
35-39: LGTM! Documentation and method signature updated correctly.The remote features operation has been properly renamed from tenant to organization context.
10-40: Verify interface implementations.Since this is an interface change, let's verify that all implementing classes have been updated accordingly.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for classes implementing CacheableFeatureFlagHelperCE ast-grep --pattern 'class $_ implements $$$CacheableFeatureFlagHelperCE$$$' # Search for any remaining references to tenant-related method names rg -i 'tenant.*features'Length of output: 130742
Interface implementations verified; no stale tenant references found.
Scanning the codebase with our AST pattern did not reveal any classes improperly implementing CacheableFeatureFlagHelperCE. Additionally, the only tenant-related references appear within an SQL dump used for testing purposes, which isn’t part of the business logic. The interface change seems fully propagated with no residual issues.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCE.java (1)
31-31: LGTM! Verify implementations.The parameter rename aligns with the organization migration initiative.
Let's verify the implementation classes handle this change consistently:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for implementations of findByIdsIn to ensure they use organizationId ast-grep --pattern 'class $_ implements WorkspaceServiceCE { $$$ findByIdsIn($_, $_, $_) { $$$ } $$$ }'Length of output: 117
Let's run an additional search using ripgrep to check for any occurrences of "findByIdsIn" that reference "organizationId" within their parameter list. This will help us verify whether the implementation classes have been updated accordingly.
🏁 Script executed:
#!/bin/bash # Search for implementations or usages of 'findByIdsIn' that include 'organizationId' in the parameter list rg "findByIdsIn\s*\(.*organizationId" -n .Length of output: 644
LGTM – Consistency Verified
The search results confirm that the parameter rename from tenantId to organizationId is applied consistently in both the interface and its implementation.app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration059PolicySetToPolicyMap.java (2)
3-3: LGTM! Import change aligns with the migration objective.
61-61: Verify if the "tenant" collection name needs to be updated.The collection names still include "tenant". If this is a database collection name, it might require a separate migration to rename.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if there are any other references to the "tenant" collection name rg -l "\"tenant\"" --type javaLength of output: 389
Tenant Collection Naming Verification
The search reveals that the string "tenant" is used consistently in multiple migration files (for example, in both Migration059PolicySetToPolicyMap.java and Migration065_CopyTenantIdToOrganizationId.java). This indicates that if renaming is desired—from "tenant" to (for example) "organization"—a dedicated, comprehensive migration covering all affected references is needed rather than a simple code change.
• Migration059PolicySetToPolicyMap.java (line 61) still includes "tenant".
• Migration065_CopyTenantIdToOrganizationId.java also references "tenant", implying that the conversion logic (e.g., copying tenant ID to organization ID) is already handled separately.No immediate update is required unless a decision is made to rename all "tenant" references consistently across the database.
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration021MoveGoogleMapsKeyToTenantConfiguration.java (2)
4-4: LGTM! Import updated correctly.The import has been correctly updated to use OrganizationConfiguration.
24-25: Update class name and query to use consistent organization terminology.The migration class still uses tenant terminology which should be updated to maintain consistency with the organization migration:
- Class name and @ChangeUnit id still reference "tenant"
- Update query uses "tenantConfiguration" path which might not match the new organization schema
Apply these changes:
-@ChangeUnit(order = "021", id = "move-google-maps-key-to-tenant-configuration") -public class Migration021MoveGoogleMapsKeyToTenantConfiguration { +@ChangeUnit(order = "021", id = "move-google-maps-key-to-organization-configuration") +public class Migration021MoveGoogleMapsKeyToOrganizationConfiguration { mongoTemplate.updateFirst( new Query(where("slug").is("default")), - new Update().set("tenantConfiguration.googleMapsKey", mapsEnvValue), + new Update().set("organizationConfiguration.googleMapsKey", mapsEnvValue), OrganizationConfiguration.class);Please verify:
- The schema structure in OrganizationConfiguration matches the update path
- No other migrations depend on this class name
Also applies to: 39-40
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check for references to the migration class name rg "Migration021MoveGoogleMapsKeyToTenantConfiguration" -A 5 # Check OrganizationConfiguration schema ast-grep --pattern 'class OrganizationConfiguration { $$$ googleMapsKey $$$ }'Length of output: 1172
Action: Update migration class and query to use organization terminology
- The grep results confirm that the class Migration021MoveGoogleMapsKeyToTenantConfiguration is only referenced within its own file, so no other migrations depend on it.
- Although the AST-grep command didn’t return an explicit snippet, the pattern indicates that the OrganizationConfiguration schema includes a googleMapsKey field, so using the update path "organizationConfiguration.googleMapsKey" is valid.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (5)
30-30: LGTM! Import statement updated correctly.The import statement for
OrganizationServicehas been added as part of the tenant to organization migration.
107-107: LGTM! Field injection updated correctly.The private field has been updated to use
organizationServiceas part of the tenant to organization migration.
201-208: LGTM! Method call and logging updated correctly.The service call and error logging have been updated to use organization terminology:
- Method call changed to
getOrganizationConfiguration()- Error logging updated to reference "organization config"
- Success callback updated to use
setOrganizationConfig
687-694: LGTM! Map key updated correctly.The map key in
consolidateAPISignaturehas been updated fromtenantConfigtoorganizationConfig.
1-721: Verify the migration impact.The tenant to organization migration appears complete in this file. However, we should verify that all related components (DTOs, controllers, etc.) have been consistently updated.
Let's verify the consistency of the migration:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining "tenant" references that might have been missed rg -i "tenant" --type java # Search for new "organization" terminology to ensure consistency rg -i "organization" --type java # Search for specific patterns that might indicate inconsistency ast-grep --pattern 'class $_ { $$$ Tenant $_($$$) { $$$ } $$$ }'Length of output: 68625
Migration Verification: Business Logic Updated; Some Legacy 'Tenant' References Still Present
The consolidated API service and its related DTOs, controllers, and domain objects are now consistently using the new “organization” nomenclature in their core business logic. Our searches reveal that any remaining “tenant” references are mostly found in:
• Migration scripts and change logs (e.g. Migration065_CopyTenantIdToOrganizationId, DatabaseChangelog2).
• Test files and cache key strings (e.g. references like "tenantNewFeatures").
• Logging and span constants defined in the external interfaces (e.g. in OrganizationSpanCE, where ORGANIZATION_SPAN is still set to "tenant").These legacy occurrences appear to be either maintained for backward compatibility or are marked with TODOs for eventual cleanup.
Overall, the migration impact across key components is as expected and consistent.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserServiceImpl.java (2)
38-38: LGTM: Parameter change aligns with migration.The replacement of TenantService with OrganizationService is consistent with the tenant-to-organization migration.
56-56: Verify parameter ordering in parent class constructor.While the organizationService argument change is consistent, please verify that the parameter order matches the parent class (UserServiceCECompatibleImpl) constructor signature.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for the parent class constructor ast-grep --pattern 'class UserServiceCECompatibleImpl { $$$ UserServiceCECompatibleImpl($$$) { $$$ } $$$ }'Length of output: 121
I'll run a new shell script to locate the parent class file (UserServiceCECompatibleImpl.java) and search for its constructor signature across the codebase. This should help us verify the parameter ordering. Please see below:
Once we review the output, we can determine if the parameter ordering in UserServiceImpl matches the parent class constructor.
🏁 Script executed:
#!/bin/bash # Step 1: Locate the file defining UserServiceCECompatibleImpl echo "Searching for UserServiceCECompatibleImpl.java file..." fd -t f -e java "UserServiceCECompatibleImpl.java" # Step 2: Search for the constructor signature in the repository. echo -e "\nSearching for the constructor signature in all Java files..." rg "public UserServiceCECompatibleImpl\\s*\\(" --break -n .Length of output: 893
Let's run a new shell script to display the parent's constructor (with some context) so we can inspect the parameter ordering.
🏁 Script executed:
#!/bin/bash # Display the constructor signature along with surrounding context to review parameter ordering. rg -n -C 5 "public UserServiceCECompatibleImpl\\s*\\(" app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/UserServiceCECompatibleImpl.javaLength of output: 691
Let's now run a shell script to display a larger portion of the parent's constructor so we can verify if there is indeed a parameter for tenantService (or its replacement) and ensure the parameter ordering in the super call. This will provide the full context of the parameter list.
🏁 Script executed:
#!/bin/bash # Display a larger portion of the parent class constructor signature rg -n -C 20 "public UserServiceCECompatibleImpl\\s*\\(" app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/UserServiceCECompatibleImpl.javaLength of output: 2281
Parameter Ordering Verified
The parent class constructor in UserServiceCECompatibleImpl expects the parameters in the following order:
• Validator
• UserRepository
• WorkspaceService
• AnalyticsService
• SessionUserService
• PasswordResetTokenRepository
• PasswordEncoder
• CommonConfig
• UserDataService
• OrganizationService
• UserUtils
• EmailVerificationTokenRepository
• EmailService
• RateLimitService
• PACConfigurationService
• UserServiceHelperGiven that the child class (UserServiceImpl) now passes organizationService (replacing tenantService) in the corresponding position, the parameter ordering is consistent with the parent's constructor signature.
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/OrganizationControllerCE.java (4)
5-8: Imports look good.
They properly align with the new organization-based domain references.
22-22: Class rename aligns with Organization migration.
This matches the PR’s objective to transition from tenant to organization.
41-43: Verify authorization.
Ensure that anonymous or unprivileged users only see allowed details about the organization config.
48-51: Check admin permissions.
Before updating the organization configuration, confirm the caller has the necessary privileges.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java (9)
5-6: Successfully updating references from tenant to organization.
No issues spotted with the new imports and field references.Also applies to: 13-13, 32-32
45-45: Documentation changes are consistent.
This mention of organization-level flags is aligned with the rename.
62-62: Docs reflect combined user/org-level flags.
Looks aligned with the rebranding from tenant to org.
68-75: Confirm overriding logic.
Organization-level flags are overwriting user-level flags. Verify this is intended.
118-118: Doc updates for remote organization flags.
No concerns here.
121-124: Ensure proper security checks.
When fetching and updating organization flags, confirm that only authorized requests can trigger migrations.Also applies to: 126-126, 131-131, 133-136, 141-143, 145-145, 147-147, 149-152
158-158: Doc updates appear correct.
No issues noted.
178-179: Delegated migration logic is fine.
This clearly defers migration to the organization service.
182-183: Getter for cached features is straightforward.
No further issues.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCEImpl.java (5)
16-16: Safe import transition.
Switching to OrganizationService is in line with the PR’s aim.
53-53: New organizationService field.
Looks good.
64-64: Constructor injection updated.
No concerns with the replaced parameter.
72-72: Field assignment is correct.
All set with the assignment.
150-153: Validate matching workspace & organization.
Currently fetching users under the default organization. Ensure that the target workspace also belongs to the same org to avoid mismatch.app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java (3)
19-19: Refactor aligns with organization-based import.
154-167: Switch from Tenant to Organization.
This code consistently references “Organization” while creating the default record. The slug-based lookup is fine as long as you ensure uniqueness or handle any slug collisions in other parts of the code.
310-316: Consistent usage of 'Organization' query.
Using Organization.Fields.slug and referencing 'organization' helps avoid confusion with old tenant references.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java (7)
10-11: Imports updated for the organization model.
These changes support the new organization-based structure instead of tenant-based references.Also applies to: 34-34, 80-80
100-100: Injecting OrganizationService.
Relying on the new OrganizationService is an appropriate move away from tenant-based services.Also applies to: 131-131
157-159: Use of default organization for email lookups.
This ensures each user is tied to a known default organization. Verify behavior when no default organization is found.
163-164: Repository method refactoring.
“findByEmailAndOrganizationId” aligns with the organization-based approach.
310-313: Fallback handling for missing default organizations.
Throwing an error here maintains consistency if no default organization is configured.
335-338: Configurable strong password policy.
This is a fine approach. If more constraints are introduced, verify they also reflect the organization’s config.
682-682: Validate super-user checks.
Ensure that using userUtils::isSuperUser corresponds correctly to the new organization-based logic.app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/OrganizationController.java (2)
4-5: Refactor to use OrganizationControllerCE.
Switching from a tenant-based to an organization-based controller is consistent with the overall changes.
13-13: Unified naming from Tenant to Organization.
Renaming the controller and constructor parameters ensures clarity in the new model.Also applies to: 15-15
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/git/GitConfigCEImpl.java (1)
4-4: LGTM! Clean service migration.The migration from TenantService to OrganizationService is clean and maintains the existing behavior.
Also applies to: 13-13, 17-19
app/server/appsmith-server/src/main/java/com/appsmith/server/featureflags/FeatureFlagIdentityTraits.java (1)
21-21: LGTM! Clean field migration.The field migration from tenantId to organizationId is straightforward and appropriate for this DTO.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/FeatureFlagServiceImpl.java (1)
11-11: LGTM! Clean service injection update.The service dependency update is consistent with the migration pattern and properly propagated to the superclass.
Also applies to: 17-17
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/enums/FeatureFlagEnum.java (1)
9-9: LGTM - Test feature flag renamed appropriatelyThe renaming from TENANT_TEST_FEATURE to ORGANIZATION_TEST_FEATURE aligns with the migration objective.
app/server/appsmith-plugins/appsmithAiPlugin/src/main/java/com/external/plugins/dtos/SourceDetails.java (1)
12-12: Verify all callers of createSourceDetails methodsThe field and method changes look correct, but we should verify that all callers have been updated to use organizationId.
Also applies to: 19-19, 28-28
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining references to tenantId in relation to SourceDetails rg -l "tenantId.*SourceDetails|SourceDetails.*tenantId"Length of output: 57
I'm going to run an additional check to look for any caller invoking createSourceDetails with a tenantId parameter. This will help us verify that none of the call sites still depend on the removed tenantId.
🏁 Script executed:
#!/bin/bash # Searching for calls to createSourceDetails that include tenantId in their parameter list rg "createSourceDetails\\s*\\([^)]*tenantId" .Length of output: 46
All createSourceDetails callers correctly updated to use organizationId
The searches indicate no residual references to tenantId—neither in direct comparisons to SourceDetails nor within calls to createSourceDetails—so it appears all callers have been updated accordingly.
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/PermissionGroup.java (2)
24-26: Track TODO for data migration completionThe deprecation is properly marked, but we should track when this field can be removed.
Would you like me to create an issue to track the removal of this deprecated field after data migration?
28-28: Verify data migration script existsEnsure there's a data migration script to populate organizationId from tenantId.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for migration scripts related to tenant to organization conversion rg -l "tenant.*organization.*migration|organization.*tenant.*migration" --type java --type sqlLength of output: 221
Migration Script Verified
The migration script "Migration065_CopyTenantIdToOrganizationId.java" is present, confirming that the organizationId is being populated from tenantId.app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/UserRepositoryCE.java (1)
32-32: Verify repository implementation and service layer updatesThe method signature change looks correct. Ensure all implementations and service layer calls are updated.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining references to findByEmailAndTenantId rg "findByEmailAndTenantId" # Search for implementations of UserRepositoryCE ast-grep --pattern 'class $_ implements UserRepositoryCE { $$$ }'Length of output: 97
🏁 Script executed:
#!/bin/bash # Verify no references to the old method remain echo "Checking for 'findByEmailAndTenantId' in the repository:" rg -n "findByEmailAndTenantId" # Verify that the updated method is being used in service layer calls echo "Searching for 'findByEmailAndOrganizationId' within the service layer:" rg -n "findByEmailAndOrganizationId" app/server/appsmith-server/src/main/java/com/appsmith/server/services/Length of output: 2039
Repository and service layers are updated correctly.
- No references to the deprecated findByEmailAndTenantId were found.
- Service layer calls are using findByEmailAndOrganizationId as expected.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserDataServiceImpl.java (1)
24-24: LGTM! Constructor parameter updated consistently.The change from
TenantServicetoOrganizationServiceis properly reflected in both the constructor parameter and super constructor call.Also applies to: 36-36
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCE.java (1)
4-4: LGTM! Interface methods and documentation updated consistently.All tenant-related terminology has been properly replaced with organization-based equivalents, including method names, parameters, and documentation.
Also applies to: 29-32, 35-38, 40-40, 42-42
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/TriggerRequestDTO.java (1)
40-40: LGTM! Field updated consistently.The
organizationIdfield is properly placed alongside other ID fields, maintaining consistent field organization.app/server/appsmith-interfaces/src/main/java/com/appsmith/external/dtos/ExecuteActionDTO.java (1)
23-23: LGTM! Field updated consistently.The
organizationIdfield is properly placed alongside other ID fields (actionId, datasourceId, workspaceId, instanceId).app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCE.java (1)
22-22: Verify all implementations and callers of findByEmailAndOrganizationId.The method signature has been updated as part of the tenant to organization migration.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Find all implementations of UserServiceCE ast-grep --pattern 'class $_ implements UserServiceCE { $$$ }' # Find all calls to the old method name rg 'findByEmailAndTenantId'Length of output: 94
I'll generate additional shell scripts to search for both the new method "findByEmailAndOrganizationId" and any lingering references to "findByEmailAndTenantId" in the repository. This additional check will help confirm whether all implementations and callers have been updated appropriately.
🏁 Script executed:
#!/bin/bash echo "Searching for usages of 'findByEmailAndOrganizationId':" rg -n "findByEmailAndOrganizationId" echo "------------------------------" echo "Searching for outdated usages of 'findByEmailAndTenantId':" rg -n "findByEmailAndTenantId"Length of output: 2587
Action Required: Verify Implementation of Updated Method Signature
The shell scripts confirmed that all implementations and callers have been updated to use findByEmailAndOrganizationId, and no references to the old findByEmailAndTenantId remain.
- Confirmed usages in:
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/UserRepositoryCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserDataServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java
No occurrences of the old method signature were found.
app/server/appsmith-plugins/appsmithAiPlugin/src/main/java/com/external/plugins/utils/HeadersUtil.java (1)
40-56: Consider updating both createSourceDetailsHeader methods for consistency.While this method has been updated to use organizationId, the overloaded method for ExecuteActionDTO (lines 19-35) doesn't include organization information.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.java (3)
29-32: LGTM! Verify usage across codebase.The method renaming and field update are consistent with the tenant to organization migration.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check for any remaining references to findByEmailAndTenantId rg "findByEmailAndTenantId" -A 2Length of output: 32
I'll run a more comprehensive search to double-check for any lingering references to the legacy method name. Please hold on while we execute the script.
🏁 Script executed:
#!/bin/bash # Double-check across the repository for any references to findByEmailAndTenantId rg "findByEmailAndTenantId" .Length of output: 29
Changes Verified: Legacy method reference eliminated
The search confirmed that no lingering references to the legacy method "findByEmailAndTenantId" remain in the codebase. The renaming to "findByEmailAndOrganizationId" has been applied consistently.
29-32: LGTM! Clean migration of tenant to organization references.The method signature and query criteria updates are consistent with the migration pattern.
29-32: LGTM! Method signature and implementation correctly updated.The changes consistently reflect the migration from tenant to organization-based identification.
app/server/appsmith-plugins/appsmithAiPlugin/src/main/java/com/external/plugins/constants/AppsmithAiConstants.java (3)
35-35: LGTM!The constant renaming is consistent with the tenant to organization migration.
35-35: LGTM! Consistent with the tenant to organization migration.The constant renaming follows the migration pattern.
35-35: LGTM! Constant correctly renamed.The constant has been properly updated to reflect the organization-based terminology.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UsagePulseServiceCEImpl.java (5)
66-103: LGTM! Verify service migration completeness.The service migration from TenantService to OrganizationService is well-implemented with consistent variable renaming.
70-103: LGTM! Clean migration of tenant to organization references in the createPulse method.The changes to use organizationId are consistent and maintain the original logic.
57-58: Review and update the TODO comment about multi-tenancy.Since we're migrating from tenant to organization model, this TODO comment might be outdated:
// TODO remove this condition after multi-tenancy is introducedPlease verify if this TODO comment is still relevant after the tenant to organization migration.
✅ Verification successful
Update the TODO Comment to Reflect the New Multi-Tenancy Approach
The migration from TenantService to OrganizationService indicates that our multi-tenancy strategy has evolved. The existing TODO comment referencing the removal of the condition once multi-tenancy is introduced appears outdated. Please update it to clearly indicate:
- Whether the current condition (checking cloud hosting) is still required as part of our new organization-based multi-tenancy strategy.
- What specific changes or feature completions will allow its removal, ensuring clarity for future maintainers.
36-36: LGTM! Service dependency and method calls correctly updated.The changes from TenantService to OrganizationService are consistent and properly implemented.
Also applies to: 67-67
57-57: Review and update the TODO comment.The TODO comment about multi-tenancy should be reviewed as it may be outdated after the migration to organization-based architecture.
Please confirm if this TODO comment is still relevant:
// TODO remove this condition after multi-tenancy is introduced✅ Verification successful
Outdated TODO Comment – Action Recommended
After reviewing the context and architecture change (from multi-tenancy to organization-based), the condition mentioned no longer seems applicable. We should update or remove the comment accordingly.
• In file app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UsagePulseServiceCEImpl.java at line 57, the comment referring to multi-tenancy is outdated.
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/User.java (2)
84-85: LGTM! New field follows consistent naming.The new
organizationIdfield maintains consistent visibility with@JsonView(Views.Public.class).
79-82: Ensure data migration plan is in place.The deprecated
tenantIdfield should be removed only after ensuring all data is migrated toorganizationId.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check for any remaining references to tenantId in the codebase rg "tenantId" --type javaLength of output: 5373
Data migration plan for tenantId is in place.
- The migration script (Migration065_CopyTenantIdToOrganizationId.java) correctly implements the logic to copy tenantId to organizationId and remove tenantId afterward.
- The deprecated tenantId fields in User.java and other domain classes are intentionally left with a TODO comment until the migration process is complete.
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/UserSessionDTO.java (1)
48-48: LGTM! Consistent field renaming across methods.The migration from
tenantIdtoorganizationIdis consistently applied in the field declaration and both token handling methods.Also applies to: 88-88, 129-129
app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java (1)
126-126: LGTM! Error code renamed consistently.The error code maintains its identifier while updating the enum name to align with the organization terminology.
app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java (1)
185-185: LGTM! Constants added for organization-related fields.The new constants follow the established naming conventions and are appropriately placed in alphabetical order.
Also applies to: 206-206
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserDataServiceCEImpl.java (2)
23-23: LGTM! Service dependency properly updated.The OrganizationService is correctly injected and initialized, maintaining the established dependency injection pattern.
Also applies to: 57-57, 76-76, 84-84
108-110: Verify the migration impact on user data retrieval.The service methods have been updated to use organizationId instead of tenantId. While the changes look correct, ensure that:
- Existing user data is migrated correctly
- No data loss occurs during the transition
- All error scenarios are handled properly
Run this script to verify the usage of organization-related methods:
Also applies to: 127-130, 174-177
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining tenant references that might have been missed rg -i "tenant" --type java # Search for organization-related method usage patterns ast-grep --pattern 'findByEmailAndOrganizationId($_)'Length of output: 94982
I'm not entirely convinced by the current output – aside from the expected tenant references in migration and constant files, there’s little evidence (via ast-grep) of any remaining user data retrieval that still relies on tenantId. However, since the ast-grep command yielded only SQL file output and not the expected Java usage, we should get additional details.
I'll run another script to search for “findByEmailAndOrganizationId” in the Java code and to review the context in UserDataServiceCEImpl.java. This will help us confirm that the service now uses organizationId correctly and that related migration code is properly invoked.
🏁 Script executed:
#!/bin/bash # Search for any Java occurrences of the method findByEmailAndOrganizationId rg -i "findByEmailAndOrganizationId" --type java # Display the surrounding lines in the file where this method is used to confirm usage context (lines 100-120) sed -n '100,120p' app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserDataServiceCEImpl.javaLength of output: 3162
Migration Update Verification:
The updates in user data retrieval now reference organizationId as intended. The search confirmed that the UserDataServiceCEImpl (and related services) use organizationService.getDefaultOrganizationId() and pass the organizationId to findByEmailAndOrganizationId. The repository and service layer consistently apply this change. Additionally, migration files (e.g., Migration065_CopyTenantIdToOrganizationId.java) indicate that tenantId data is being correctly migrated—which should prevent data loss. The reactive chains, while not featuring explicit error recovery logic, follow standard error propagation and appear consistent with the overall design.Overall, the migration implementation looks correct. However, consider a manual integration test to ensure that existing records are correctly handled during the migration process.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java (5)
3-3: LGTM!Import change aligns with the migration from Tenant to Organization.
24-24: LGTM! Test coverage confirmed.Method changes look good. Based on previous learnings, similar methods have good test coverage.
20-20: Verify method references in dependent code.Method rename is consistent with the migration. Ensure all callers are updated.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining references to getDefaultTenantId rg "getDefaultTenantId"Length of output: 23
Method rename is clean—no outdated references found.
The search for "getDefaultTenantId" returned no results, confirming that all callers have been updated to use getDefaultOrganizationId.
26-26: Verify cache key updates.Method rename looks good. Ensure cache keys in the implementation are also updated to reflect the organization terminology.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining tenant-related cache keys rg -A 3 "tenant.*cache.*key" --type javaLength of output: 42
I'll perform an additional search to verify that no tenant-related cache keys remain in the codebase. Let's run a few more searches for related identifiers (like tenantId and evictCachedTenant) that might have been overlooked.
🏁 Script executed:
#!/bin/bash echo "Searching for tenantId occurrences in Java files:" rg -i "tenantId" --type java echo "Searching for evictCachedTenant references in Java files:" rg -i "evictCachedTenant" --type java echo "Searching for tenant-related cache key phrases in Java files:" rg -i "tenant.*cache" --type javaLength of output: 7353
Let's check the implementation file to see if the cache keys have been updated accordingly. I'm going to search for any "tenant" references versus "organization" references within CacheableRepositoryHelperCE.java.
🏁 Script executed:
#!/bin/bash echo "Displaying relevant parts of CacheableRepositoryHelperCE.java (lines 1-100):" sed -n '1,100p' app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java echo "Searching for 'tenant' keyword in CacheableRepositoryHelperCE.java:" rg -i "tenant" app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java echo "Searching for 'organization' keyword in CacheableRepositoryHelperCE.java:" rg -i "organization" app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.javaLength of output: 3575
Cache Key Update Verified
The repository’s CacheableRepositoryHelperCE interface now consistently uses organization terminology. All cache-related methods—including evictCachedOrganization—as well as associated keys appear to have been updated appropriately, with no remaining tenant references in this context.
18-18: Verify parameter usage in implementations.The parameter rename looks good, but ensure all implementations and callers are updated accordingly.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining references to tenantId in method implementations ast-grep --pattern 'evictPermissionGroupsUser($email, $tenantId)'Length of output: 67
I'll run an additional script to search for any invocations of evictPermissionGroupsUser that include "tenantId" instead of "organizationId." This will help confirm that all callers have been updated.
🏁 Script executed:
#!/bin/bash # Search for any calls to evictPermissionGroupsUser that reference "tenantId" in their parameters. rg "evictPermissionGroupsUser\([^)]*tenantId" .Length of output: 49
All checks indicate that the parameter rename has been fully applied—no references to "tenantId" remain in either the method declaration or its call sites.
Parameter verification complete: The renaming to "organizationId" is consistently updated in both implementations and callers.
| sessionStr.substring(sessionStr.indexOf('{'), sessionStr.lastIndexOf('}') + 1); | ||
| JsonNode jsonNode = objectMapper.readTree(jsonStr); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve substring-based JSON extraction.
Using substring with fixed braces can break if multiple or malformed JSON objects appear in the session string. Consider safer parsing strategies to avoid substring out-of-bounds or unexpected JSON structures.
| @Override | ||
| public void onApplicationEvent(ApplicationStartedEvent event) { | ||
| cleanupAndUpdateRefreshDefaultOrganizationPolicies().block(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to the event handler.
The event handler uses a blocking call without error handling, which could lead to silent failures during application startup.
@Override
public void onApplicationEvent(ApplicationStartedEvent event) {
- cleanupAndUpdateRefreshDefaultOrganizationPolicies().block();
+ try {
+ cleanupAndUpdateRefreshDefaultOrganizationPolicies()
+ .doOnError(error -> log.error("Failed to update organization policies", error))
+ .block();
+ } catch (Exception e) {
+ log.error("Critical error during organization policies update", e);
+ // Consider if application should fail to start in this case
+ throw e;
+ }
}📝 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.
| @Override | |
| public void onApplicationEvent(ApplicationStartedEvent event) { | |
| cleanupAndUpdateRefreshDefaultOrganizationPolicies().block(); | |
| } | |
| @Override | |
| public void onApplicationEvent(ApplicationStartedEvent event) { | |
| try { | |
| cleanupAndUpdateRefreshDefaultOrganizationPolicies() | |
| .doOnError(error -> log.error("Failed to update organization policies", error)) | |
| .block(); | |
| } catch (Exception e) { | |
| log.error("Critical error during organization policies update", e); | |
| // Consider if application should fail to start in this case | |
| throw e; | |
| } | |
| } |
| // TODO: Convert to current organization when the feature is enabled | ||
| Mono<Organization> organizationMono = organizationRepository.findBySlug(DEFAULT); | ||
| return Mono.zip(instanceIdMono, organizationMono, getUserDefaultTraits(user)) | ||
| .flatMap(objects -> { | ||
| String tenantId = objects.getT2().getId(); | ||
| String organizationId = objects.getT2().getId(); | ||
| String appsmithVersion = releaseNotesService.getRunningVersion(); | ||
| FeatureFlagIdentityTraits featureFlagIdentityTraits = new FeatureFlagIdentityTraits( | ||
| objects.getT1(), tenantId, Set.of(userIdentifier), objects.getT3(), appsmithVersion); | ||
| objects.getT1(), organizationId, Set.of(userIdentifier), objects.getT3(), appsmithVersion); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle missing default organization.
Using DEFAULT for the organization slug could cause a null result if no such organization exists. Consider adding a fallback or an error path instead of assuming existence. The TODO comment further signals a future refactor, so let's plan.
Would you like me to open a new issue to handle fallback logic for missing default organizations?
| public Mono<String> getDefaultOrganizationId() { | ||
|
|
||
| // If the value exists in cache, return it as is | ||
| if (StringUtils.hasLength(organizationId)) { | ||
| return Mono.just(organizationId); | ||
| } | ||
| return repository.findBySlug(FieldName.DEFAULT).map(Organization::getId).map(organizationId -> { | ||
| // Set the cache value before returning. | ||
| this.organizationId = organizationId; | ||
| return organizationId; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential concurrency issue with cached organization ID.
The field "organizationId" is stored in the service instance and is updated in getDefaultOrganizationId(). Concurrent or reactive calls could lead to race conditions. Consider using a thread-safe caching mechanism or an external cache.
| @Override | ||
| public Mono<Organization> save(Organization organization) { | ||
| Mono<Void> evictCachedOrganization = cacheableRepositoryHelper.evictCachedOrganization(organizationId); | ||
| Mono<Organization> savedOrganizationMono = repository.save(organization).cache(); | ||
| return savedOrganizationMono | ||
| .then(Mono.defer(() -> evictCachedOrganization)) | ||
| .then(savedOrganizationMono); | ||
| } |
There was a problem hiding this comment.
Evicting the default organization cache instead of the saved record.
The “save” method calls evictCachedOrganization(organizationId) using a private field rather than the freshly saved organization’s ID. If you intend to evict the cache of the saved organization, use organization.getId().
- Mono<Void> evictCachedOrganization = cacheableRepositoryHelper.evictCachedOrganization(organizationId);
+ Mono<Void> evictCachedOrganization = cacheableRepositoryHelper.evictCachedOrganization(organization.getId());📝 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.
| @Override | |
| public Mono<Organization> save(Organization organization) { | |
| Mono<Void> evictCachedOrganization = cacheableRepositoryHelper.evictCachedOrganization(organizationId); | |
| Mono<Organization> savedOrganizationMono = repository.save(organization).cache(); | |
| return savedOrganizationMono | |
| .then(Mono.defer(() -> evictCachedOrganization)) | |
| .then(savedOrganizationMono); | |
| } | |
| @Override | |
| public Mono<Organization> save(Organization organization) { | |
| - Mono<Void> evictCachedOrganization = cacheableRepositoryHelper.evictCachedOrganization(organizationId); | |
| + Mono<Void> evictCachedOrganization = cacheableRepositoryHelper.evictCachedOrganization(organization.getId()); | |
| Mono<Organization> savedOrganizationMono = repository.save(organization).cache(); | |
| return savedOrganizationMono | |
| .then(Mono.defer(() -> evictCachedOrganization)) | |
| .then(savedOrganizationMono); | |
| } |
| OrganizationConfiguration defaultTenantConfiguration = new OrganizationConfiguration(); | ||
| if (defaultOrganization == null) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Resolve naming mismatch and error message references to "tenant".
The variable name and exception message should consistently refer to "Organization" rather than "Tenant".
Proposed diff:
- OrganizationConfiguration defaultTenantConfiguration = new OrganizationConfiguration();
- if (defaultOrganization == null) {
- throw new IllegalStateException("Default tenant not found");
- }
+ OrganizationConfiguration defaultOrganizationConfiguration = new OrganizationConfiguration();
+ if (defaultOrganization == null) {
+ throw new IllegalStateException("Default organization not found");
+ }📝 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.
| OrganizationConfiguration defaultTenantConfiguration = new OrganizationConfiguration(); | |
| if (defaultOrganization == null) { | |
| OrganizationConfiguration defaultOrganizationConfiguration = new OrganizationConfiguration(); | |
| if (defaultOrganization == null) { | |
| throw new IllegalStateException("Default organization not found"); | |
| } |
| OrganizationConfiguration defaultTenantConfiguration = new OrganizationConfiguration(); | ||
| if (Objects.nonNull(defaultOrganization.getOrganizationConfiguration())) { | ||
| defaultTenantConfiguration = defaultOrganization.getOrganizationConfiguration(); | ||
| } | ||
| defaultTenantConfiguration.setInstanceName(instanceName); | ||
| defaultTenant.setTenantConfiguration(defaultTenantConfiguration); | ||
| mongoTemplate.save(defaultTenant); | ||
| defaultOrganization.setOrganizationConfiguration(defaultTenantConfiguration); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize variable naming to use Organization terminology.
The variable names still use "tenant" terminology while working with Organization objects.
-OrganizationConfiguration defaultTenantConfiguration = new OrganizationConfiguration();
+OrganizationConfiguration defaultOrgConfiguration = new OrganizationConfiguration();
if (Objects.nonNull(defaultOrganization.getOrganizationConfiguration())) {
- defaultTenantConfiguration = defaultOrganization.getOrganizationConfiguration();
+ defaultOrgConfiguration = defaultOrganization.getOrganizationConfiguration();
}
-defaultTenantConfiguration.setInstanceName(instanceName);
-defaultOrganization.setOrganizationConfiguration(defaultTenantConfiguration);
+defaultOrgConfiguration.setInstanceName(instanceName);
+defaultOrganization.setOrganizationConfiguration(defaultOrgConfiguration);
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java (2)
451-504: Add error handling and transaction management.The super user update logic could benefit from:
- Error handling for missing environment variables
- Transaction management to ensure atomic updates
Consider wrapping the updates in a transaction:
@ChangeSet(order = "10000", id = "update-super-users", author = "", runAlways = true) public void updateSuperUsers(...) { + String adminEmailsStr = Optional.ofNullable(System.getenv(String.valueOf(APPSMITH_ADMIN_EMAILS))) + .orElseThrow(() -> new IllegalStateException("APPSMITH_ADMIN_EMAILS environment variable is required")); + mongoTemplate.executeInTransaction(status -> { // Existing code here + return null; + }); }
201-223: Add validation and error handling for tenant existence.The anonymous user creation logic should validate tenant existence and handle potential errors.
Consider adding validation:
@ChangeSet(order = "023", id = "add-anonymousUser", author = "") public void addAnonymousUser(MongoTemplate mongoTemplate) { Query tenantQuery = new Query(); tenantQuery.addCriteria(where(Tenant.Fields.slug).is("default")); Tenant tenant = mongoTemplate.findOne(tenantQuery, Tenant.class); + if (tenant == null) { + throw new IllegalStateException("Default tenant not found"); + } // Rest of the code }
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java (1)
225-293: Add documentation for permission setup.The instance configuration and permission group setup is complex. Consider adding detailed documentation explaining the permission model and its implications.
Add documentation:
+/** + * Sets up instance configuration and creates associated permission groups. + * + * Permission Model: + * - Instance Manager Permission Group: Controls instance-wide configurations + * - Permissions: + * - MANAGE_INSTANCE_CONFIGURATION: Full control over instance settings + * - READ_INSTANCE_CONFIGURATION: Read-only access to instance settings + */ @ChangeSet(order = "029", id = "add-instance-config-object", author = "") public void addInstanceConfigurationPlaceHolder(MongoTemplate mongoTemplate) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Tenant.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/domains/TenantConfiguration.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/TenantConfigurationCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/InstanceConfigHelperCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration059PolicySetToPolicyMap.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration061TenantPolicySetToPolicyMap.java(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Tenant.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/TenantConfigurationCE.java
🚧 Files skipped from review as they are similar to previous changes (4)
- app/server/appsmith-server/src/main/java/com/appsmith/server/domains/TenantConfiguration.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration059PolicySetToPolicyMap.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration061TenantPolicySetToPolicyMap.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/InstanceConfigHelperCEImpl.java
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-lint / client-lint
- GitHub Check: server-unit-tests / server-unit-tests
| Query organizationQuery = new Query(); | ||
| organizationQuery.addCriteria(where(Organization.Fields.slug).is("default")); | ||
| Organization organization = mongoTemplate.findOne(organizationQuery, Organization.class); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider completing the tenant to organization transition.
While these lines have been updated to use Organization, there are still many references to Tenant throughout the code. This partial transition could lead to confusion.
Let's verify the remaining tenant references:
🏁 Script executed:
#!/bin/bash
# Search for remaining Tenant references
rg -i "tenant" --type javaLength of output: 35609
Finish Tenant to Organization Transition Consistently
The search confirms that many Tenant references remain throughout the codebase, including in migration classes like DatabaseChangelog2.java. This partial transition can lead to confusion and potential issues. It appears that the migration logic is still using Tenant in several places (e.g., queries based on Tenant.Fields.slug, tenant-specific configuration, and index definitions).
- Multiple migration scripts and domain classes still reference
Tenant. - The changes in lines 476-479 of
DatabaseChangelog2.javaare not applied uniformly across the codebase.
It is advisable to either complete the transition to the Organization model or clearly isolate the legacy Tenant functionality to avoid ambiguity.
Failed server tests
|
…nsure that its always executed post all the other migrations
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration10000_UpdateSuperUser.java (1)
68-71: Add null checks for Config object.
instanceAdminConfigurationmay benullif theFieldName.INSTANCE_CONFIGrecord doesn't exist in the database, potentially causing a NullPointerException on line 71.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration10000_UpdateSuperUser.java(1 hunks)
💤 Files with no reviewable changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-lint / client-lint
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration10000_UpdateSuperUser.java (3)
62-63: Handle unset or empty environment variable.If
adminEmailsStrisnullor empty, the migration logic proceeds without error handling. Verify thatTextUtils.csvToSet(null)returns an empty set as intended, or consider adding explicit null checks.
80-82: Verify the presence of default organization.If the "default" organization record is not found,
organizationbecomesnull, leading to issues in subsequent user creation. Consider logging or gracefully handling this case.
92-96: Confirm safe handling of user creation logic.When creating new super users, ensure that the data (email, organization, permission group) is valid and consistent with the rest of the system. Incorrect values may lead to partial user records.
| @RollbackExecution | ||
| public void rollbackExecution() {} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provide a rollback plan.
The rollbackExecution method is empty, meaning there's no way to revert changes if something goes wrong. Consider implementing a rollback strategy for safer migrations.
Failed server tests
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration10000_UpdateSuperUser.java (1)
44-54: Inject UpdateSuperUserMigrationHelper for better testability.Instead of instantiating
UpdateSuperUserMigrationHelperin the constructor, inject it as a dependency. This improves testability by allowing mock injection during tests.public Migration10000_UpdateSuperUser( MongoTemplate mongoTemplate, CacheableRepositoryHelper cacheableRepositoryHelper, PolicySolution policySolution, - PolicyGenerator policyGenerator) { + PolicyGenerator policyGenerator, + UpdateSuperUserMigrationHelper updateSuperUserMigrationHelper) { this.mongoTemplate = mongoTemplate; this.cacheableRepositoryHelper = cacheableRepositoryHelper; this.policySolution = policySolution; this.policyGenerator = policyGenerator; - this.updateSuperUserMigrationHelper = new UpdateSuperUserMigrationHelper(); + this.updateSuperUserMigrationHelper = updateSuperUserMigrationHelper; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration10000_UpdateSuperUser.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/solutions/ce/UpdateSuperUserMigrationHelperCE.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/solutions/ce/UpdateSuperUserMigrationHelperCE.java
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration10000_UpdateSuperUser.java (2)
34-35: Consider the implications ofrunAlways=true.Setting
runAlways=truemeans this migration will execute on every deployment, which could impact performance due to repeated cache invalidations and database operations. Consider if this is really necessary or if it should only run once.
56-57: Provide a rollback plan.The
rollbackExecutionmethod is empty, meaning there's no way to revert changes if something goes wrong. Consider implementing a rollback strategy for safer migrations.
| String adminEmailsStr = System.getenv(String.valueOf(APPSMITH_ADMIN_EMAILS)); | ||
|
|
||
| Set<String> adminEmails = TextUtils.csvToSet(adminEmailsStr); |
There was a problem hiding this comment.
Add validation and error handling for admin emails.
The code should validate:
- That APPSMITH_ADMIN_EMAILS environment variable exists
- That the email formats are valid
-String adminEmailsStr = System.getenv(String.valueOf(APPSMITH_ADMIN_EMAILS));
+String adminEmailsStr = Optional.ofNullable(System.getenv(String.valueOf(APPSMITH_ADMIN_EMAILS)))
+ .orElseThrow(() -> new IllegalStateException("APPSMITH_ADMIN_EMAILS environment variable is required"));
Set<String> adminEmails = TextUtils.csvToSet(adminEmailsStr);
+adminEmails.forEach(email -> {
+ if (!email.matches("^[A-Za-z0-9+_.-]+@(.+)$")) {
+ throw new IllegalArgumentException("Invalid email format: " + email);
+ }
+});📝 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.
| String adminEmailsStr = System.getenv(String.valueOf(APPSMITH_ADMIN_EMAILS)); | |
| Set<String> adminEmails = TextUtils.csvToSet(adminEmailsStr); | |
| String adminEmailsStr = Optional.ofNullable(System.getenv(String.valueOf(APPSMITH_ADMIN_EMAILS))) | |
| .orElseThrow(() -> new IllegalStateException("APPSMITH_ADMIN_EMAILS environment variable is required")); | |
| Set<String> adminEmails = TextUtils.csvToSet(adminEmailsStr); | |
| adminEmails.forEach(email -> { | |
| if (!email.matches("^[A-Za-z0-9+_.-]+@(.+)$")) { | |
| throw new IllegalArgumentException("Invalid email format: " + email); | |
| } | |
| }); |
| @Execution | ||
| public void executeMigration() { | ||
| // Read the admin emails from the environment and update the super users accordingly | ||
| String adminEmailsStr = System.getenv(String.valueOf(APPSMITH_ADMIN_EMAILS)); | ||
|
|
||
| Set<String> adminEmails = TextUtils.csvToSet(adminEmailsStr); | ||
|
|
||
| Query instanceConfigurationQuery = new Query(); | ||
| instanceConfigurationQuery.addCriteria(where(Config.Fields.name).is(FieldName.INSTANCE_CONFIG)); | ||
| Config instanceAdminConfiguration = mongoTemplate.findOne(instanceConfigurationQuery, Config.class); | ||
|
|
||
| String instanceAdminPermissionGroupId = | ||
| (String) instanceAdminConfiguration.getConfig().get(DEFAULT_PERMISSION_GROUP); | ||
|
|
||
| Query permissionGroupQuery = new Query(); | ||
| permissionGroupQuery | ||
| .addCriteria(where(PermissionGroup.Fields.id).is(instanceAdminPermissionGroupId)) | ||
| .fields() | ||
| .include(PermissionGroup.Fields.assignedToUserIds); | ||
| PermissionGroup instanceAdminPG = mongoTemplate.findOne(permissionGroupQuery, PermissionGroup.class); | ||
|
|
||
| Query organizationQuery = new Query(); | ||
| organizationQuery.addCriteria(where(Organization.Fields.slug).is("default")); | ||
| Organization organization = mongoTemplate.findOne(organizationQuery, Organization.class); | ||
|
|
||
| Set<String> userIds = adminEmails.stream() | ||
| .map(email -> email.trim()) | ||
| .map(String::toLowerCase) | ||
| .map(email -> { | ||
| Query userQuery = new Query(); | ||
| userQuery.addCriteria(where(User.Fields.email).is(email)); | ||
| User user = mongoTemplate.findOne(userQuery, User.class); | ||
|
|
||
| if (user == null) { | ||
| log.info("Creating super user with username {}", email); | ||
| user = updateSuperUserMigrationHelper.createNewUser( | ||
| email, organization, instanceAdminPG, mongoTemplate, policySolution, policyGenerator); | ||
| } | ||
|
|
||
| return user.getId(); | ||
| }) | ||
| .collect(Collectors.toSet()); | ||
|
|
||
| Set<String> oldSuperUsers = instanceAdminPG.getAssignedToUserIds(); | ||
| Set<String> updatedUserIds = findSymmetricDiff(oldSuperUsers, userIds); | ||
| evictPermissionCacheForUsers(updatedUserIds, mongoTemplate, cacheableRepositoryHelper); | ||
|
|
||
| Update update = new Update().set(PermissionGroup.Fields.assignedToUserIds, userIds); | ||
| mongoTemplate.updateFirst(permissionGroupQuery, update, PermissionGroup.class); | ||
|
|
||
| // Assign all super users to the default role | ||
| updateSuperUserMigrationHelper.assignAllSuperUsersToDefaultRole( | ||
| userIds, mongoTemplate, cacheableRepositoryHelper); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add transaction management for database operations.
The migration performs multiple database operations without transaction management. Consider wrapping the operations in a transaction to ensure data consistency.
@Execution
-public void executeMigration() {
+@Transactional
+public void executeMigration() {
// Rest of the code...📝 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.
| @Execution | |
| public void executeMigration() { | |
| // Read the admin emails from the environment and update the super users accordingly | |
| String adminEmailsStr = System.getenv(String.valueOf(APPSMITH_ADMIN_EMAILS)); | |
| Set<String> adminEmails = TextUtils.csvToSet(adminEmailsStr); | |
| Query instanceConfigurationQuery = new Query(); | |
| instanceConfigurationQuery.addCriteria(where(Config.Fields.name).is(FieldName.INSTANCE_CONFIG)); | |
| Config instanceAdminConfiguration = mongoTemplate.findOne(instanceConfigurationQuery, Config.class); | |
| String instanceAdminPermissionGroupId = | |
| (String) instanceAdminConfiguration.getConfig().get(DEFAULT_PERMISSION_GROUP); | |
| Query permissionGroupQuery = new Query(); | |
| permissionGroupQuery | |
| .addCriteria(where(PermissionGroup.Fields.id).is(instanceAdminPermissionGroupId)) | |
| .fields() | |
| .include(PermissionGroup.Fields.assignedToUserIds); | |
| PermissionGroup instanceAdminPG = mongoTemplate.findOne(permissionGroupQuery, PermissionGroup.class); | |
| Query organizationQuery = new Query(); | |
| organizationQuery.addCriteria(where(Organization.Fields.slug).is("default")); | |
| Organization organization = mongoTemplate.findOne(organizationQuery, Organization.class); | |
| Set<String> userIds = adminEmails.stream() | |
| .map(email -> email.trim()) | |
| .map(String::toLowerCase) | |
| .map(email -> { | |
| Query userQuery = new Query(); | |
| userQuery.addCriteria(where(User.Fields.email).is(email)); | |
| User user = mongoTemplate.findOne(userQuery, User.class); | |
| if (user == null) { | |
| log.info("Creating super user with username {}", email); | |
| user = updateSuperUserMigrationHelper.createNewUser( | |
| email, organization, instanceAdminPG, mongoTemplate, policySolution, policyGenerator); | |
| } | |
| return user.getId(); | |
| }) | |
| .collect(Collectors.toSet()); | |
| Set<String> oldSuperUsers = instanceAdminPG.getAssignedToUserIds(); | |
| Set<String> updatedUserIds = findSymmetricDiff(oldSuperUsers, userIds); | |
| evictPermissionCacheForUsers(updatedUserIds, mongoTemplate, cacheableRepositoryHelper); | |
| Update update = new Update().set(PermissionGroup.Fields.assignedToUserIds, userIds); | |
| mongoTemplate.updateFirst(permissionGroupQuery, update, PermissionGroup.class); | |
| // Assign all super users to the default role | |
| updateSuperUserMigrationHelper.assignAllSuperUsersToDefaultRole( | |
| userIds, mongoTemplate, cacheableRepositoryHelper); | |
| } | |
| @Execution | |
| @Transactional | |
| public void executeMigration() { | |
| // Read the admin emails from the environment and update the super users accordingly | |
| String adminEmailsStr = System.getenv(String.valueOf(APPSMITH_ADMIN_EMAILS)); | |
| Set<String> adminEmails = TextUtils.csvToSet(adminEmailsStr); | |
| Query instanceConfigurationQuery = new Query(); | |
| instanceConfigurationQuery.addCriteria(where(Config.Fields.name).is(FieldName.INSTANCE_CONFIG)); | |
| Config instanceAdminConfiguration = mongoTemplate.findOne(instanceConfigurationQuery, Config.class); | |
| String instanceAdminPermissionGroupId = | |
| (String) instanceAdminConfiguration.getConfig().get(DEFAULT_PERMISSION_GROUP); | |
| Query permissionGroupQuery = new Query(); | |
| permissionGroupQuery | |
| .addCriteria(where(PermissionGroup.Fields.id).is(instanceAdminPermissionGroupId)) | |
| .fields() | |
| .include(PermissionGroup.Fields.assignedToUserIds); | |
| PermissionGroup instanceAdminPG = mongoTemplate.findOne(permissionGroupQuery, PermissionGroup.class); | |
| Query organizationQuery = new Query(); | |
| organizationQuery.addCriteria(where(Organization.Fields.slug).is("default")); | |
| Organization organization = mongoTemplate.findOne(organizationQuery, Organization.class); | |
| Set<String> userIds = adminEmails.stream() | |
| .map(email -> email.trim()) | |
| .map(String::toLowerCase) | |
| .map(email -> { | |
| Query userQuery = new Query(); | |
| userQuery.addCriteria(where(User.Fields.email).is(email)); | |
| User user = mongoTemplate.findOne(userQuery, User.class); | |
| if (user == null) { | |
| log.info("Creating super user with username {}", email); | |
| user = updateSuperUserMigrationHelper.createNewUser( | |
| email, organization, instanceAdminPG, mongoTemplate, policySolution, policyGenerator); | |
| } | |
| return user.getId(); | |
| }) | |
| .collect(Collectors.toSet()); | |
| Set<String> oldSuperUsers = instanceAdminPG.getAssignedToUserIds(); | |
| Set<String> updatedUserIds = findSymmetricDiff(oldSuperUsers, userIds); | |
| evictPermissionCacheForUsers(updatedUserIds, mongoTemplate, cacheableRepositoryHelper); | |
| Update update = new Update().set(PermissionGroup.Fields.assignedToUserIds, userIds); | |
| mongoTemplate.updateFirst(permissionGroupQuery, update, PermissionGroup.class); | |
| // Assign all super users to the default role | |
| updateSuperUserMigrationHelper.assignAllSuperUsersToDefaultRole( | |
| userIds, mongoTemplate, cacheableRepositoryHelper); | |
| } |
Failed server tests
|
…o follow in another commit which also tackles migration
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java (3)
42-42:⚠️ Potential issueConsider using AtomicReference for thread-safe caching.
The field
organizationIdis stored in the service instance and updated ingetDefaultOrganizationId(). Concurrent or reactive calls could lead to race conditions.Apply this diff to make the caching thread-safe:
- private String organizationId = null; + private final AtomicReference<String> organizationId = new AtomicReference<>();
253-260:⚠️ Potential issueFix cache eviction in save operation.
The cache eviction is using the class field
organizationIdinstead of the saved organization's ID.Apply this diff to fix the cache eviction:
public Mono<Organization> save(Organization organization) { - Mono<Void> evictCachedOrganization = cacheableRepositoryHelper.evictCachedOrganization(organizationId); Mono<Organization> savedOrganizationMono = repository.save(organization).cache(); + Mono<Void> evictCachedOrganization = savedOrganizationMono + .map(Organization::getId) + .flatMap(cacheableRepositoryHelper::evictCachedOrganization); return savedOrganizationMono .then(Mono.defer(() -> evictCachedOrganization)) .then(savedOrganizationMono); }
75-86: 🛠️ Refactor suggestionUpdate cache operations to use AtomicReference.
The caching logic needs to be updated to use atomic operations for thread safety.
Apply this diff to update the caching logic:
public Mono<String> getDefaultOrganizationId() { - if (StringUtils.hasLength(organizationId)) { - return Mono.just(organizationId); + String cachedId = organizationId.get(); + if (StringUtils.hasLength(cachedId)) { + return Mono.just(cachedId); } return repository.findBySlug(FieldName.DEFAULT).map(Organization::getId).map(organizationId -> { - this.organizationId = organizationId; + this.organizationId.set(organizationId); return organizationId; }); }
🧹 Nitpick comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java (3)
104-112: Improve SMTP validation logic.The SMTP validation can be improved by checking for empty strings using StringUtils.
Apply this diff to enhance the validation:
- if (mailHost == null || mailHost == "") { + if (!StringUtils.hasText(mailHost)) { return Mono.error(new AppsmithException(AppsmithError.INVALID_SMTP_CONFIGURATION)); }
268-298: Consider adding retry mechanism for feature flag migrations.The feature flag migration process could benefit from a retry mechanism to handle transient failures.
Add retry logic to handle transient failures:
public Mono<Organization> checkAndExecuteMigrationsForOrganizationFeatureFlags(Organization organization) { if (!isMigrationRequired(organization)) { return Mono.just(organization); } Map<FeatureFlagEnum, FeatureMigrationType> featureMigrationTypeMap = organization.getOrganizationConfiguration().getFeaturesWithPendingMigration(); FeatureFlagEnum featureFlagEnum = featureMigrationTypeMap.keySet().stream().findFirst().orElse(null); return featureFlagMigrationHelper .checkAndExecuteMigrationsForFeatureFlag(organization, featureFlagEnum) + .retryWhen(Retry.backoff(3, Duration.ofSeconds(1)) + .filter(throwable -> throwable instanceof TransientException)) .flatMap(isSuccessful -> {
331-346: Add logging for organization restart process.The restart process could benefit from more detailed logging to track the restart flow.
Add more detailed logging:
public Mono<Void> restartOrganization() { + log.info("Starting organization restart process"); Mono<Organization> defaultOrganizationMono = this.getDefaultOrganizationId().flatMap(this::retrieveById); return defaultOrganizationMono.flatMap(updatedOrganization -> { if (TRUE.equals(updatedOrganization.getOrganizationConfiguration().getIsRestartRequired())) { - log.debug("Triggering organization restart after the feature flag migrations are executed"); + log.info("Triggering organization restart for organization: {}", updatedOrganization.getId()); OrganizationConfiguration organizationConfiguration = updatedOrganization.getOrganizationConfiguration(); organizationConfiguration.setIsRestartRequired(false); return this.update(updatedOrganization.getId(), updatedOrganization) .then(envManager.restartWithoutAclCheck()); } + log.debug("No restart required for organization: {}", updatedOrganization.getId()); return Mono.empty(); }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/acl/AclPermission.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/acl/AppsmithRole.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/acl/AclPermission.java
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: server-unit-tests / server-unit-tests
| @@ -62,7 +62,7 @@ public enum AppsmithRole { | |||
| WORKSPACE_READ_APPLICATIONS, | |||
| WORKSPACE_INVITE_USERS, | |||
| WORKSPACE_EXECUTE_DATASOURCES)), | |||
| TENANT_ADMIN("", "", Set.of(MANAGE_TENANT)), | |||
| TENANT_ADMIN("", "", Set.of(MANAGE_ORGANIZATION)), | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename TENANT_ADMIN to ORGANIZATION_ADMIN and add proper documentation.
The role name should be updated to maintain consistency with the organization terminology. Additionally, meaningful name and description should be provided instead of empty strings.
- TENANT_ADMIN("", "", Set.of(MANAGE_ORGANIZATION)),
+ /**
+ * Administrator role for organization-wide management.
+ * Has permissions to manage organization settings and configurations.
+ */
+ ORGANIZATION_ADMIN(
+ "Organization Administrator",
+ "Can manage organization-wide settings and configurations",
+ Set.of(MANAGE_ORGANIZATION)),📝 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.
| TENANT_ADMIN("", "", Set.of(MANAGE_ORGANIZATION)), | |
| /** | |
| * Administrator role for organization-wide management. | |
| * Has permissions to manage organization settings and configurations. | |
| */ | |
| ORGANIZATION_ADMIN( | |
| "Organization Administrator", | |
| "Can manage organization-wide settings and configurations", | |
| Set.of(MANAGE_ORGANIZATION)), |
…ew permission names
… still readable in the code
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
app/client/cypress/e2e/Regression/ClientSide/Branding/Branding_spec.js (2)
12-36: 🛠️ Refactor suggestionRefactor selectors to use data- attributes*
Per coding guidelines, replace CSS selectors with data-* attributes for better maintainability and reliability.
Examples to implement:
- LeftPaneBrandingLink: ".t--settings-category-branding", + LeftPaneBrandingLink: "[data-testid=settings-category-branding]", - submitButton: "button[type='submit']", + submitButton: "[data-testid=submit-button]",
136-136: 🛠️ Refactor suggestionRemove hardcoded waits
Avoid using
cy.wait()with arbitrary timeouts. Instead, use explicit waiting for elements or network requests.Replace with:
- cy.wait(2000); + cy.waitUntil(() => { + return cy.get(locators.AdminSettingsColorInput).should('be.visible'); + });Also applies to: 155-155
🧹 Nitpick comments (1)
app/client/cypress/e2e/Regression/ClientSide/Branding/Branding_spec.js (1)
78-81: Avoid using force: true in file selectionsUsing
force: truecan mask underlying issues. Ensure elements are properly visible and interactable.Consider handling the element visibility explicitly:
- { force: true } + cy.get(locators.AdmingSettingsLogoInput).should('exist').and('not.be.disabled')Also applies to: 95-98
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/e2e/Regression/ClientSide/Branding/Branding_spec.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`app/client/cypress/**/**.*`: Review the following e2e test ...
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/e2e/Regression/ClientSide/Branding/Branding_spec.js
🔇 Additional comments (1)
app/client/cypress/e2e/Regression/ClientSide/Branding/Branding_spec.js (1)
38-315: LGTM! Test structure follows best practicesThe test suite demonstrates good practices with:
- Multiple assertions in expect statements
- Proper test isolation
- Clear test descriptions
| long updatedCount = mongoTemplate | ||
| .getCollection(mongoTemplate.getCollectionName(domainClass)) | ||
| .updateMany(query.getQueryObject(), pipeline) | ||
| .getModifiedCount(); |
There was a problem hiding this comment.
Do you think it will be a good idea to batch the updates, to reduce memory usage?
There was a problem hiding this comment.
@abhvsn : From what I understand, updateMany underlying implementation uses batching (mongo db level) and is the most efficient way to update documents. Do you have suggestions for improvement?
There was a problem hiding this comment.
I think that's bulkWrite not updateMany, can you share if there is any reference. Otherwise for batched process I was thinking breaking the query in 2 DB calls:
- Find (batching will be done for this)
- updateMany
This will increase the time considerable as we are hitting the DB couple of time only if the collection is bigger, which is what we are trying to optimise the memory footprint here.
| "organizationId", | ||
| objectNode.get("tenantId").asText()); | ||
| // Remove tenantId | ||
| objectNode.remove("tenantId"); |
There was a problem hiding this comment.
Will there be any impact if older pods remove the cache because of the de-serialisation exception while migration is going on?
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java (1)
200-200:⚠️ Potential issueRemove redundant
TENANT_IDconstant.As part of the tenant to organization migration,
TENANT_IDshould be removed sinceORGANIZATION_IDis its replacement.Apply this diff:
- public static final String TENANT_ID = "tenantId";Also applies to: 208-208
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (1)
266-278: 🛠️ Refactor suggestionUpdate metadata key constant for consistency.
While the method now correctly uses
organizationService, it still uses theTENANT_IDconstant for the metadata key. This should be updated to maintain consistency with the tenant to organization migration.Let's verify if there's a corresponding organization constant:
#!/bin/bash # Search for organization-related constants that could replace TENANT_ID rg -A 5 'ORGANIZATION_ID|ORG_ID' app/server/appsmith-server/src/main/java/com/appsmith/server/constants/
🧹 Nitpick comments (2)
app/client/src/ce/reducers/index.tsx (1)
186-186: Consider removing theanytype.The
anytype should be replaced with a more specific type to improve type safety.- organization: OrganizationReduxState<any>; + organization: OrganizationReduxState<unknown>;app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java (1)
186-186: Group related organization constants together.For better maintainability, keep related constants together.
Apply this diff to group the organization-related constants:
- public static final String ORGANIZATION = "organization"; - public static final String SUFFIX_USER_MANAGEMENT_ROLE = " User Management"; - public static final String REPO_URL = "repoUrl"; public static final String REPO_NAME = "repoName"; public static final String GIT_HOSTING_PROVIDER = "gitHostingProvider"; public static final String IS_MERGEABLE = "isMergeable"; public static final String FILE_LOCK_DURATION = "fileLockDuration"; public static final String REMOTE_PLUGINS = "remotePlugins"; public static final String INSTANCE_ID = "instanceId"; public static final String TENANT_ID = "tenantId"; public static final String IP_ADDRESS = "ipAddress"; public static final String VERSION = "version"; public static final String PUBLISHED = "published"; public static final String UNPUBLISHED = "unpublished"; public static final String ARTIFACT_CONTEXT = "artifactContext"; public static final String ARTIFACT_ID = "artifactId"; public static final String BODY = "body"; - public static final String ORGANIZATION_ID = "organizationId"; + // Organization related constants + public static final String ORGANIZATION = "organization"; + public static final String ORGANIZATION_ID = "organizationId";Also applies to: 208-208
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/client/src/ce/reducers/index.tsx(3 hunks)app/client/src/ce/sagas/PageSagas.tsx(1 hunks)app/client/src/ce/sagas/index.tsx(2 hunks)app/client/src/ce/utils/permissionHelpers.tsx(1 hunks)app/client/src/selectors/propertyPaneSelectors.tsx(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java(5 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceImpl.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- app/client/src/ce/utils/permissionHelpers.tsx
- app/client/src/ce/sagas/PageSagas.tsx
- app/client/src/selectors/propertyPaneSelectors.tsx
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java
- app/client/src/ce/sagas/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: server-spotless / spotless-check
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (5)
app/client/src/ce/reducers/index.tsx (2)
57-58: LGTM!The imports follow the established pattern and are correctly placed.
93-93: LGTM!The organization reducer is correctly integrated into the root reducer object.
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (2)
34-34: LGTM!The import of OrganizationService aligns with the tenant to organization migration.
100-100: LGTM!The OrganizationService is properly injected and initialized following the existing pattern.
Also applies to: 129-129, 149-149
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceImpl.java (1)
14-14:❓ Verification inconclusive
LGTM! Verify complete tenant-to-organization migration.
The changes correctly implement the tenant-to-organization migration for the DatasourceServiceImpl class.
Let's verify that all tenant-related code has been migrated:
Also applies to: 47-47, 68-68
🏁 Script executed:
#!/bin/bash # Search for any remaining tenant references in the codebase rg -i "tenant" app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/Length of output: 424
LGTM for the migration in DatasourceServiceImpl!
The migration correctly replaces TenantService with OrganizationService in this file (including the import, constructor parameter, and super call). However, our search revealed lingering tenant references in the CE variant (
DatasourceServiceCEImpl.java), such as the static import ofTENANT_IDand its usage in the metadata. Please confirm whether these references are intentional (e.g., if the CE version follows a different migration path) or if they also need to be updated for consistency.
|
Cypress tests started failing just now. They are passing on EE. Unrelated failures. |
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/test all
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13391658708
Commit: d30db44
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/ActionExecution/FrameworkFunctions_LocalStoreValueFunctions_spec.ts
- cypress/e2e/Regression/ClientSide/SetProperty/WidgetPropertySetters2_spec.ts
- cypress/e2e/Regression/ClientSide/Widgets/TableV2/table_data_change_spec.ts
List of identified flaky tests.Tue, 18 Feb 2025 15:17:25 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Refactor
Chore
These updates improve consistency and clarity in how configurations and permissions are managed and presented.