Skip to content

[HCMPRE-2741] central instance changes for egov user #740

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

holashchand
Copy link
Collaborator

@holashchand holashchand commented May 12, 2025

Summary by CodeRabbit

  • New Features

    • Introduced dynamic schema support for multi-tenant environments, enabling tenant-specific database operations.
    • Added a utility to handle schema name resolution based on tenant ID.
    • Added a new error code identifier for invalid tenant IDs.
    • Enforced non-null validation on tenant ID fields in user request and search request inputs.
    • Included tenant context in user retrieval and failed login attempt management.
  • Bug Fixes

    • Improved handling of failed login attempts by ensuring tenant context is included.
  • Chores

    • Enhanced migration script to support running database migrations across multiple schemas.
  • Tests

    • Updated repository tests to accommodate new schema utility dependencies.
    • Temporarily disabled several controller test suites to streamline testing.

Copy link

coderabbitai bot commented May 12, 2025

Walkthrough

This update introduces dynamic schema resolution for multi-tenant support across user-related repositories. SQL queries in repositories and query builders now use a schema placeholder, replaced at runtime using a new DatabaseSchemaUtils utility based on the tenant ID. Repository constructors and relevant method signatures are updated to integrate this utility. Associated tests are adjusted for the new dependencies. Additionally, failed login attempt methods now include tenant ID parameters. The Flyway migration script is enhanced to support multiple schemas. Some test classes are annotated to be ignored. Validation for tenant ID fields is strengthened with @NotNull annotations. A new error code constant for invalid tenant IDs is added.

Changes

File(s) Change Summary
core-services/egov-user/src/main/java/org/egov/user/config/UserServiceConstants.java Added new error code constant INVALID_TENANT_ID_ERR_CODE.
core-services/egov-user/src/main/java/org/egov/user/domain/service/UserService.java Updated getUserByUuid method signature to include tenant ID; updated calls to pass tenant ID; updated failed login attempt methods to pass tenant ID to repository methods.
core-services/egov-user/src/main/java/org/egov/user/persistence/repository/AddressRepository.java
core-services/egov-user/src/main/java/org/egov/user/persistence/repository/AuditRepository.java
core-services/egov-user/src/main/java/org/egov/user/persistence/repository/RoleRepository.java
core-services/egov-user/src/main/java/org/egov/user/persistence/repository/UserRepository.java
Integrated DatabaseSchemaUtils for dynamic schema replacement in all SQL queries; updated constructors and relevant method signatures; removed MultiStateInstanceUtil from UserRepository.
core-services/egov-user/src/main/java/org/egov/user/repository/builder/AddressQueryBuilder.java
core-services/egov-user/src/main/java/org/egov/user/repository/builder/RoleQueryBuilder.java
core-services/egov-user/src/main/java/org/egov/user/repository/builder/UserTypeQueryBuilder.java
Updated SQL query constants to use schema placeholder (SCHEMA_REPLACE_STRING) for all table and sequence names.
core-services/egov-user/src/main/java/org/egov/user/utils/DatabaseSchemaUtils.java Added new utility class for schema resolution based on tenant ID, with methods for placeholder replacement and tenant ID parsing.
core-services/egov-user/src/main/resources/db/migrate.sh Rewrote migration script to support running Flyway migrations on multiple schemas using dynamic URL construction.
core-services/egov-user/src/test/java/org/egov/user/persistence/repository/AddressRepositoryTest.java
core-services/egov-user/src/test/java/org/egov/user/persistence/repository/UserRepositoryTest.java
Updated tests to inject and use DatabaseSchemaUtils in repository constructors.
core-services/egov-user/src/test/java/org/egov/user/web/controller/PasswordControllerTest.java
core-services/egov-user/src/test/java/org/egov/user/web/controller/UserControllerTest.java
core-services/egov-user/src/test/java/org/egov/user/web/controller/UserRequestControllerTest.java
Added @Ignore annotation at class level to disable all tests in these test classes.
core-services/egov-user/src/main/java/org/egov/user/web/contract/UserRequest.java
core-services/egov-user/src/main/java/org/egov/user/web/contract/UserSearchRequest.java
Added @NotNull validation annotation to tenantId fields to enforce non-null tenant IDs.
core-services/egov-user/src/main/java/org/egov/user/domain/model/UpdateRequest.java Set tenantId field in domain Address objects constructed from update request.
core-services/egov-user/src/main/java/org/egov/user/security/oauth2/custom/authproviders/CustomAuthenticationProvider.java Replaced MultiStateInstanceUtil with DatabaseSchemaUtils for schema utility dependency.
core-services/egov-user/pom.xml Updated versions of services-common and enc-client dependencies.

