Add support for multitenancy in SQL DAO#37
Merged
kaushalaroraharman merged 28 commits intomainfrom Dec 18, 2025
Merged
Conversation
…tasource-implementation-for-multitenancy Feature 17 Multitenancy support in sql-dao library
…tasource-implementation-for-multitenancy Update version to 1.9-feat-17-1-SNAPSHOT
…tasource-implementation-for-multitenancy Feature 17 SQL DAO MultiTenancy Support.
Update GitHub Actions workflow for secret checks
…tasource-implementation-for-multitenancy Feature 17 Fixed Javadoc issues
…tasource-implementation-for-multitenancy Feature 17 SQL DAO Multitenancy Support.
…tasource-implementation-for-multitenancy Feature #17 abstractroutingdatasource implementation for multitenancy
…tasource-implementation-for-multitenancy GitHub Feature #17 Added DependsOn to prevent a race condition.
…y.enabled property read issue.
…tasource-implementation-for-multitenancy GitHub Feature #17 Changed TenantRoutingDataSource to fix multitenanc…
…multi.tenant.ids & Replaced hardcoded values with constants.
…tasource-implementation-for-multitenancy Feature #17 SQL DAO Multitenancy Support - Assigned default value to …
1 task
🔍 EOL Dependencies Check Results🔍 Dependencies End-of-Life ReportGenerated on 2025-12-10 07:50:35 UTC 📊 Summary
❌ End-of-Life DependenciesThese dependencies have reached their end-of-life and should be updated immediately:
✅ Supported DependenciesClick to expand untracked dependencies
❓ Untracked DependenciesClick to expand untracked dependenciesThese dependencies could not be tracked, as they were not found in the EOL database:
🔧 RecommendationsImmediate Action Required
Useful Resources
|
Contributor
Author
|
Code Review comments with GPT-5.1-Codex-Max: PR Review Comments for eclipse-ecsp/sql-dao PR #37Category: Configuration Management, Severity: Critical -> In Category: Testing, Severity: Critical -> In Category: Testing, Severity: Moderate -> |
Contributor
Author
kaushalaroraharman
left a comment
There was a problem hiding this comment.
Review comments added.
| */ | ||
| @Configuration | ||
| @EnableScheduling | ||
| @Component("postgresDbConfig") |
Contributor
Author
There was a problem hiding this comment.
Adding @component alongside already present @configuration is redundant; Remove @component unless required for a very specific reason, in that case remove @configuration
| ExecutorService executor = Executors.newFixedThreadPool(threadCount); | ||
| List<CompletableFuture<Void>> futures = new ArrayList<>(); | ||
|
|
||
| for (String tenantId : tenantIds) { |
Contributor
Author
There was a problem hiding this comment.
This will throw NPE if tenantIDs is null. It is being checked at line#195, but only at that line. Check if you tenantIds are available before calling initializeDataSourcesForTenants method, otherwise the whole method call can be skipped.
| futures.add(CompletableFuture.runAsync(() -> { | ||
| try { | ||
| DataSource tenantDataSource = initDataSource(tenantId, | ||
| multiTenantDbProperties.getTenants().get(tenantId)); |
Contributor
Author
There was a problem hiding this comment.
Why do you need a separate property for tenantIDs? We already have all the IDs if you iterate through the keys under multiTenantDbProperties.getTenants(). Having another tenantIDs list from properties seems redundant.
| public class MultiTenantDatabaseProperties { | ||
|
|
||
| /** Map of tenant IDs to their database properties */ | ||
| private Map<String, TenantDatabaseProperties> tenants = new HashMap<>(); |
Contributor
Author
There was a problem hiding this comment.
MultiTenantDatabaseProperties currently expects property names as tenant.tenants.tenantA.propertyName. Instead, we should have tenant.tenantA.propertyName. The additional tenants in property name is redundant.
Can be done by Expose a Map bean bound to the prefix:
@configuration
public class TenantConfig {
@bean
@ConfigurationProperties(prefix = "tenant")
public Map<String, TenantDatabaseProperties> tenants() { return new HashMap<>(); } }
Now properties like tenant.tenantA.jdbc-url will be bound as map entries, and we can skip the additional tenants in property names.
| DatabaseProperties tenantDbProperties = | ||
| multiTenantDbProperties.getTenants().get(tenantId); | ||
| credsProviderMap.put(tenantId, | ||
| (CredentialsProvider) utils.getClassInstance(tenantDbProperties.getCredentialProviderBeanName())); |
Contributor
Author
There was a problem hiding this comment.
utils.getClassInstance can throw unhandled SqlDaoException if the class is not loaded by the given name from tenantDbProperties.getCredentialProviderBeanName(). If there are 3 tenants, and load fails for the first one, because of unhandled exception it won't proceed for the other 2 tenants, even if they had valid credential provider bean names configured.
| LOGGER.info("Multitenancy is disabled. Using default tenant credentials provider."); | ||
| credsProviderMap.put(MultitenantConstants.DEFAULT_TENANT_ID, | ||
| (CredentialsProvider) utils.getClassInstance(defaultDbProperties.getCredentialProviderBeanName())); | ||
| } else { |
Contributor
Author
There was a problem hiding this comment.
Return credsProviderMap from the if block, instead of writing an else block to reduce conditional complexity.
| Map<String, TenantDatabaseProperties> tenants = multiTenantDbProperties.getTenants(); | ||
| if (tenants == null || tenants.isEmpty()) { | ||
| LOGGER.info("Executing credentials refresh job for default tenant."); | ||
| executeCredsRefreshForTenant(MultitenantConstants.DEFAULT_TENANT_ID, |
Contributor
Author
There was a problem hiding this comment.
Can't we have a default tenantID in properties - "tenant.default.propertyName"? This will map default tenantID with properties in MultiTenantDatabaseProperties, and we can avoid conditions for checking if tenantID is present or not, which is present at multiple places and explicitly adding and managing separate configuration for DEFAULT_TENANT_ID in the code.
| dbProperties.setConnectionRetryCount(connectionRetryCount); | ||
| connection = createConnections(tenantId, dbProperties); | ||
| if (connection != null && !connection.isValid(1)) { | ||
| Thread.sleep(connectionRetryDelay); |
Contributor
Author
There was a problem hiding this comment.
Thread.sleep(connectionRetryDelay) is current at two places, one here, and another in the catch block. Instead, this can be placed once at the start of try block, since this method is called only if createConnection fails, we should wait the delay time before the first retry as well.
| private String rootCrtPath; | ||
| @Bean("targetDataSources") | ||
| @DependsOn("credentialsProvider") | ||
| public Map<Object, Object> constructTargetDataSources() { |
Contributor
Author
There was a problem hiding this comment.
This bean will always have TenantID (String) as key and dataSource as value, as such the map can be defined accordingly Map<String, DataSource>, instead of keeping it generic Object, unless required for any specific reason. This will avoid unnecessary checks and casting in code.
| * @version 1.1 | ||
| * @since 2025-10-28 | ||
| */ | ||
| public class PostgresMultiTenantCredentialsProvider implements CredentialsProvider { |
Contributor
Author
There was a problem hiding this comment.
Add Test to name of this class
…tasource-implementation-for-multitenancy Addressing review comments - 17 feat abstractroutingdatasource implementation for multitenancy
🔍 EOL Dependencies Check Results🔍 Dependencies End-of-Life ReportGenerated on 2025-12-18 13:07:47 UTC 📊 Summary
❌ End-of-Life DependenciesThese dependencies have reached their end-of-life and should be updated immediately:
✅ Supported DependenciesClick to expand untracked dependencies
❓ Untracked DependenciesClick to expand untracked dependenciesThese dependencies could not be tracked, as they were not found in the EOL database:
🔧 RecommendationsImmediate Action Required
Useful Resources
|
Adding support for dynamic refresh and addition/updation/removal of data sources at runtime
🔍 EOL Dependencies Check Results🔍 Dependencies End-of-Life ReportGenerated on 2025-12-18 15:55:50 UTC 📊 Summary
❌ End-of-Life DependenciesThese dependencies have reached their end-of-life and should be updated immediately:
✅ Supported DependenciesClick to expand untracked dependencies
❓ Untracked DependenciesClick to expand untracked dependenciesThese dependencies could not be tracked, as they were not found in the EOL database:
🔧 RecommendationsImmediate Action Required
Useful Resources
|
Contributor
Author
|
Reviewed and Approved. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please refer to our contributing docs for any questions on submitting a pull request.
Issues are required for both bug fixes and features.
Resolves #17
Describe behaviour before the change
Describe behaviour after the change
Pull request checklist
Does this introduce a breaking change?