Conversation
[Feature] Oauth 유저 조회 방법 수정
[Feature] Template 검색 API
[Feature] 폴더 이동, 이름 수정, 조회, 삭제 API 구현
[Feature] 이메일 인증 여부 반환 api 구현
[Feature] 전체 허용과 편집 허용 분리
[Fix] ImageUrl 어노테이션 수정 및 마이페이지 로직 변경
[Fix] 변수명 및 매개변수 순서 변경
[Feature] 누끼따기 및 첫 템플릿 합성 API 구현
Summary of ChangesHello @seoahS01, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 AI 기반 이미지 처리 기능을 도입하고, 기존 폴더 관리 시스템을 대폭 개선하며, 인증 관련 기능을 확장합니다. 사용자들은 이제 AI를 활용하여 이미지 배경을 제거하거나 템플릿에 합성할 수 있으며, 폴더를 더욱 유연하게 관리(삭제, 이름 변경, 이동, 하위 폴더 조회)할 수 있게 됩니다. 또한, 이메일 인증 상태를 확인하는 새로운 API가 추가되었고, Google OAuth 사용자 식별 방식이 업데이트되었습니다. 이러한 변경 사항들은 전반적인 시스템의 기능성과 유용성을 크게 향상시킬 것입니다. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant number of new features, including image generation capabilities with AI, project management, and extensive enhancements to the folder and permission system. The overall direction is good, with many improvements to the data model and API design. However, I've identified a critical security vulnerability where an endpoint exposes all users' projects, which must be addressed immediately. I've also found some high-severity performance issues related to database operations that should be fixed. Additionally, there are several opportunities for medium-severity improvements regarding API design, code clarity, and removal of redundant code. Please review the detailed comments for specific suggestions.
| public List<ProjectDashboardResponse> getProjects() { | ||
| return projectRepository.findAll().stream() | ||
| .map(ProjectDashboardResponse::from) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
This method fetches and returns all projects from all users by calling projectRepository.findAll(). This is a critical security vulnerability as it exposes potentially sensitive user data to anyone who calls this endpoint. The query must be scoped to retrieve only the projects belonging to the currently authenticated user. You should pass the user's ID to this method and update the repository to filter projects based on user ownership or permissions.
| Long existsReadableDescendant(@Param("folderId") Long folderId, | ||
| @Param("aclRootFolderId") Long aclRootFolderId, | ||
| @Param("userId") Long userId); |
There was a problem hiding this comment.
The method parameter aclRootFolderId is declared but not used within the native query. The query instead relies on fd.acl_root_folder_id fetched from a join. This discrepancy can lead to confusion and potential bugs. Please either utilize the :aclRootFolderId parameter in your query or remove it from the method signature if it's truly not needed.
| for (Long id : subFolderIds) { | ||
| folderRepository.deleteById(id); | ||
| folderRepository.flush(); | ||
| } |
There was a problem hiding this comment.
Deleting folders one by one within a loop and flushing after each deletion is inefficient and can lead to significant performance degradation, especially for folders with many sub-items. The FolderRepository already provides a deleteAllByIdsIn(List<Long> folderIds) method for bulk deletion. Using this method will perform the deletion in a single, much more efficient database operation.
folderRepository.deleteAllByIdsIn(subFolderIds);| public static EmailValidationResponse create(String email, boolean isVerified) { | ||
| return new EmailValidationResponse(email, isVerified); | ||
| } |
There was a problem hiding this comment.
The static factory method create is redundant for a Java record. Records automatically provide a public canonical constructor, so you can instantiate this record directly using new EmailValidationResponse(email, isVerified). Removing this method will result in cleaner and more idiomatic code. This feedback also applies to FoldersDto and FoldersResponse.
| } | ||
|
|
||
| public EmailValidationResponse getEmailValidation(String email) { | ||
| boolean isAvailable = verifiedEmailRepository.findById(email).isPresent(); |
There was a problem hiding this comment.
The variable name isAvailable is misleading in this context. The method checks if an email has been verified. Renaming the variable to isVerified would make the code's intent clearer and align it with the field name in the EmailValidationResponse DTO.
| boolean isAvailable = verifiedEmailRepository.findById(email).isPresent(); | |
| boolean isVerified = verifiedEmailRepository.findById(email).isPresent(); |
References
- In a DTO, a boolean field representing email verification status should be named
isVerifiedfor clarity, rather than a more generic name likeisAvailable.
| public void folderManagerUpdate( | ||
| @AuthenticationPrincipal JwtUserPrincipal principal, | ||
| @PathVariable Long folderId, | ||
| @PathVariable Long targetUserId | ||
| @PathVariable Long targetUserId, | ||
| @RequestParam Permission permission | ||
| ) { | ||
| folderManagerService.updatePermission(principal.userId(), targetUserId, folderId); | ||
| folderManagerService.updatePermission(principal.userId(), targetUserId, folderId, permission); | ||
| } |
There was a problem hiding this comment.
Using PUT with @RequestParam is not a standard RESTful practice for updates. PUT requests are typically expected to contain the resource's state in the request body. To make this endpoint more REST-compliant, consider passing the permission in the request body.
For example:
// DTO
public record UpdatePermissionRequest(
@NotNull Permission permission
) {}
// Controller method
@PutMapping("/v1/folder/manager/{folderId}/{targetUserId}")
public void folderManagerUpdate(
@AuthenticationPrincipal JwtUserPrincipal principal,
@PathVariable Long folderId,
@PathVariable Long targetUserId,
@RequestBody @Valid UpdatePermissionRequest request
) {
folderManagerService.updatePermission(principal.userId(), targetUserId, folderId, request.permission());
}| public static FoldersDto create( | ||
| Long folderId, | ||
| String folderName, | ||
| List<String> images | ||
| ) { | ||
| return new FoldersDto( | ||
| folderId, | ||
| folderName, | ||
| images | ||
| ); | ||
| } |
There was a problem hiding this comment.
The static factory method create is redundant for a Java record. Records automatically provide a public canonical constructor. You can directly instantiate the record using new FoldersDto(...), which makes the code more concise and idiomatic.
| public static FoldersResponse create(List<FoldersDto> folders, PageInfo pageInfo) { | ||
| return new FoldersResponse(folders, pageInfo); | ||
| } |
There was a problem hiding this comment.
The static factory method create is redundant for a Java record. Records automatically provide a public canonical constructor. You can directly instantiate the record using new FoldersResponse(...), which makes the code more concise and idiomatic.
|
|
||
| return folderManagerRepository.save( | ||
| FolderManager.create(targetUser, folder, permission) | ||
| FolderManager.create(targetUser, folder, userPermission) |
There was a problem hiding this comment.
When creating a new FolderManager entry, it is first created with the user's existing inherited permission (userPermission) and then immediately updated to the new permission in a subsequent step. This results in an unnecessary UPDATE query right after an INSERT. You can optimize this by creating the FolderManager directly with the new permission from the request.
| FolderManager.create(targetUser, folder, userPermission) | |
| FolderManager.create(targetUser, folder, permission) |
| @Configuration | ||
| @EnableConfigurationProperties(GeminiProperties.class) | ||
| public class AiPropertiesConfig { |
There was a problem hiding this comment.
This configuration class is redundant. The main application class, StudioxBeApplication, is already annotated with @ConfigurationPropertiesScan, which automatically discovers and registers @ConfigurationProperties beans like GeminiProperties. You can safely remove this class to simplify the configuration.
2 participants
#️⃣ 연관된 이슈
#️⃣ 작업 내용
#️⃣ 테스트 결과
#️⃣ 변경 사항 체크리스트
#️⃣ 스크린샷 (선택)
#️⃣ 리뷰 요구사항 (선택)
📎 참고 자료 (선택)