Feat/teacher profile form 선생님 구글폼 프로필 등록 #4
Conversation
WalkthroughThis pull request introduces functionality for teacher profile management with AWS S3 integration. The changes enable teachers to upload profile images by adding new classes and methods across multiple layers of the application. The implementation includes a new endpoint in the Changes
Sequence DiagramsequenceDiagram
participant Controller as TeacherController
participant UseCase as TeacherManageUseCase
participant GetService as TeacherGetService
participant S3Service as S3UploadService
participant UpdateService as TeacherUpdateService
Controller->>UseCase: saveTeacherProfile(profile, request)
UseCase->>GetService: byPhoneNumber(phoneNumber)
GetService-->>UseCase: Teacher
UseCase->>S3Service: saveProfileFile(profile)
S3Service-->>UseCase: profileUrl
UseCase->>UpdateService: updateProfile(teacher, profileUrl)
UpdateService->>UseCase: profile updated
Possibly Related PRs
Poem
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
src/main/java/com/yedu/backend/global/config/s3/S3Config.java (1)
22-30: Consider using builder pattern consistently.The type casting to AmazonS3Client is unnecessary since the builder already returns the correct type.
Apply this diff to improve the code:
@Bean - public AmazonS3Client amazonS3Client() { + public AmazonS3 amazonS3Client() { AWSCredentials credentials = new BasicAWSCredentials(accessKey, secretKey); - return (AmazonS3Client) - AmazonS3ClientBuilder.standard() + return AmazonS3ClientBuilder.standard() .withCredentials(new AWSStaticCredentialsProvider(credentials)) .withRegion(region) .build(); }src/main/java/com/yedu/backend/domain/teacher/domain/entity/TeacherClassInfo.java (1)
43-46: Track TODO comment in issue tracker.The TODO comment about adding video functionality should be tracked in your issue management system.
Would you like me to create an issue to track this TODO item for implementing the video update functionality? I can help generate a detailed issue description with acceptance criteria.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
build.gradle(1 hunks)src/main/java/com/yedu/backend/domain/teacher/application/dto/req/TeacherProfileFormRequest.java(1 hunks)src/main/java/com/yedu/backend/domain/teacher/application/usecase/TeacherManageUseCase.java(3 hunks)src/main/java/com/yedu/backend/domain/teacher/domain/entity/Teacher.java(1 hunks)src/main/java/com/yedu/backend/domain/teacher/domain/entity/TeacherClassInfo.java(1 hunks)src/main/java/com/yedu/backend/domain/teacher/domain/repository/TeacherRepository.java(1 hunks)src/main/java/com/yedu/backend/domain/teacher/domain/service/TeacherGetService.java(1 hunks)src/main/java/com/yedu/backend/domain/teacher/domain/service/TeacherUpdateService.java(1 hunks)src/main/java/com/yedu/backend/domain/teacher/presentation/TeacherController.java(2 hunks)src/main/java/com/yedu/backend/global/config/s3/S3Config.java(1 hunks)src/main/java/com/yedu/backend/global/config/s3/S3UploadService.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/com/yedu/backend/domain/teacher/application/dto/req/TeacherProfileFormRequest.java
🔇 Additional comments (3)
src/main/java/com/yedu/backend/domain/teacher/presentation/TeacherController.java (1)
22-22: Add CORS configuration if needed.Consider adding CORS configuration if the form is hosted on a different domain.
Run this script to check if CORS is configured:
✅ Verification successful
CORS is already configured but needs review for production
The application has a global CORS configuration in
SecurityConfig.javathat allows requests from all origins (*). While this works for development, consider restricting allowed origins to specific domains in production for better security.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for CORS configuration in the codebase # Look for CORS configuration echo "Checking for CORS configuration..." rg -l "CrossOrigin|addCorsMappings|CorsConfiguration" # Look for security configuration echo "Checking for security configuration..." rg -l "WebSecurityConfigurerAdapter|SecurityFilterChain|WebSecurityCustomizer"Length of output: 450
Script:
#!/bin/bash # Examine the CORS configuration implementation echo "Content of SecurityConfig.java:" cat src/main/java/com/yedu/backend/global/config/security/SecurityConfig.javaLength of output: 3019
src/main/java/com/yedu/backend/domain/teacher/domain/entity/Teacher.java (1)
54-56: LGTM!The updateProfile method correctly delegates to teacherClassInfo.
src/main/java/com/yedu/backend/domain/teacher/application/usecase/TeacherManageUseCase.java (1)
25-27: LGTM! Dependencies are properly injected.The new services are correctly injected using constructor injection via
@RequiredArgsConstructor.
| @Service | ||
| public class TeacherUpdateService { | ||
| public void updateProfile(Teacher teacher, String profile) { | ||
| teacher.updateProfile(profile); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add transaction management and error handling.
The service implementation should include:
@Transactionalannotation for database operations- Error handling for failed profile updates
Apply this diff:
@Service
+@Transactional
public class TeacherUpdateService {
public void updateProfile(Teacher teacher, String profile) {
+ if (teacher == null) {
+ throw new IllegalArgumentException("Teacher cannot be null");
+ }
+ if (profile == null || profile.isBlank()) {
+ throw new IllegalArgumentException("Profile cannot be null or blank");
+ }
teacher.updateProfile(profile);
}
}📝 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.
| @Service | |
| public class TeacherUpdateService { | |
| public void updateProfile(Teacher teacher, String profile) { | |
| teacher.updateProfile(profile); | |
| } | |
| } | |
| @Service | |
| @Transactional | |
| public class TeacherUpdateService { | |
| public void updateProfile(Teacher teacher, String profile) { | |
| if (teacher == null) { | |
| throw new IllegalArgumentException("Teacher cannot be null"); | |
| } | |
| if (profile == null || profile.isBlank()) { | |
| throw new IllegalArgumentException("Profile cannot be null or blank"); | |
| } | |
| teacher.updateProfile(profile); | |
| } | |
| } |
|
|
||
| public interface TeacherRepository extends JpaRepository<Teacher, Long> { | ||
| Optional<Teacher> findByTeacherInfo_PhoneNumber(String PhoneNumber); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix parameter naming convention and consider adding an index.
- Parameter name should be camelCase
- Consider adding an index on the phone_number column for better query performance
Apply this diff:
- Optional<Teacher> findByTeacherInfo_PhoneNumber(String PhoneNumber);
+ Optional<Teacher> findByTeacherInfo_PhoneNumber(String phoneNumber);Add the following annotation to the phoneNumber field in TeacherInfo class:
@Column(name = "phone_number", index = true)
private String phoneNumber;| public Teacher byPhoneNumber(String phoneNumber) { | ||
| return teacherRepository.findByTeacherInfo_PhoneNumber(phoneNumber) | ||
| .orElseThrow(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and add phone number validation.
- Use custom exception with meaningful message
- Validate phone number format before query
- Add logging for debugging purposes
Apply this diff:
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
@Service
@RequiredArgsConstructor
public class TeacherGetService {
+ private static final Logger log = LoggerFactory.getLogger(TeacherGetService.class);
private final TeacherRepository teacherRepository;
public Teacher byPhoneNumber(String phoneNumber) {
+ if (phoneNumber == null || !phoneNumber.matches("^\\d{10,11}$")) {
+ throw new IllegalArgumentException("Invalid phone number format");
+ }
+ log.debug("Searching for teacher with phone number: {}", phoneNumber);
return teacherRepository.findByTeacherInfo_PhoneNumber(phoneNumber)
- .orElseThrow();
+ .orElseThrow(() ->
+ new TeacherNotFoundException("Teacher not found with phone number: " + phoneNumber));
}
}Committable suggestion skipped: line range outside the PR's diff.
| @PostMapping("/form/profile") | ||
| public ResponseEntity saveProfileByForm(@RequestPart TeacherProfileFormRequest request, @RequestPart MultipartFile profile) { | ||
| manageUseCase.saveTeacherProfile(profile, request); | ||
| return ResponseEntity.ok().build(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation, documentation and proper response type.
- Add file size and type validation
- Add Swagger/OpenAPI documentation
- Specify response type and error responses
Apply this diff:
+ @Operation(summary = "Save teacher profile from form submission")
+ @ApiResponses(value = {
+ @ApiResponse(responseCode = "200", description = "Profile saved successfully"),
+ @ApiResponse(responseCode = "400", description = "Invalid request"),
+ @ApiResponse(responseCode = "404", description = "Teacher not found"),
+ @ApiResponse(responseCode = "500", description = "Internal server error")
+ })
@PostMapping("/form/profile")
- public ResponseEntity saveProfileByForm(@RequestPart TeacherProfileFormRequest request, @RequestPart MultipartFile profile) {
+ public ResponseEntity<Void> saveProfileByForm(
+ @RequestPart @Valid TeacherProfileFormRequest request,
+ @RequestPart @ValidFile(maxSize = 5 * 1024 * 1024, types = {"image/jpeg", "image/png"}) MultipartFile profile) {
manageUseCase.saveTeacherProfile(profile, request);
return ResponseEntity.ok().build();
}Create a custom validator annotation:
@Target({ElementType.PARAMETER})
@Retention(RetentionPolicy.RUNTIME)
@Constraint(validatedBy = FileValidator.class)
public @interface ValidFile {
String message() default "Invalid file";
Class<?>[] groups() default {};
Class<? extends Payload>[] payload() default {};
long maxSize() default 1024 * 1024;
String[] types() default {};
}| @Value("${cloud.aws.credentials.access-key}") | ||
| public String accessKey; | ||
| @Value("${cloud.aws.credentials.secret-key}") | ||
| public String secretKey; | ||
| @Value("${cloud.aws.region.static}") | ||
| public String region; |
There was a problem hiding this comment.
Enhance security by making fields private and using AWS credential provider chain.
The configuration fields should not be public. Additionally, consider using AWS SDK's default credential provider chain instead of explicitly storing credentials.
Apply this diff to improve security:
- public String accessKey;
- public String secretKey;
- public String region;
+ private String accessKey;
+ private String secretKey;
+ private String region;Consider using AWS SDK's default credential provider chain by:
- Setting up AWS credentials in
~/.aws/credentials - Using environment variables (
AWS_ACCESS_KEY_ID,AWS_SECRET_ACCESS_KEY) - Using IAM roles for EC2 instances
📝 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.
| @Value("${cloud.aws.credentials.access-key}") | |
| public String accessKey; | |
| @Value("${cloud.aws.credentials.secret-key}") | |
| public String secretKey; | |
| @Value("${cloud.aws.region.static}") | |
| public String region; | |
| @Value("${cloud.aws.credentials.access-key}") | |
| private String accessKey; | |
| @Value("${cloud.aws.credentials.secret-key}") | |
| private String secretKey; | |
| @Value("${cloud.aws.region.static}") | |
| private String region; |
| public void deleteProfileImage(String url) { | ||
| try { | ||
| String decodedUrl = URLDecoder.decode(url, "UTF-8"); | ||
| String fileName = decodedUrl.substring(decodedUrl.lastIndexOf('/') + 1); | ||
| amazonS3.deleteObject(bucket + "/" + profile, fileName); | ||
| } catch (UnsupportedEncodingException e) { | ||
| throw new IllegalArgumentException(); //todo : 적절한 예외 처리 | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix path construction in deleteProfileImage method.
The bucket path construction is incorrect and the error handling needs improvement.
Apply this diff:
public void deleteProfileImage(String url) {
try {
String decodedUrl = URLDecoder.decode(url, "UTF-8");
String fileName = decodedUrl.substring(decodedUrl.lastIndexOf('/') + 1);
- amazonS3.deleteObject(bucket + "/" + profile, fileName);
+ amazonS3.deleteObject(bucket, profile + "/" + fileName);
} catch (UnsupportedEncodingException e) {
- throw new IllegalArgumentException(); //todo : 적절한 예외 처리
+ throw new FileDeleteException("Failed to decode file URL", e);
+ } catch (AmazonS3Exception e) {
+ throw new FileDeleteException("Failed to delete file from S3", e);
}
}📝 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.
| public void deleteProfileImage(String url) { | |
| try { | |
| String decodedUrl = URLDecoder.decode(url, "UTF-8"); | |
| String fileName = decodedUrl.substring(decodedUrl.lastIndexOf('/') + 1); | |
| amazonS3.deleteObject(bucket + "/" + profile, fileName); | |
| } catch (UnsupportedEncodingException e) { | |
| throw new IllegalArgumentException(); //todo : 적절한 예외 처리 | |
| } | |
| } | |
| public void deleteProfileImage(String url) { | |
| try { | |
| String decodedUrl = URLDecoder.decode(url, "UTF-8"); | |
| String fileName = decodedUrl.substring(decodedUrl.lastIndexOf('/') + 1); | |
| amazonS3.deleteObject(bucket, profile + "/" + fileName); | |
| } catch (UnsupportedEncodingException e) { | |
| throw new FileDeleteException("Failed to decode file URL", e); | |
| } catch (AmazonS3Exception e) { | |
| throw new FileDeleteException("Failed to delete file from S3", e); | |
| } | |
| } |
| private String getString(MultipartFile multipartFile, String dir) { | ||
| try { | ||
| String originalFilename = multipartFile.getOriginalFilename(); | ||
| String fileName = dir +"/" + originalFilename + UUID.randomUUID(); | ||
|
|
||
| ObjectMetadata metadata = new ObjectMetadata(); | ||
| metadata.setContentLength(multipartFile.getSize()); | ||
| metadata.setContentType(multipartFile.getContentType()); | ||
|
|
||
| amazonS3.putObject(bucket, fileName, multipartFile.getInputStream(), metadata); | ||
| return amazonS3.getUrl(bucket, fileName).toString(); | ||
| } catch (IOException ex) { | ||
| throw new IllegalArgumentException(); //todo : 적절한 예외 처리 | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve file upload security and error handling.
Several improvements needed in the file upload logic:
- File name collision possible with UUID appended after original name
- Missing file size limits
- Generic exception handling
- Missing content type validation
Apply these improvements:
private String getString(MultipartFile multipartFile, String dir) {
+ if (multipartFile.getSize() > 5_000_000) { // 5MB limit
+ throw new FileSizeLimitExceededException("File size exceeds limit", multipartFile.getSize(), 5_000_000);
+ }
+ String contentType = multipartFile.getContentType();
+ if (contentType == null || !contentType.startsWith("image/")) {
+ throw new InvalidFileTypeException("Only image files are allowed");
+ }
try {
String originalFilename = multipartFile.getOriginalFilename();
- String fileName = dir +"/" + originalFilename + UUID.randomUUID();
+ String extension = originalFilename.substring(originalFilename.lastIndexOf("."));
+ String fileName = dir + "/" + UUID.randomUUID() + extension;
ObjectMetadata metadata = new ObjectMetadata();
metadata.setContentLength(multipartFile.getSize());
metadata.setContentType(multipartFile.getContentType());
amazonS3.putObject(bucket, fileName, multipartFile.getInputStream(), metadata);
return amazonS3.getUrl(bucket, fileName).toString();
} catch (IOException ex) {
- throw new IllegalArgumentException(); //todo : 적절한 예외 처리
+ throw new FileUploadException("Failed to upload file to S3", ex);
}
}Committable suggestion skipped: line range outside the PR's diff.
| public void saveTeacherProfile(MultipartFile profile, TeacherProfileFormRequest request) { | ||
| String profileUrl = s3UploadService.saveProfileFile(profile); | ||
| Teacher teacher = teacherGetService.byPhoneNumber(request.phoneNumber()); | ||
| teacherUpdateService.updateProfile(teacher, profileUrl); | ||
| } |
There was a problem hiding this comment.
Add error handling for file upload and teacher lookup.
The method needs to handle potential failure scenarios:
- Invalid or empty profile file
- S3 upload failures
- Teacher not found by phone number
Consider adding proper error handling:
public void saveTeacherProfile(MultipartFile profile, TeacherProfileFormRequest request) {
+ if (profile == null || profile.isEmpty()) {
+ throw new IllegalArgumentException("Profile image is required");
+ }
+
String profileUrl = s3UploadService.saveProfileFile(profile);
Teacher teacher = teacherGetService.byPhoneNumber(request.phoneNumber());
+
+ if (teacher == null) {
+ throw new EntityNotFoundException("Teacher not found with phone number: " + request.phoneNumber());
+ }
+
teacherUpdateService.updateProfile(teacher, profileUrl);
}📝 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.
| public void saveTeacherProfile(MultipartFile profile, TeacherProfileFormRequest request) { | |
| String profileUrl = s3UploadService.saveProfileFile(profile); | |
| Teacher teacher = teacherGetService.byPhoneNumber(request.phoneNumber()); | |
| teacherUpdateService.updateProfile(teacher, profileUrl); | |
| } | |
| public void saveTeacherProfile(MultipartFile profile, TeacherProfileFormRequest request) { | |
| if (profile == null || profile.isEmpty()) { | |
| throw new IllegalArgumentException("Profile image is required"); | |
| } | |
| String profileUrl = s3UploadService.saveProfileFile(profile); | |
| Teacher teacher = teacherGetService.byPhoneNumber(request.phoneNumber()); | |
| if (teacher == null) { | |
| throw new EntityNotFoundException("Teacher not found with phone number: " + request.phoneNumber()); | |
| } | |
| teacherUpdateService.updateProfile(teacher, profileUrl); | |
| } |
|
|
||
| // aws s3 | ||
| implementation 'org.springframework.cloud:spring-cloud-starter-aws:2.2.6.RELEASE' |
There was a problem hiding this comment.
Fix Spring Cloud AWS version compatibility with Spring Boot 3.x.
The current Spring Cloud AWS starter version (2.2.6.RELEASE) is not compatible with Spring Boot 3.x. This will cause runtime issues.
Replace with the compatible AWS SDK dependency:
- implementation 'org.springframework.cloud:spring-cloud-starter-aws:2.2.6.RELEASE'
+ implementation 'io.awspring.cloud:spring-cloud-aws-starter-s3:3.0.2'This version is specifically designed for Spring Boot 3.x and provides better integration with AWS S3.
📝 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.
| // aws s3 | |
| implementation 'org.springframework.cloud:spring-cloud-starter-aws:2.2.6.RELEASE' | |
| // aws s3 | |
| implementation 'io.awspring.cloud:spring-cloud-aws-starter-s3:3.0.2' |
🐈 PR 요약
✨ PR 상세
build.gradle추가🚨 참고사항
✅ 체크리스트
Summary by CodeRabbit
New Features
Dependencies
Improvements