Skip to content

chore: Minor code refactor for seat based pricing #38359

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.

Sign up for GitHub

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

Merged
trishaanand merged 2 commits into from
Dec 25, 2024
Merged

chore: Minor code refactor for seat based pricing #38359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6de9d09
chore: Minor code refactor for seat based pricing
trishaanand Dec 25, 2024
a06c916
Changing access to protected instead of public since its not required…
trishaanand Dec 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions ...in/java/com/appsmith/server/authentication/handlers/ce/CustomOAuth2UserServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,7 @@ private Mono<User> checkAndCreateUser(OAuth2User oAuth2User, OAuth2UserRequest u

String username = oAuth2User.getName();

return repository
.findByEmail(username)
.switchIfEmpty(repository.findFirstByEmailIgnoreCaseOrderByCreatedAtDesc(username))
return this.findByUsername(username)
.switchIfEmpty(Mono.defer(() -> {
User newUser = new User();
newUser.setName(oAuth2User.getName());
Expand All @@ -76,4 +74,10 @@ private Mono<User> checkAndCreateUser(OAuth2User oAuth2User, OAuth2UserRequest u
error -> new AppsmithOAuth2AuthenticationException(
new OAuth2Error(error.getAppErrorCode().toString(), error.getMessage(), "")));
}

protected Mono<User> findByUsername(String email) {
return repository
.findByEmail(email)
.switchIfEmpty(repository.findFirstByEmailIgnoreCaseOrderByCreatedAtDesc(email));
}
Comment on lines +78 to +82
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve method naming and consider query optimization

  1. The method name should reflect that it's working with email addresses.
  2. 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.

Suggested change
protected Mono<User> findByUsername(String email) {
return repository
.findByEmail(email)
.switchIfEmpty(repository.findFirstByEmailIgnoreCaseOrderByCreatedAtDesc(email));
}
protected Mono<User> findByEmail(String email) {
return repository.findFirstByEmailIgnoreCaseOrderByCreatedAtDesc(email);
}

Copy link
Contributor Author

@trishaanand trishaanand Dec 25, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@coderabbitai coderabbitai bot Dec 25, 2024

Choose a reason for hiding this comment

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

@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.

}
10 changes: 7 additions & 3 deletions ...main/java/com/appsmith/server/authentication/handlers/ce/CustomOidcUserServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,7 @@ public Mono<User> checkAndCreateUser(OidcUser oidcUser, OidcUserRequest userRequ

String username = (!StringUtils.isEmpty(oidcUser.getEmail())) ? oidcUser.getEmail() : oidcUser.getName();

return repository
.findByEmail(username)
.switchIfEmpty(repository.findFirstByEmailIgnoreCaseOrderByCreatedAtDesc(username))
return this.findByUsername(username)
.switchIfEmpty(Mono.defer(() -> {
User newUser = new User();
if (oidcUser.getUserInfo() != null) {
Expand Down Expand Up @@ -82,4 +80,10 @@ public Mono<User> checkAndCreateUser(OidcUser oidcUser, OidcUserRequest userRequ
error -> new AppsmithOAuth2AuthenticationException(
new OAuth2Error(error.getAppErrorCode().toString(), error.getMessage(), "")));
}

protected Mono<User> findByUsername(String email) {
return repository
.findByEmail(email)
.switchIfEmpty(repository.findFirstByEmailIgnoreCaseOrderByCreatedAtDesc(email));
}
}
4 changes: 4 additions & 0 deletions ...mith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
import org.springframework.data.mongodb.core.query.UpdateDefinition;
import reactor.core.publisher.Mono;

import java.util.Set;

public interface CustomUserRepositoryCE extends AppsmithRepository<User> {

Mono<User> findByEmail(String email, AclPermission aclPermission);
Expand All @@ -14,5 +16,7 @@ public interface CustomUserRepositoryCE extends AppsmithRepository<User> {

Mono<Boolean> isUsersEmpty();

Set<String> getSystemGeneratedUserEmails();

Mono<Integer> updateById(String id, UpdateDefinition updateObj);
}
3 changes: 2 additions & 1 deletion ...-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ public Mono<Boolean> isUsersEmpty() {
.map(count -> count == 0);
}

protected Set<String> getSystemGeneratedUserEmails() {
@Override
public Set<String> getSystemGeneratedUserEmails() {
Set<String> systemGeneratedEmails = new HashSet<>();
systemGeneratedEmails.add(FieldName.ANONYMOUS_USER);
return systemGeneratedEmails;
Expand Down
2 changes: 2 additions & 0 deletions app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ public interface UserServiceCE extends CrudService<User, String> {

Flux<User> getAllByEmails(Set<String> emails, AclPermission permission);

Mono<User> signupIfAllowed(User user);

Mono<User> updateWithoutPermission(String id, User update);

Mono<Boolean> resendEmailVerification(ResendEmailVerificationDTO resendEmailVerificationDTO, String redirectUrl);
Expand Down
3 changes: 2 additions & 1 deletion ...rver/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,8 @@ public Mono<UserSignupDTO> createUser(User user) {
* @param user User object representing the user to be created/enabled.
* @return Publishes the user object, after having been saved.
*/
private Mono<User> signupIfAllowed(User user) {
@Override
public Mono<User> signupIfAllowed(User user) {
boolean isAdminUser = false;

if (!commonConfig.getAdminEmails().contains(user.getEmail())) {
Expand Down