Sequence Diagram(s)

Loading
sequenceDiagram
    participant Service as UserService
    participant Repo as UserRepository
    participant Utils as DatabaseSchemaUtils
    participant DB as Database

    Service->>Repo: resetFailedLoginAttempts(tenantId, userUuid)
    Repo->>Utils: replaceSchemaPlaceholder(query, tenantId)
    Utils-->>Repo: query with schema replaced
    Repo->>DB: Execute schema-specific query

    Service->>Repo: insertFailedLoginAttempt(tenantId, attempt)
    Repo->>Utils: replaceSchemaPlaceholder(query, tenantId)
    Utils-->>Repo: query with schema replaced
    Repo->>DB: Execute schema-specific query
Loading
sequenceDiagram
    participant Repo as AddressRepository/RoleRepository/AuditRepository
    participant Utils as DatabaseSchemaUtils
    participant DB as Database

    Repo->>Utils: replaceSchemaPlaceholder(query, tenantId)
    Utils-->>Repo: query with schema replaced
    Repo->>DB: Execute schema-specific query

Poem

In burrows deep, with whiskers keen,
The schemas now are tenant-seen!
A hop, a skip, dynamic flows—
Each query knows just where it goes.
With clever code and schema art,
Our multi-tenant dreams now start!
🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a50136c and e01a84f.

📒 Files selected for processing (1)
  • core-services/egov-user/src/main/java/org/egov/user/persistence/repository/UserRepository.java (15 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • core-services/egov-user/src/main/java/org/egov/user/persistence/repository/UserRepository.java
✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🔭 Outside diff range comments (1)
core-services/egov-user/src/main/java/org/egov/user/persistence/repository/UserRepository.java (1)

210-213: ⚠️ Potential issue

Incorrect regex – tenant extraction will never work

tenantId.split("//.")[0] tries to split on the literal string "//.", not a dot.
The intention is clearly to obtain the state-level part before '.', but the current code leaves tenantId unchanged and can break authorisation or schema resolution logic.

-        if(UserType.CITIZEN.equals(oldUser.getType()) && tenantId.contains("."))
-        		tenantId = tenantId.split("//.")[0];
+        if (UserType.CITIZEN.equals(oldUser.getType()) && tenantId.contains(".")) {
+            /* split on a dot; escape it because dot is a regex meta-character */
+            tenantId = tenantId.split("\\.")[0];
+        }

Please add a test covering both a two-level tenant (pb.amritsar) and a single-level tenant (in) to ensure the logic behaves as expected.

🧹 Nitpick comments (12)
core-services/egov-user/src/main/resources/db/migrate.sh (2)

3-3: Avoid echoing sensitive data to logs.
Printing the full DB URL, schema names, or credentials may leak secrets in CI/CD logs. Limit or mask these echos, or gate them behind a debug flag.

Also applies to: 6-6, 9-9


7-11: Improve schema iteration with IFS and proper quoting.
Instead of ${schemas//,/ }, use IFS to split on commas and trim whitespace. Quote all expansions in the flyway command for safety.

# Replace the loop with:
IFS=',' read -ra schema_array <<< "${SCHEMA_NAME}"
for schemaname in "${schema_array[@]}"; do
  schemaname="${schemaname//[[:space:]]/}"
  flyway \
    -url="${baseurl}?currentSchema=${schemaname}" \
    -table="${SCHEMA_TABLE}" \
    -user="${FLYWAY_USER}" \
    -password="${FLYWAY_PASSWORD}" \
    -locations="${FLYWAY_LOCATIONS}" \
    -baselineOnMigrate=true \
    -outOfOrder=true \
    migrate
done

This approach handles edge cases (spaces, empty entries) and ensures each argument is safely quoted.

core-services/egov-user/src/main/java/org/egov/user/repository/builder/RoleQueryBuilder.java (1)

5-17: Consider co-locating the schema constant with the new DatabaseSchemaUtils utility

All new repositories now rely on DatabaseSchemaUtils for placeholder substitution, yet the constant SCHEMA_REPLACE_STRING is still sourced from MultiStateInstanceUtil. Keeping the constant in a different utility couples the builders to an unrelated class and may be confusing once MultiStateInstanceUtil becomes obsolete.

You can gradually move the constant to DatabaseSchemaUtils (or create a lightweight interface that re-exports it) and update the static imports in the builders.
This is a non-blocking clean-up but will make the intent of the code clearer and reduce accidental coupling.

core-services/egov-user/src/main/java/org/egov/user/persistence/repository/AddressRepository.java (2)

91-97: Variable name shadows java.util.Map

final Map<String, Object> Map = new HashMap<>(); re-uses the type name as a variable identifier, reducing readability and possibly confusing IDE tooling. Prefer a lower-camel-case name such as params or queryParams.

-final Map<String, Object> Map = new HashMap<String, Object>();
+final Map<String, Object> params = new HashMap<>();

184-189: Redundant import versus fully-qualified usage

isEmpty is statically imported at the top (import static org.springframework.util.CollectionUtils.isEmpty;) but inside this block you still call CollectionUtils.isEmpty(ids). Stick to one style to avoid confusion.

No functional impact, but a quick clean-up promotes consistency.

core-services/egov-user/src/main/java/org/egov/user/utils/DatabaseSchemaUtils.java (7)

15-17: Clean up commented-out annotations

The class has commented-out annotations (@component and @AllArgsConstructor) that should either be removed completely or uncommented if needed. Commented code reduces readability and can confuse other developers about the intended configuration.

@Configuration
-//@Component
-//@AllArgsConstructor
public class DatabaseSchemaUtils {

27-29: Remove unnecessary null initialization

The finalQuery variable is initialized to null but is always assigned a value before being returned. This initialization is unnecessary.

public String replaceSchemaPlaceholder(String query, String tenantId) {
-    String finalQuery = null;
+    String finalQuery;
    if (this.getIsEnvironmentCentralInstance()) {

41-45: Consider consistent schema placeholder replacement logic

The current implementation uses different approaches for replacing the schema placeholder between central and non-central instances:

  • For central instances, it replaces the placeholder with a schema name
  • For non-central instances, it removes the placeholder AND the dot after it

This difference could lead to SQL syntax issues if queries aren't carefully constructed with this behavior in mind.

Consider making the replacement logic more consistent:

if (this.getIsEnvironmentCentralInstance()) {
    finalQuery = query.replaceAll("(?i)" + Pattern.quote(SCHEMA_REPLACE_STRING), schemaName);
} else {
-    finalQuery = query.replaceAll("(?i)" + Pattern.quote(SCHEMA_REPLACE_STRING.concat(".")), "");
+    // Just remove the schema placeholder itself, consistent with central instance approach
+    finalQuery = query.replaceAll("(?i)" + Pattern.quote(SCHEMA_REPLACE_STRING), "");
}

Alternatively, if SQL queries are designed with the expectation that the dot will be removed in non-central instances, document this behavior clearly to prevent misunderstandings.


58-72: Use StringBuilder for efficient string concatenation

The current implementation uses string concatenation in a loop which is inefficient. Consider using StringBuilder for better performance, especially if handling large numbers of tenants.

public String getStateLevelTenant(String tenantId) {
    String[] tenantArray = tenantId.split("\\.");
-    String stateTenant = tenantArray[0];
+    StringBuilder stateTenant = new StringBuilder(tenantArray[0]);
    if (this.getIsEnvironmentCentralInstance()) {
        if (this.getStateLevelTenantIdLength() < tenantArray.length) {
            for(int i = 1; i < this.getStateLevelTenantIdLength(); ++i) {
-                stateTenant = stateTenant.concat(".").concat(tenantArray[i]);
+                stateTenant.append(".").append(tenantArray[i]);
            }
        } else {
-            stateTenant = tenantId;
+            return tenantId;
        }
    }

-    return stateTenant;
+    return stateTenant.toString();
}

30-32: Improve error message for better troubleshooting

The current error message when a tenant ID is too short could be more helpful by including the actual values in the message.

if (this.stateSchemaIndexPositionInTenantId >= tenantId.length()) {
-    throw new CustomException(INVALID_TENANT_ID_ERR_CODE, "The tenantId length is smaller than the defined schema index in tenantId for central instance");
+    throw new CustomException(INVALID_TENANT_ID_ERR_CODE, 
+        String.format("The tenantId '%s' length (%d) is smaller than the defined schema index (%d) for central instance", 
+        tenantId, tenantId.length(), this.stateSchemaIndexPositionInTenantId));
}

1-109: Consider adding JavaDoc documentation

This utility class contains complex logic for schema management and tenant processing, but lacks comprehensive documentation. Adding JavaDoc comments would improve maintainability and help other developers understand how to use this class correctly.

Add class-level and method-level JavaDoc documentation explaining:

  1. The purpose of the class
  2. What each method does
  3. Parameter descriptions
  4. Return value descriptions
  5. Exception conditions
  6. Examples of usage for complex methods

Example for the class:

/**
 * Utility class for handling database schema operations in multi-tenant environments.
 * This class provides methods to dynamically resolve schemas based on tenant IDs,
 * replacing schema placeholders in SQL queries and determining tenant hierarchy levels.
 * <p>
 * The behavior varies between central and non-central instances:
 * - In central instances, tenant IDs contain schema information at configurable positions
 * - In non-central instances, schema prefixes are removed from queries
 */
@Configuration
public class DatabaseSchemaUtils {
    // ...
}

19-19: Make SCHEMA_REPLACE_STRING final for immutability

The SCHEMA_REPLACE_STRING constant should be declared as final since it's not intended to be modified.

-public static String SCHEMA_REPLACE_STRING = "{schema}";
+public static final String SCHEMA_REPLACE_STRING = "{schema}";
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aa563a9 and 73dece0.

📒 Files selected for processing (13)
  • core-services/egov-user/src/main/java/org/egov/user/config/UserServiceConstants.java (1 hunks)
  • core-services/egov-user/src/main/java/org/egov/user/domain/service/UserService.java (3 hunks)
  • core-services/egov-user/src/main/java/org/egov/user/persistence/repository/AddressRepository.java (8 hunks)
  • core-services/egov-user/src/main/java/org/egov/user/persistence/repository/AuditRepository.java (3 hunks)
  • core-services/egov-user/src/main/java/org/egov/user/persistence/repository/RoleRepository.java (6 hunks)
  • core-services/egov-user/src/main/java/org/egov/user/persistence/repository/UserRepository.java (16 hunks)
  • core-services/egov-user/src/main/java/org/egov/user/repository/builder/AddressQueryBuilder.java (1 hunks)
  • core-services/egov-user/src/main/java/org/egov/user/repository/builder/RoleQueryBuilder.java (1 hunks)
  • core-services/egov-user/src/main/java/org/egov/user/repository/builder/UserTypeQueryBuilder.java (4 hunks)
  • core-services/egov-user/src/main/java/org/egov/user/utils/DatabaseSchemaUtils.java (1 hunks)
  • core-services/egov-user/src/main/resources/db/migrate.sh (1 hunks)
  • core-services/egov-user/src/test/java/org/egov/user/persistence/repository/AddressRepositoryTest.java (2 hunks)
  • core-services/egov-user/src/test/java/org/egov/user/persistence/repository/UserRepositoryTest.java (3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
core-services/egov-user/src/main/java/org/egov/user/repository/builder/RoleQueryBuilder.java (2)
core-services/egov-user/src/main/java/org/egov/user/repository/builder/AddressQueryBuilder.java (1)
  • Component (7-14)
core-services/egov-user/src/main/java/org/egov/user/repository/builder/UserTypeQueryBuilder.java (1)
  • Component (56-308)
🔇 Additional comments (22)
core-services/egov-user/src/main/java/org/egov/user/config/UserServiceConstants.java (1)

73-73: Addition of INVALID_TENANT_ID_ERR_CODE constant looks good.

This constant provides a standardized error code for tenant validation scenarios, which aligns well with the multi-tenant schema support being implemented across the codebase.

core-services/egov-user/src/test/java/org/egov/user/persistence/repository/UserRepositoryTest.java (1)

16-16: DatabaseSchemaUtils dependency integration looks good.

The test has been appropriately updated to inject the new DatabaseSchemaUtils dependency and pass it to the UserRepository constructor, reflecting the changes made to support dynamic schema resolution based on tenant IDs.

Also applies to: 71-73, 91-93

core-services/egov-user/src/test/java/org/egov/user/persistence/repository/AddressRepositoryTest.java (1)

5-5: DatabaseSchemaUtils dependency integration looks good.

The AddressRepositoryTest has been correctly updated to autowire the DatabaseSchemaUtils dependency and pass it to the repository constructor, maintaining consistency with other repository tests.

Also applies to: 36-38, 44-44

core-services/egov-user/src/main/java/org/egov/user/domain/service/UserService.java (3)

503-504: Tenant ID now correctly passed to resetFailedLoginAttemptsForUser method

The change correctly passes the tenant ID to the repository method, enabling proper schema resolution in a multi-tenant environment.


542-544: Enhanced fetchFailedAttemptsByUserAndTime call with tenant context

The repository method now receives tenant ID as its first parameter, allowing for proper schema selection in the multi-tenant database environment. This ensures user login attempts are fetched from the correct tenant schema.


561-562: Added tenant ID to insertFailedLoginAttempt method call

The repository method now correctly receives tenant ID as the first parameter, ensuring failed login attempts are recorded in the appropriate tenant schema.

core-services/egov-user/src/main/java/org/egov/user/repository/builder/AddressQueryBuilder.java (3)

5-5: Added import for schema replacement utility

Correctly imports the SCHEMA_REPLACE_STRING constant needed for dynamic schema resolution.


10-10: Applied schema placeholder to GET_ADDRESSBY_IDAND_TENANT query

The query now includes the schema placeholder prefix that will be dynamically replaced at runtime based on tenant ID.


12-12: Applied schema placeholder to DELETE_ADDRESS query

The query now includes the schema placeholder prefix that will be dynamically replaced at runtime based on tenant ID.

core-services/egov-user/src/main/java/org/egov/user/persistence/repository/AuditRepository.java (6)

4-4: Added import for schema replacement utility

Correctly imports the SCHEMA_REPLACE_STRING constant needed for dynamic schema resolution.


19-19: Added import for DatabaseSchemaUtils

The import for the new utility class that will handle schema resolution logic.


27-28: Applied schema placeholder to INSERT_AUDIT_DETAILS query

The query now includes the schema placeholder prefix that will be dynamically replaced at runtime based on tenant ID.


32-33: Added DatabaseSchemaUtils field

Added field to store the schema utilities instance, which will be used for schema resolution.


35-38: Updated constructor to include DatabaseSchemaUtils

Constructor now accepts and initializes the DatabaseSchemaUtils dependency for schema resolution.


152-154: Implemented dynamic schema resolution

The query is now processed through DatabaseSchemaUtils to replace the schema placeholder with the actual schema based on the tenant ID before execution.

core-services/egov-user/src/main/java/org/egov/user/persistence/repository/RoleRepository.java (6)

25-25: Added import for DatabaseSchemaUtils

The import for the new utility class that will handle schema resolution logic.


48-48: Added DatabaseSchemaUtils field

Added field to store the schema utilities instance, which will be used for schema resolution.


66-71: Updated constructor to include DatabaseSchemaUtils

Constructor now accepts and initializes the DatabaseSchemaUtils dependency for schema resolution.


85-88: Applied schema resolution to GET_ROLES_BY_ID_TENANTID query

The query is now processed through DatabaseSchemaUtils to replace the schema placeholder with the actual schema before execution.


102-104: Applied schema resolution to GET_ROLES_BY_ROLEIDS query

The query is now processed through DatabaseSchemaUtils to replace the schema placeholder with the actual schema before execution.


122-125: Applied schema resolution to GET_ROLE_BYTENANT_ANDCODE query

The query is now processed through DatabaseSchemaUtils to replace the schema placeholder with the actual schema before execution.

core-services/egov-user/src/main/java/org/egov/user/persistence/repository/UserRepository.java (1)

451-453: Double-check getStateLevelTenant implementation

databaseSchemaUtils.getStateLevelTenant() is invoked here, but its contract is not visible in this diff.
Verify that it:

  1. Handles null/empty input gracefully.
  2. Correctly extracts the state-level code for tenants both with and without a dot.

If the method is unavailable or implemented differently, this call will fail or return an unexpected value, leading to MDMS role look-ups against the wrong schema.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
core-services/egov-user/src/test/java/org/egov/user/web/controller/PasswordControllerTest.java (1)

13-14: Remove unused imports.

The imports for MaskingService and MdmsFetcher appear to be unused in this file. They should be removed to avoid confusion.

-import org.egov.encryption.masking.MaskingService;
-import org.egov.encryption.util.MdmsFetcher;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 73dece0 and 0fffe20.

📒 Files selected for processing (3)
  • core-services/egov-user/src/test/java/org/egov/user/web/controller/PasswordControllerTest.java (2 hunks)
  • core-services/egov-user/src/test/java/org/egov/user/web/controller/UserControllerTest.java (1 hunks)
  • core-services/egov-user/src/test/java/org/egov/user/web/controller/UserRequestControllerTest.java (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
core-services/egov-user/src/test/java/org/egov/user/web/controller/UserControllerTest.java (2)
core-services/egov-user/src/test/java/org/egov/user/web/controller/PasswordControllerTest.java (1)
  • Ignore (34-100)
core-services/egov-user/src/test/java/org/egov/user/web/controller/UserRequestControllerTest.java (1)
  • Ignore (47-210)
core-services/egov-user/src/test/java/org/egov/user/web/controller/UserRequestControllerTest.java (2)
core-services/egov-user/src/test/java/org/egov/user/web/controller/PasswordControllerTest.java (1)
  • Ignore (34-100)
core-services/egov-user/src/test/java/org/egov/user/web/controller/UserControllerTest.java (1)
  • Ignore (65-485)
🔇 Additional comments (3)
core-services/egov-user/src/test/java/org/egov/user/web/controller/UserControllerTest.java (1)

65-65: Ensure this test class doesn't remain ignored long-term.

The addition of the @Ignore annotation at the class level disables all tests in this class. While this may be necessary during the implementation of multi-tenant schema support, consider documenting why these tests are ignored and create a follow-up task to re-enable them once the schema changes are fully implemented.

core-services/egov-user/src/test/java/org/egov/user/web/controller/UserRequestControllerTest.java (1)

47-47: Consider adding a comment explaining why the test class is ignored.

Adding @Ignore at the class level disables all tests in this controller. This appears to be part of a coordinated effort to temporarily disable controller tests during the implementation of multi-tenant schema support. To improve maintainability, add a comment explaining the rationale and link to any relevant issue tracking the re-enabling of these tests.

core-services/egov-user/src/test/java/org/egov/user/web/controller/PasswordControllerTest.java (1)

34-34: Document the reason for disabling tests.

Similar to the other controller test classes, this class has been annotated with @Ignore to skip all tests. Add a comment explaining why these tests are temporarily disabled and how/when they will be re-enabled once the multi-tenant schema implementation is complete.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between b812749 and 1b90731.

📒 Files selected for processing (2)
  • core-services/egov-user/src/main/java/org/egov/user/web/contract/UserRequest.java (2 hunks)
  • core-services/egov-user/src/main/java/org/egov/user/web/contract/UserSearchRequest.java (2 hunks)
🔇 Additional comments (2)
core-services/egov-user/src/main/java/org/egov/user/web/contract/UserRequest.java (1)

19-19: Clean import addition.

Good addition of the @NotBlank validation constraint import that will be used for tenant ID validation.

core-services/egov-user/src/main/java/org/egov/user/web/contract/UserSearchRequest.java (1)

12-12: Clean import addition.

Good addition of the @NotBlank validation constraint import that will be used for tenant ID validation.

@holashchand holashchand requested a review from Sreejit-K May 23, 2025 04:50
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 28e35c7 and a50136c.

📒 Files selected for processing (10)
  • core-services/egov-user/pom.xml (1 hunks)
  • core-services/egov-user/src/main/java/org/egov/user/persistence/repository/AddressRepository.java (8 hunks)
  • core-services/egov-user/src/main/java/org/egov/user/persistence/repository/AuditRepository.java (3 hunks)
  • core-services/egov-user/src/main/java/org/egov/user/persistence/repository/RoleRepository.java (6 hunks)
  • core-services/egov-user/src/main/java/org/egov/user/persistence/repository/UserRepository.java (16 hunks)
  • core-services/egov-user/src/main/java/org/egov/user/repository/builder/AddressQueryBuilder.java (1 hunks)
  • core-services/egov-user/src/main/java/org/egov/user/repository/builder/RoleQueryBuilder.java (1 hunks)
  • core-services/egov-user/src/main/java/org/egov/user/repository/builder/UserTypeQueryBuilder.java (4 hunks)
  • core-services/egov-user/src/main/java/org/egov/user/security/oauth2/custom/authproviders/CustomAuthenticationProvider.java (2 hunks)
  • core-services/egov-user/src/main/java/org/egov/user/utils/DatabaseSchemaUtils.java (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • core-services/egov-user/src/main/java/org/egov/user/security/oauth2/custom/authproviders/CustomAuthenticationProvider.java
  • core-services/egov-user/pom.xml
🚧 Files skipped from review as they are similar to previous changes (7)
  • core-services/egov-user/src/main/java/org/egov/user/persistence/repository/RoleRepository.java
  • core-services/egov-user/src/main/java/org/egov/user/repository/builder/AddressQueryBuilder.java
  • core-services/egov-user/src/main/java/org/egov/user/repository/builder/UserTypeQueryBuilder.java
  • core-services/egov-user/src/main/java/org/egov/user/persistence/repository/AuditRepository.java
  • core-services/egov-user/src/main/java/org/egov/user/persistence/repository/AddressRepository.java
  • core-services/egov-user/src/main/java/org/egov/user/repository/builder/RoleQueryBuilder.java
  • core-services/egov-user/src/main/java/org/egov/user/utils/DatabaseSchemaUtils.java
🔇 Additional comments (11)
core-services/egov-user/src/main/java/org/egov/user/persistence/repository/UserRepository.java (11)

36-36: LGTM: Clean dependency injection pattern for DatabaseSchemaUtils.

The introduction of DatabaseSchemaUtils as a dependency is well-implemented with proper field declaration and constructor injection.

Also applies to: 53-53, 64-68


89-89: LGTM: Consistent schema replacement pattern for user search.

The extraction of tenantId and subsequent schema placeholder replacement follows the expected pattern for multi-tenant support.

Also applies to: 111-112


131-132: LGTM: Schema replacement applied to role search query.

The pattern is consistently applied to the user role search functionality.


156-157: LGTM: Schema replacement for user presence check.

The tenant-specific schema resolution is properly applied to the user existence validation query.


330-332: LGTM: Schema replacement for user updates.

The update query properly uses schema replacement with the extracted tenantId.


342-343: LGTM: Method signatures updated for tenant-aware failed login operations.

The failed login attempt methods now properly accept tenantId parameters, enabling tenant-specific schema resolution for security-related operations.

Also applies to: 346-346, 365-365, 378-378


358-361: LGTM: Schema replacement applied to failed login attempt queries.

All failed login attempt database operations now use proper schema replacement, maintaining consistency with the multi-tenant architecture.

Also applies to: 371-373, 379-382


459-459: LGTM: Updated to use new schema utility for state-level tenant.

The replacement of the old multi-state utility with databaseSchemaUtils.getStateLevelTenant() aligns with the new schema management approach.


487-489: LGTM: Schema replacement applied to batch operations.

User role insertions, user creation, and role updates all properly use schema replacement, ensuring tenant isolation for these critical operations.

Also applies to: 579-581, 624-626


586-596: LGTM: Enhanced documentation and schema support for sequence generation.

The getNextSequence method is well-documented and properly implements tenant-specific schema resolution for sequence generation.


171-171:

✅ Verification successful

Verify that all callers pass the required tenantId parameter.

The getNextSequence method signature change requires tenantId. Ensure all calling code has been updated to provide this parameter.


🏁 Script executed:

#!/bin/bash
# Search for calls to getNextSequence to verify they pass tenantId
ast-grep --pattern 'getNextSequence($_)'

Length of output: 366


All calls to getNextSequence now pass tenantId

Ran an AST search for getNextSequence(...) across the codebase and found only two usages—all supplying a tenantId argument:

  • UserRepository.java:171getNextSequence(user.getTenantId())
  • AddressRepository.java:60getNextSequence(tenantId)

No callers remain without the required parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants