Profile Service#28
Open
men229 wants to merge 1 commit into
Open
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds a complete user profile update feature with avatar support. It introduces a new authenticated ChangesUser Profile Update Feature
Sequence DiagramsequenceDiagram
participant Client
participant ProfileController
participant ProfileService
participant IdentityService
participant UserRepository
participant UserMapper
participant S3Service
Client->>ProfileController: GET /api/v1/users/me
ProfileController->>ProfileService: getCurrentUserProfile()
ProfileService->>IdentityService: getCurrentUserId()
ProfileService->>UserRepository: findById(userId)
ProfileService->>UserMapper: toUserResponse(userEntity)
UserMapper->>S3Service: buildPresignedDownloadUrl(avatarS3Key)
UserMapper->>ProfileController: UserResponse
ProfileController->>Client: 200 OK UserResponse
Client->>ProfileController: PATCH /api/v1/users/me (with avatarUpdated=true)
ProfileController->>ProfileService: updateCurrentUserProfile(request)
ProfileService->>UserRepository: existsByUsername(newUsername)
ProfileService->>UserRepository: save(updatedEntity)
ProfileService->>S3Service: generatePresignedUrlForUserAvatar(userId)
ProfileService->>ProfileController: UserAvatarResponse with uploadUrl
ProfileController->>Client: 200 OK UserAvatarResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend/smartjam-api/src/main/java/com/smartjam/smartjamapi/service/ProfileService.java`:
- Around line 48-53: The current check uses
repository.existsByUsername(request.username()) and will throw
UserAlreadyExistsException even if the user re-submits their own username;
change the logic to ignore the current user's record by either first comparing
request.username() to user.getUsername() and skipping assignment when unchanged,
or replace the existence check with a repository call that excludes the current
user (e.g. existsByUsernameAndIdNot(request.username(), user.getId())) and only
throw if that returns true; then call user.setUsername(request.username()) only
when the username is different and not taken by another user.
- Around line 63-68: The avatar key returned by
s3Service.generatePresignedUrlForUserAvatar is not being persisted, so user
profiles lack a durable pointer; update the avatar-update flow in
ProfileService: when request.avatarUpdated() is true, obtain the key via
s3Service.getAvatarsKey(userId), persist that key to the user's record (e.g.,
via the user repository or UserService update method) and then return
UserAvatarResponse with the URL; if you need upload-confirmed semantics, split
into two endpoints (issue presigned PUT URL, then a confirm endpoint that
persists the key), ensuring the persisted field matches what
s3Service.generatePresignedUrlForUserAvatar expects.
In
`@backend/smartjam-api/src/main/java/com/smartjam/smartjamapi/service/S3Service.java`:
- Around line 85-92: determineBucket currently defaults any unexpected key to
the avatars bucket; change it to validate the key and explicitly map only known
prefixes ("submissions/", "references/", "avatars/") and throw an
IllegalArgumentException (or similar unchecked exception) when key is null or
has an unknown prefix so callers fail fast; update the determineBucket method to
check for "avatars/" as a valid branch and replace the final return with a throw
new IllegalArgumentException(...) that includes the offending key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e6f1d93-979c-4579-b8dc-eba093615fb0
📒 Files selected for processing (7)
backend/smartjam-api/src/main/java/com/smartjam/smartjamapi/config/MinioProperties.javabackend/smartjam-api/src/main/java/com/smartjam/smartjamapi/controller/ProfileController.javabackend/smartjam-api/src/main/java/com/smartjam/smartjamapi/mapper/UserMapper.javabackend/smartjam-api/src/main/java/com/smartjam/smartjamapi/service/ProfileService.javabackend/smartjam-api/src/main/java/com/smartjam/smartjamapi/service/S3Service.javabackend/smartjam-api/src/main/resources/application.yamlopenapi-spec/api.yaml
Comment on lines
+48
to
+53
| if (request.username() != null && !request.username().isBlank()) { | ||
| if (repository.existsByUsername(request.username())) { | ||
| throw new UserAlreadyExistsException("Username already exists"); | ||
| } | ||
| user.setUsername(request.username()); | ||
| } |
There was a problem hiding this comment.
Avoid false 409 when username is unchanged.
If Line 48 sends the same username the user already owns, Line 49 still returns true and throws conflict. Compare against current username first (or query excluding current user ID).
Suggested fix
- if (request.username() != null && !request.username().isBlank()) {
- if (repository.existsByUsername(request.username())) {
+ if (request.username() != null && !request.username().isBlank()) {
+ String newUsername = request.username().trim();
+ if (!newUsername.equals(user.getUsername()) && repository.existsByUsername(newUsername)) {
throw new UserAlreadyExistsException("Username already exists");
}
- user.setUsername(request.username());
+ user.setUsername(newUsername);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend/smartjam-api/src/main/java/com/smartjam/smartjamapi/service/ProfileService.java`
around lines 48 - 53, The current check uses
repository.existsByUsername(request.username()) and will throw
UserAlreadyExistsException even if the user re-submits their own username;
change the logic to ignore the current user's record by either first comparing
request.username() to user.getUsername() and skipping assignment when unchanged,
or replace the existence check with a repository call that excludes the current
user (e.g. existsByUsernameAndIdNot(request.username(), user.getId())) and only
throw if that returns true; then call user.setUsername(request.username()) only
when the username is different and not taken by another user.
Comment on lines
+63
to
+68
| if (request.avatarUpdated() != null && request.avatarUpdated()) { | ||
| String key = s3Service.getAvatarsKey(userId); | ||
| avatarUrl = s3Service.generatePresignedUrlForUserAvatar(key); | ||
| } | ||
| // TODO: обновить key в бд, нужна новая ручка|валидатор | ||
| return new UserAvatarResponse(avatarUrl); |
There was a problem hiding this comment.
Persist avatar key as part of the avatar-update flow.
Line 67 confirms the key is not stored. That leaves profile/avatar retrieval inconsistent after upload because no durable pointer is saved on the user.
Suggested direction
if (request.avatarUpdated() != null && request.avatarUpdated()) {
String key = s3Service.getAvatarsKey(userId);
+ user.setAvatarUrl(key);
avatarUrl = s3Service.generatePresignedUrlForUserAvatar(key);
}
- // TODO: обновить key в бд, нужна новая ручка|валидатор
return new UserAvatarResponse(avatarUrl);If you need “upload-confirmed” semantics, split into two endpoints: (1) issue presigned PUT URL, (2) confirm upload + persist key.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend/smartjam-api/src/main/java/com/smartjam/smartjamapi/service/ProfileService.java`
around lines 63 - 68, The avatar key returned by
s3Service.generatePresignedUrlForUserAvatar is not being persisted, so user
profiles lack a durable pointer; update the avatar-update flow in
ProfileService: when request.avatarUpdated() is true, obtain the key via
s3Service.getAvatarsKey(userId), persist that key to the user's record (e.g.,
via the user repository or UserService update method) and then return
UserAvatarResponse with the URL; if you need upload-confirmed semantics, split
into two endpoints (issue presigned PUT URL, then a confirm endpoint that
persists the key), ensuring the persisted field matches what
s3Service.generatePresignedUrlForUserAvatar expects.
Comment on lines
+85
to
92
| private String determineBucket(String key) { | ||
| if (key != null && key.startsWith("submissions/")) { | ||
| return minioProperties.getBuckets().getSubmissions(); | ||
| } else if (key != null && key.startsWith("references/")) { | ||
| return minioProperties.getBuckets().getReferences(); | ||
| } | ||
| return minioProperties.getBuckets().getAvatars(); | ||
| } |
There was a problem hiding this comment.
Fail fast on unknown S3 key prefixes instead of defaulting to avatars.
Line 91 currently routes any unexpected prefix to the avatars bucket, which can produce incorrect presigned URLs and mask upstream key bugs. Treat unknown prefixes as invalid input.
Suggested fix
private String determineBucket(String key) {
if (key != null && key.startsWith("submissions/")) {
return minioProperties.getBuckets().getSubmissions();
} else if (key != null && key.startsWith("references/")) {
return minioProperties.getBuckets().getReferences();
+ } else if (key != null && key.startsWith("avatars/")) {
+ return minioProperties.getBuckets().getAvatars();
}
- return minioProperties.getBuckets().getAvatars();
+ throw new IllegalArgumentException("Unknown S3 key prefix: " + key);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend/smartjam-api/src/main/java/com/smartjam/smartjamapi/service/S3Service.java`
around lines 85 - 92, determineBucket currently defaults any unexpected key to
the avatars bucket; change it to validate the key and explicitly map only known
prefixes ("submissions/", "references/", "avatars/") and throw an
IllegalArgumentException (or similar unchecked exception) when key is null or
has an unknown prefix so callers fail fast; update the determineBucket method to
check for "avatars/" as a valid branch and replace the final return with a throw
new IllegalArgumentException(...) that includes the offending key.
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
2 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.
No description provided.