chore: Minor code refactor for seat based pricing#38359
Conversation
trishaanand
requested review from
nidhi-nair and
sondermanish
as code owners
WalkthroughThis pull request introduces several modifications to user authentication and management components in the Appsmith server. The changes primarily focus on refactoring user retrieval methods, enhancing code modularity, and adjusting method visibility. Key modifications include adding a new Changes
Possibly related PRs
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
trishaanand
added
Platform Administration Pod
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCE.java (1)
44-45: Consider adding documentation comments for the new interface method
A brief Javadoc on usage and constraints would help future maintainers understand when to call this method and under what conditions.app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.java (1)
51-52: Consider returning an unmodifiable or cached set.Returning a new
HashSeteach time may introduce unnecessary object allocation. Also, an unmodifiable set could help prevent unintended modifications by consumers.public Set<String> getSystemGeneratedUserEmails() { - Set<String> systemGeneratedEmails = new HashSet<>(); - systemGeneratedEmails.add(FieldName.ANONYMOUS_USER); - return systemGeneratedEmails; + return Collections.singleton(FieldName.ANONYMOUS_USER); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOAuth2UserServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOidcUserServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCE.java(2 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/services/ce/UserServiceCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java(1 hunks)
🔇 Additional comments (7)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java (1)
513-514: Ensure adequate testing for the newly exposed method
Since the method is now publicly accessible, make sure there are unit or integration tests verifying permission checks, domain validations, and other logic branches.
Would you like help creating or extending tests for this method?
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOAuth2UserServiceCEImpl.java (2)
Line range hint 50-77: Consider validating the username before creating a new user.
It might be helpful to check whether username is null or empty before going ahead with the user creation. This can add a safeguard against accidental blank emails.
78-82: Caveat: Potentially returning multiple matches with different email casing.
The fallback query findFirstByEmailIgnoreCaseOrderByCreatedAtDesc(email) handles case insensitivity, but if there are multiple users with slightly varied email casing, it returns the first created user, possibly not the expected one. Ensure this is the desired behavior.
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOidcUserServiceCEImpl.java (2)
Line range hint 52-82: Check for empty or null email string.
When calling this.findByUsername(username), consider adding a defensive check against a null or empty username to avoid potential edge cases.
84-88: Same caution about multiple users with different email casing.
As in the OAuth2 flow, this method also retrieves the first user in descending order of creation date if the exact match is not found. Confirm whether this fallback behavior is intended in case of multiple users with different letter cases in their email addresses.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCE.java (2)
9-10: All good for the import usage.
The import of java.util.Set makes sense for the new method. No issues here.
19-20: Interface method addition looks good.
Exposing getSystemGeneratedUserEmails() in the interface is clear and does not appear to introduce any breaking changes. Ensure the rest of the codebase is aware of this contract.
… outside of these services
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOAuth2UserServiceCEImpl.java (1)
Line range hint
50-76: Consider renaming method call for clarityThe method name
findByUsernameis misleading here as it's being called with an email parameter. This could cause confusion for future maintainers.- return this.findByUsername(username) + return this.findByEmail(username)
📜 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/authentication/handlers/ce/CustomOAuth2UserServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOidcUserServiceCEImpl.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOidcUserServiceCEImpl.java
🔇 Additional comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOAuth2UserServiceCEImpl.java (1)
Line range hint 50-76: Verify OAuth2 user creation consistency
Let's ensure this refactor maintains consistent user creation behavior across all OAuth2 implementations.
✅ Verification successful
OAuth2 user creation implementation is consistent and secure
The implementation follows a secure pattern where:
- CustomOAuth2UserServiceImpl properly extends the CE implementation
- User creation logic is centralized in the CE class
- All security aspects (activation state, source tracking, error handling) are maintained
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar OAuth2 user creation patterns
ast-grep --pattern 'class $_ implements $_ {
$$$
private Mono<User> checkAndCreateUser($_, $_) {
$$$
}
$$$
}'
Length of output: 123
Script:
#!/bin/bash
# Search for OAuth2 user creation patterns with broader context
rg -U "class.*OAuth2.*implements.*\{[\s\S]*?new User\(\)[\s\S]*?\}" --type java -A 10
Length of output: 87
Script:
#!/bin/bash
# Search for user creation in OAuth2 related files
fd -e java -e kt "OAuth2" --exec rg -l "new User\(\)"
Length of output: 178
Script:
#!/bin/bash
# Look for OAuth2 authentication handlers
fd -e java "OAuth2.*Handler|.*OAuth2.*Service" --type f
Length of output: 298
Script:
#!/bin/bash
# Check implementation in CustomOAuth2UserServiceImpl.java
cat app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/CustomOAuth2UserServiceImpl.java
# Check the CE implementation for comparison
cat app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOAuth2UserServiceCEImpl.java
Length of output: 5185
| protected Mono<User> findByUsername(String email) { | ||
| return repository | ||
| .findByEmail(email) | ||
| .switchIfEmpty(repository.findFirstByEmailIgnoreCaseOrderByCreatedAtDesc(email)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve method naming and consider query optimization
- The method name should reflect that it's working with email addresses.
- Consider combining the two repository queries into a single case-insensitive query for better performance.
- protected Mono<User> findByUsername(String email) {
- return repository
- .findByEmail(email)
- .switchIfEmpty(repository.findFirstByEmailIgnoreCaseOrderByCreatedAtDesc(email));
+ protected Mono<User> findByEmail(String email) {
+ return repository.findFirstByEmailIgnoreCaseOrderByCreatedAtDesc(email);Additionally, consider adding a comment explaining why we need case-insensitive email lookup, as this might not be immediately obvious to other developers.
📝 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.
| protected Mono<User> findByUsername(String email) { | |
| return repository | |
| .findByEmail(email) | |
| .switchIfEmpty(repository.findFirstByEmailIgnoreCaseOrderByCreatedAtDesc(email)); | |
| } | |
| protected Mono<User> findByEmail(String email) { | |
| return repository.findFirstByEmailIgnoreCaseOrderByCreatedAtDesc(email); | |
| } |
There was a problem hiding this comment.
Lets stick to the existing version. For most usecases, the first repository call would return the user. The second repository call is very expensive and is being used only if the first repository call did not return a user. Also, while creating users, now we are ensuring that the email is in small case. The second repository call is only for very very old users where we weren't ignoring cases during creation of the user.
There was a problem hiding this comment.
@trishaanand Understood! Keeping the two queries makes sense with the fallback approach for older users who didn’t have their email in lowercase originally. Thanks for clarifying.
✏️ Learnings added
Learnt from: trishaanand
PR: appsmithorg/appsmith#38359
File: app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOAuth2UserServiceCEImpl.java:78-82
Timestamp: 2024-12-25T06:50:40.449Z
Learning: In `CustomOAuth2UserServiceCEImpl`, the second repository call for finding users by case-insensitive email is needed because older users might have uppercase or mixed-case emails, while new users have emails stored in lowercase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
## 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 Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /test sanity ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12490386991> > Commit: a06c916 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12490386991&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Wed, 25 Dec 2024 07:41:48 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced methods for user retrieval and signup conditions, enhancing user management capabilities. - Added functionality to retrieve system-generated user emails. - **Bug Fixes** - Improved user lookup process by centralizing logic into dedicated methods. - **Documentation** - Updated method visibility for better accessibility of user-related functionalities. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## 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 Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /test sanity ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12490386991> > Commit: a06c916 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12490386991&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Wed, 25 Dec 2024 07:41:48 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced methods for user retrieval and signup conditions, enhancing user management capabilities. - Added functionality to retrieve system-generated user emails. - **Bug Fixes** - Improved user lookup process by centralizing logic into dedicated methods. - **Documentation** - Updated method visibility for better accessibility of user-related functionalities. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
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 sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12490386991
Commit: a06c916
Cypress dashboard.
Tags:
@tag.SanitySpec:
Wed, 25 Dec 2024 07:41:48 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation