✨ Feature/#27 - 4.4 가게 방문 인증용 영수증 OCR 조회하기 API 구현 #36
Conversation
WalkthroughThe changes introduce a new API endpoint for receipt Optical Character Recognition (OCR) functionality and enhance existing components related to this feature. A new controller, service, and DTOs are added to manage the OCR process, including handling requests and responses. The error handling capabilities are expanded with a new error code for OCR recognition failures. Additionally, utility classes are modified to support the new functionality, and the API documentation is updated to reflect these changes. Changes
Assessment against linked issues
Possibly related issues
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
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: 15
🧹 Outside diff range and nitpick comments (17)
src/main/java/com/daon/onjung/onjung/application/usecase/ReceiptOCRUseCase.java (1)
7-10: LGTM! Consider adding interface documentation.The interface follows clean architecture principles and has a clear single responsibility. Consider adding Javadoc to document:
- Expected file formats and size limits
- Possible exceptions that might be thrown
- Example usage
@UseCase +/** + * Use case for processing receipt images through OCR. + * + * @throws IllegalArgumentException if the file is empty or invalid + * @throws OCRProcessingException if OCR processing fails + */ public interface ReceiptOCRUseCase { + /** + * Processes the receipt image using OCR. + * + * @param file the receipt image file (supported formats: JPEG, PNG) + * @return DTO containing the extracted receipt data + */ ReceiptOCRResponseDto execute(MultipartFile file); }src/main/java/com/daon/onjung/onjung/application/controller/command/OnjungCommandV1Controller.java (2)
12-16: Consider improving class naming and API path structureWhile the implementation is correct, consider these improvements:
- The class name
OnjungCommandV1Controlleris too generic. Consider renaming toReceiptOCRCommandControllerto better reflect its specific responsibility.- Add a class-level
@RequestMapping("/api/v1")to avoid path repetition and make versioning easier to maintain.@RestController @RequiredArgsConstructor -public class OnjungCommandV1Controller { +@RequestMapping("/api/v1") +public class ReceiptOCRCommandController {
1-27: Consider Docker configuration for OCR serviceSince this controller is part of a store visit verification system and Docker testing is pending, ensure that:
- The OCR service dependencies are properly configured in the Docker environment
- Resource limits are set appropriately for OCR processing
- Temporary storage for uploaded files is configured correctly
Consider adding the following to your Docker configuration:
- Memory limits suitable for OCR processing
- Temporary storage volume for file uploads
- Health check endpoint for the OCR service
src/main/java/com/daon/onjung/onjung/application/service/ReceiptOCRService.java (1)
1-36: Consider async processing and resilience patternsSince OCR processing can be time-consuming and relies on external services, consider these architectural improvements:
- Implement async processing using
@Asyncor reactive programming- Add circuit breaker pattern for external service calls
- Configure timeouts and rate limiting
- Consider implementing a retry mechanism
Would you like me to provide implementation examples for any of these suggestions?
src/main/java/com/daon/onjung/onjung/application/dto/response/ReceiptOCRResponseDto.java (2)
25-33: Improve constructor robustness.The constructor should validate parameters before object construction to prevent partially initialized objects.
@Builder public ReceiptOCRResponseDto(String storeName, String storeAddress, String paymentDate, String paymentAmount) { + validateParameters(storeName, storeAddress, paymentDate, paymentAmount); this.storeName = storeName; this.storeAddress = storeAddress; this.paymentDate = paymentDate; - this.paymentAmount = paymentAmount; + this.paymentAmount = new BigDecimal(paymentAmount); this.validateSelf(); } + + private void validateParameters(String storeName, String storeAddress, String paymentDate, String paymentAmount) { + if (paymentAmount != null && !paymentAmount.matches("^\\d+(\\.\\d{1,2})?$")) { + throw new IllegalArgumentException("Invalid payment amount format"); + } + // Add other parameter validations as needed + }
1-59: Consider architectural improvements.
Consider implementing caching for OCR results to improve performance and reduce API calls:
- Use a distributed cache (e.g., Redis) to store OCR results
- Cache key could be based on receipt image hash or metadata
- Set appropriate TTL based on business requirements
Add logging for better observability:
- Log OCR processing steps
- Include correlation IDs for request tracing
- Log performance metrics
Would you like me to provide implementation details for these suggestions?
src/main/java/com/daon/onjung/core/exception/error/ErrorCode.java (1)
28-29: LGTM! Consider adding error code range documentation.The new OCR error category and code follow the established patterns correctly. The error code (40010) aligns with the HTTP status (400) and maintains consistency with the existing structure.
Consider adding a comment at the top of the file documenting the error code ranges for each category to aid maintainability:
/** * Error Code Ranges: * 400xx - Invalid Argument Errors * 401xx - Unauthorized Errors * 403xx - Access Denied Errors * 404xx - Not Found Errors * 405xx - Method Not Allowed Errors * 500xx - Internal Server Errors * 502xx - External Server Errors */src/main/java/com/daon/onjung/core/utility/OCRUtil.java (3)
19-30: Add class-level documentation and validate configuration values.Consider adding:
- JavaDoc documentation explaining the class's purpose and responsibilities
- Validation for configuration values to ensure they're not null/empty at startup
@Component +@Validated public class OCRUtil { + /** + * Utility class for handling OCR (Optical Character Recognition) operations. + * Manages communication with Naver Cloud OCR service for receipt processing. + */ @Value("${naver-cloud.ocr.url}") + @NotBlank(message = "OCR URL must not be blank") private String ocrUrl; @Value("${naver-cloud.ocr.path}") + @NotBlank(message = "OCR path must not be blank") private String ocrPath; @Value("${naver-cloud.ocr.key}") + @NotBlank(message = "OCR key must not be blank") private String ocrKey;
33-37: Add URL validation and logging.The URL construction should be validated and logged for debugging purposes.
+ private static final Logger log = LoggerFactory.getLogger(OCRUtil.class); + public String createOCRRequestUrl() { - return UriComponentsBuilder.fromHttpUrl(ocrUrl) + String url = UriComponentsBuilder.fromHttpUrl(ocrUrl) .path(ocrPath) .toUriString(); + try { + new URL(url); // Validate URL format + log.debug("Created OCR request URL: {}", url); + return url; + } catch (MalformedURLException e) { + throw new IllegalStateException("Invalid OCR URL configuration", e); + } }
39-44: Enhance headers configuration.The headers setup could be more comprehensive for multipart requests.
public HttpHeaders createOCRRequestHeaders() { HttpHeaders headers = new HttpHeaders(); headers.add("X-OCR-SECRET", ocrKey); - headers.setContentType(org.springframework.http.MediaType.MULTIPART_FORM_DATA); + headers.setContentType(MediaType.MULTIPART_FORM_DATA); + headers.setAccept(Collections.singletonList(MediaType.APPLICATION_JSON)); return headers; }src/main/java/com/daon/onjung/core/utility/RestClientUtil.java (2)
36-36: Address TODO: Consider splitting responsibilitiesThe TODO comment highlights a valid concern about dependency separation. The class currently handles multiple responsibilities (HTTP operations, error handling, response mapping).
Consider:
- Extract error handling into a separate
HttpErrorHandlerclass- Create a dedicated
ResponseMapperfor JSON conversions- Keep only HTTP client operations in this class
This would improve maintainability and make the code more testable.
Line range hint
22-22: Add timeout configuration and retry mechanismThe RestClient is created without timeout configuration, which could lead to hanging requests in production.
Consider configuring the RestClient with appropriate timeouts and retry mechanism:
- private final RestClient restClient = RestClient.create(); + private final RestClient restClient = RestClient.builder() + .requestFactory(clientHttpRequestFactory()) + .build(); + + private ClientHttpRequestFactory clientHttpRequestFactory() { + HttpComponentsClientHttpRequestFactory factory = new HttpComponentsClientHttpRequestFactory(); + factory.setConnectTimeout(Duration.ofSeconds(5)); + factory.setReadTimeout(Duration.ofSeconds(10)); + return factory; + }src/main/java/com/daon/onjung/core/utility/DateTimeUtil.java (1)
24-49: Consider optimizing the date format patterns.While the list is comprehensive, consider the following improvements:
- Some patterns might be redundant as
DateTimeFormattercan handle single-digit months/days by default- Consider adding common receipt formats like:
- "dd/MM/yyyy"
- "MM/dd/yyyy"
- Time-included formats (for receipts with timestamps)
private static final List<DateTimeFormatter> DATE_FORMATTERS = List.of( DateTimeFormatter.ofPattern("yyyyMMdd"), DateTimeFormatter.ofPattern("yyyyMMdd(E)"), DateTimeFormatter.ofPattern("yyyyMMdd (E)"), DateTimeFormatter.ofPattern("yyyyMMdd E"), DateTimeFormatter.ofPattern("yyyy-MM-dd"), - DateTimeFormatter.ofPattern("yyyy-M-d"), // Redundant DateTimeFormatter.ofPattern("yyyy-MM-dd(E)"), DateTimeFormatter.ofPattern("yyyy-MM-dd (E)"), DateTimeFormatter.ofPattern("yyyy-MM-dd E"), DateTimeFormatter.ofPattern("yyyy/MM/dd"), - DateTimeFormatter.ofPattern("yyyy/M/d"), // Redundant DateTimeFormatter.ofPattern("yyyy/MM/dd(E)"), DateTimeFormatter.ofPattern("yyyy/MM/dd (E)"), DateTimeFormatter.ofPattern("yyyy/MM/dd E"), + DateTimeFormatter.ofPattern("dd/MM/yyyy"), // Common in receipts + DateTimeFormatter.ofPattern("MM/dd/yyyy"), // Common in receipts + DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss"), // For timestamps // ... rest of the patterns );src/main/java/com/daon/onjung/core/dto/ReceiptOCRDto.java (4)
6-253: Consider extracting nested records into separate files for better organizationThe
ReceiptOCRDtoclass contains multiple nested records, which can make the class lengthy and harder to navigate. For better code organization and maintainability, consider moving some of the nested records into separate class files. This approach enhances readability and adheres to best practices for class design.
77-89: Clarify the naming of thetelfield inStoreInfoIn the
StoreInforecord, the fieldtel(lines 87-88) is a list ofTextBlock, suggesting multiple telephone numbers. If only one telephone number is expected, consider changing the type to a singleTextBlock. If multiple numbers are possible, consider renaming the field totelsortelephoneNumbersfor clarity.
109-148: CombineDateInfoandTimeInfointo a unifiedDateTimeInforecordThe
DateInfo(lines 109-127) andTimeInfo(lines 130-148) records share similar structures, both representing temporal information with identical fields except for theirformattedproperty. Consider merging them into a singleDateTimeInforecord to reduce code duplication and improve maintainability.Here's a conceptual example of how you might refactor the code:
- public record DateInfo( + public record DateTimeInfo( @JsonProperty("text") String text, - @JsonProperty("formatted") - FormattedDate formatted, + @JsonProperty("formatted") + FormattedDateTime formatted, @JsonProperty("keyText") String keyText, @JsonProperty("confidenceScore") Double confidenceScore, @JsonProperty("boundingPolys") List<Polygon> boundingPolys, @JsonProperty("maskingPolys") List<Polygon> maskingPolys ) {}You would also need to adjust the
formattedproperty to accommodate both date and time formats.
197-203: Avoid naming conflicts by renaming thePricerecord or its fieldsHaving a
Pricerecord with a field namedprice(lines 198-199) can lead to confusion. To improve clarity, consider renaming the record toPriceInfoand the fields to more descriptive names, such astotalPriceandunitPrice.Apply this diff to implement the suggestion:
- public record Price( + public record PriceInfo( @JsonProperty("price") - TextBlock price, + TextBlock totalPrice, @JsonProperty("unitPrice") TextBlock unitPrice ) {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
http/onjung/OnjungControllerHttpRequest.http(1 hunks)src/main/java/com/daon/onjung/core/dto/ReceiptOCRDto.java(1 hunks)src/main/java/com/daon/onjung/core/exception/error/ErrorCode.java(1 hunks)src/main/java/com/daon/onjung/core/utility/DateTimeUtil.java(3 hunks)src/main/java/com/daon/onjung/core/utility/OCRUtil.java(1 hunks)src/main/java/com/daon/onjung/core/utility/RestClientUtil.java(3 hunks)src/main/java/com/daon/onjung/onjung/application/controller/command/OnjungCommandV1Controller.java(1 hunks)src/main/java/com/daon/onjung/onjung/application/dto/response/ReceiptOCRResponseDto.java(1 hunks)src/main/java/com/daon/onjung/onjung/application/service/ReceiptOCRService.java(1 hunks)src/main/java/com/daon/onjung/onjung/application/usecase/ReceiptOCRUseCase.java(1 hunks)
🔇 Additional comments (9)
http/onjung/OnjungControllerHttpRequest.http (2)
14-15: LGTM! Improved formatting
The added newline after the authorization header improves readability and follows HTTP request formatting conventions.
19-20: Verify multipart configuration
Ensure that the server is properly configured to handle multipart file uploads with appropriate size limits.
✅ Verification successful
Multipart configuration is properly implemented
The server is correctly configured to handle multipart file uploads:
- The controller
OnjungCommandV1Controllerproperly declares multipart handling with@PostMapping(consumes = "multipart/form-data") - The endpoint correctly uses
@RequestPartannotation withMultipartFilefor file upload handling - Spring Boot's default multipart configuration is sufficient for basic file uploads, as no custom size limits or configurations are overridden
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for multipart configuration in application properties and configuration files
echo "Searching for multipart configuration..."
rg -l "multipart" --type properties --type yaml
rg -l "MultipartConfig|@MultipartConfig" --type java
Length of output: 228
Script:
#!/bin/bash
# Search for multipart configuration with broader file patterns
echo "Searching for multipart configuration in properties and yaml files..."
fd -e properties -e yml -e yaml | xargs rg -l "multipart"
echo -e "\nSearching for multipart configuration in Java files..."
fd -e java | xargs rg -l "multipart|MultipartConfig|@MultipartConfig"
echo -e "\nSearching for Spring Boot configuration classes..."
ast-grep --pattern 'class $_ {
$$$
@Configuration
$$$
}'
Length of output: 1221
Script:
#!/bin/bash
# Check the actual multipart configuration implementation
echo "Checking OnjungCommandV1Controller implementation..."
rg -A 10 "multipart" src/main/java/com/daon/onjung/onjung/application/controller/command/OnjungCommandV1Controller.java
echo -e "\nChecking Spring configuration for multipart..."
fd -e java -e properties -e yml -e yaml | xargs rg "spring\..*multipart|@EnableConfigurationProperties"
Length of output: 962
src/main/java/com/daon/onjung/onjung/application/controller/command/OnjungCommandV1Controller.java (2)
1-11: LGTM! Clean and well-organized imports
The package structure and imports are appropriate for a REST controller implementing the command pattern.
18-26:
Add input validation and API documentation
The endpoint implementation has several security considerations that should be addressed:
- Missing input validation for the file parameter
- No file size limits specified
- No file type restrictions
- Missing API documentation annotations
Apply these security improvements:
+ @Operation(summary = "Process receipt OCR", description = "Upload receipt image for OCR processing")
+ @ApiResponse(responseCode = "200", description = "Successfully processed OCR")
+ @ApiResponse(responseCode = "400", description = "Invalid file format or size")
@PostMapping(value = "/api/v1/receipts/ocr", consumes = "multipart/form-data")
public ResponseDto<ReceiptOCRResponseDto> OCRReceipt(
- @RequestPart(value = "file") MultipartFile file
+ @RequestPart(value = "file")
+ @Parameter(description = "Receipt image file (JPEG/PNG only, max 5MB)")
+ @ValidImage(maxSize = 5_242_880, types = {"image/jpeg", "image/png"})
+ MultipartFile file
) {
+ if (file == null || file.isEmpty()) {
+ throw new InvalidInputException("Receipt image file is required");
+ }
return ResponseDto.ok(receiptOCRUseCase.execute(file));
}Let's verify if similar validation exists in other controllers:
src/main/java/com/daon/onjung/onjung/application/service/ReceiptOCRService.java (1)
1-19: Verify OCR utility configurations and dependencies
The service structure looks good, following Spring best practices. However, we should verify the configuration of the utility classes.
src/main/java/com/daon/onjung/core/exception/error/ErrorCode.java (1)
29-29: Verify OCR error handling implementation.
Let's ensure this error code is properly implemented and there are no duplicate OCR-related error codes.
✅ Verification successful
Let me check the implementation details of the OCR error handling to ensure it's properly used.
Error code implementation is appropriate and non-redundant
The OCR_RECOGNITION_ERROR code is:
- Properly implemented as the only OCR-related error code in ErrorCode.java
- Correctly used in ReceiptOCRResponseDto to handle OCR processing failures
- Well-integrated with the OCR-related components (ReceiptOCRService, ReceiptOCRUseCase) without any duplicate error handling patterns
The error code serves its intended purpose without redundancy in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for OCR-related error handling implementation and potential duplicates
# Check for OCR error handling usage
echo "Checking OCR error handling implementation..."
rg -l "OCR_RECOGNITION_ERROR"
# Check for other potential OCR-related error codes
echo "Checking for other OCR-related error codes..."
rg -i "ocr.*error|error.*ocr" --type java
Length of output: 818
Script:
#!/bin/bash
# Get the context of OCR error usage in ReceiptOCRResponseDto
echo "Checking OCR error usage context..."
rg -B 5 -A 5 "OCR_RECOGNITION_ERROR" src/main/java/com/daon/onjung/onjung/application/dto/response/ReceiptOCRResponseDto.java
# Check if there are any other OCR-related classes or methods
echo "Checking OCR-related implementations..."
rg -l -i "ocr|receipt.*recognition"
Length of output: 1698
src/main/java/com/daon/onjung/core/utility/RestClientUtil.java (1)
55-73: Verify handling of large OCR image uploads
Since this method will be used for OCR image processing, we need to ensure proper handling of large files.
src/main/java/com/daon/onjung/core/utility/DateTimeUtil.java (1)
8-9: LGTM!
The new imports are necessary and properly used for the new date parsing functionality.
src/main/java/com/daon/onjung/core/dto/ReceiptOCRDto.java (1)
55-58: Validate the estimatedLanguage field in Meta
The estimatedLanguage field (lines 56-57) holds a language code. Consider validating this field against a list of supported language codes to ensure correctness, and handle any unexpected values appropriately.
You can verify the usage of estimatedLanguage in the codebase:
|
|
||
| @UseCase | ||
| public interface ReceiptOCRUseCase { | ||
| ReceiptOCRResponseDto execute(MultipartFile file); |
There was a problem hiding this comment.
💡 Codebase verification
Add input validation for file uploads before OCR processing
The implementation lacks necessary file validation checks. The service directly processes the MultipartFile without any validation:
- No file size limits are enforced
- No file type validation (currently hardcoded as "jpg" in OCRUtil)
- No null or empty file checks
- No malware scanning
Add validation in ReceiptOCRService.execute() before processing:
public ReceiptOCRResponseDto execute(MultipartFile file) {
if (file == null || file.isEmpty()) {
throw new IllegalArgumentException("File cannot be empty");
}
if (!file.getContentType().startsWith("image/")) {
throw new IllegalArgumentException("Only image files are allowed");
}
// ... existing code
}Also consider:
- Adding multipart configuration in application.properties/yml for max file size
- Implementing virus scanning if handling sensitive documents
- Adding proper image format validation beyond just MIME type
🔗 Analysis chain
Add input validation for file uploads.
The execute method accepts a MultipartFile without any apparent validation. Consider:
- File size limits
- Allowed file types (JPEG, PNG)
- Basic image validation
- Malware scanning
Let's check if there's any validation in the implementation:
Let me search for more specific information about the implementation and file handling.
Let me check for any application configuration files that might contain file upload settings and the implementation details of the OCR service.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for file validation logic in the implementation
# Expected: Find MultipartFile validation in the service layer
# Search for the implementation class
ast-grep --pattern 'class $_ implements ReceiptOCRUseCase {
$$$
}'
# Search for validation utilities
rg -l "MultipartFile.*validate|checkFile|isValid"
Length of output: 457
Script:
#!/bin/bash
# Search for the implementation of ReceiptOCRUseCase
rg -l "class.*implements.*ReceiptOCRUseCase"
# Look for MultipartFile handling in the codebase
rg "MultipartFile" -A 5
# Check for any existing file size or type configurations
rg -l "multipart|file\.(size|type|extension)"
Length of output: 10886
Script:
#!/bin/bash
# Check application properties/yml files for multipart configuration
rg -l "multipart" --glob "*.properties" --glob "*.yml"
# Look at the implementation of ReceiptOCRService
rg -A 20 "class ReceiptOCRService implements ReceiptOCRUseCase"
# Check OCRUtil implementation for any validation
rg -A 10 "createOCRRequestBody.*MultipartFile"
Length of output: 3744
| // @no-log | ||
| POST {{host_url}}/api/v1/receipts/ocr | ||
| Content-Type: multipart/form-data; boundary=boundary | ||
| Authorization: Bearer {{access_token}} | ||
|
|
||
| --boundary | ||
| Content-Disposition: form-data; name="file"; filename="receipt.jpg" | ||
| Content-Type: image/jpeg | ||
|
|
||
| < /Users/kyeom/Desktop/receipt3.jpeg |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve OCR endpoint documentation
While the basic structure is good, there are several improvements needed:
- Replace the hardcoded local file path with a placeholder or relative path
- Add example response structure
- Document file size limits and supported formats
Apply this diff:
### 4.4 가게 방문 인증용 영수증 OCR 조회하기
// @no-log
POST {{host_url}}/api/v1/receipts/ocr
Content-Type: multipart/form-data; boundary=boundary
Authorization: Bearer {{access_token}}
--boundary
Content-Disposition: form-data; name="file"; filename="receipt.jpg"
Content-Type: image/jpeg
-< /Users/kyeom/Desktop/receipt3.jpeg
+< ./test-data/sample-receipt.jpg
+
+### Example Response
+{
+ "success": true,
+ "data": {
+ // Add expected OCR response structure here
+ }
+}
+
+### Notes
+// Maximum file size: [specify limit]
+// Supported formats: JPEG, PNG, etc.Committable suggestion skipped: line range outside the PR's diff.
| try { | ||
| return ReceiptOCRResponseDto.fromEntity( | ||
| ocrUtil.mapToReceiptOCRDto( | ||
| restClientUtil.sendMultipartFormDataPostMethod(url, headers, body) | ||
| ) | ||
| ); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("OCR 요청 중 오류가 발생했습니다. {}", e); | ||
| } |
There was a problem hiding this comment.
Improve error handling and message formatting
The current error handling has several issues:
- Using generic RuntimeException instead of specific exceptions
- Incorrect string formatting syntax
- Loss of original error context
Apply these improvements:
- try {
- return ReceiptOCRResponseDto.fromEntity(
- ocrUtil.mapToReceiptOCRDto(
- restClientUtil.sendMultipartFormDataPostMethod(url, headers, body)
- )
- );
- } catch (Exception e) {
- throw new RuntimeException("OCR 요청 중 오류가 발생했습니다. {}", e);
- }
+ try {
+ String response = restClientUtil.sendMultipartFormDataPostMethod(url, headers, body);
+ log.debug("Received OCR response: {}", response);
+
+ var ocrDto = ocrUtil.mapToReceiptOCRDto(response);
+ return ReceiptOCRResponseDto.fromEntity(ocrDto);
+ } catch (RestClientException e) {
+ log.error("Failed to communicate with OCR service: {}", e.getMessage(), e);
+ throw new OCRServiceException("Failed to process OCR request: " + e.getMessage(), e);
+ } catch (Exception e) {
+ log.error("Unexpected error during OCR processing: {}", e.getMessage(), e);
+ throw new OCRProcessingException("OCR 요청 중 오류가 발생했습니다: " + e.getMessage(), e);
+ }Also, create specific exception classes:
public class OCRServiceException extends RuntimeException {
public OCRServiceException(String message, Throwable cause) {
super(message, cause);
}
}
public class OCRProcessingException extends RuntimeException {
public OCRProcessingException(String message, Throwable cause) {
super(message, cause);
}
}| @Override | ||
| public ReceiptOCRResponseDto execute(MultipartFile file) { | ||
| String url = ocrUtil.createOCRRequestUrl(); | ||
| HttpHeaders headers = ocrUtil.createOCRRequestHeaders(); | ||
| MultiValueMap<String, Object> body = ocrUtil.createOCRRequestBody(file); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and logging for better observability
The current implementation could benefit from:
- Input validation for the MultipartFile
- Logging for debugging and monitoring
- Breaking down the chained calls for better error tracking
Consider applying these improvements:
@Override
public ReceiptOCRResponseDto execute(MultipartFile file) {
+ if (file == null || file.isEmpty()) {
+ throw new IllegalArgumentException("Receipt file is required");
+ }
+ log.debug("Processing receipt OCR for file: {}", file.getOriginalFilename());
+
String url = ocrUtil.createOCRRequestUrl();
HttpHeaders headers = ocrUtil.createOCRRequestHeaders();
MultiValueMap<String, Object> body = ocrUtil.createOCRRequestBody(file);
+ log.debug("Prepared OCR request for URL: {}", url);Committable suggestion skipped: line range outside the PR's diff.
| public class ReceiptOCRResponseDto extends SelfValidating<ReceiptOCRDto> { | ||
|
|
||
| @JsonProperty("store_name") | ||
| private final String storeName; | ||
|
|
||
| @JsonProperty("store_address") | ||
| private final String storeAddress; | ||
|
|
||
| @JsonProperty("payment_date") | ||
| private final String paymentDate; | ||
|
|
||
| @JsonProperty("payment_amount") | ||
| private final String paymentAmount; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving type safety and documentation.
- The
paymentAmountfield should be a numeric type (e.g.,BigDecimal) instead ofStringto ensure precise monetary calculations and prevent potential parsing issues. - Add validation annotations (e.g.,
@NotNull,@Pattern) to enforce data integrity since the class extendsSelfValidating. - Include Javadoc documentation for the class and fields to improve maintainability.
+/**
+ * Response DTO for OCR receipt data containing store and payment information.
+ */
public class ReceiptOCRResponseDto extends SelfValidating<ReceiptOCRDto> {
+ /** The name of the store from the receipt. */
+ @NotNull(message = "Store name cannot be null")
@JsonProperty("store_name")
private final String storeName;
+ /** The address of the store from the receipt. */
+ @NotNull(message = "Store address cannot be null")
@JsonProperty("store_address")
private final String storeAddress;
+ /** The payment date in formatted string (e.g., YYYY-MM-DD). */
+ @NotNull(message = "Payment date cannot be null")
+ @Pattern(regexp = "\\d{4}-\\d{2}-\\d{2}", message = "Invalid date format")
@JsonProperty("payment_date")
private final String paymentDate;
+ /** The payment amount from the receipt. */
+ @NotNull(message = "Payment amount cannot be null")
@JsonProperty("payment_amount")
- private final String paymentAmount;
+ private final BigDecimal paymentAmount;📝 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 class ReceiptOCRResponseDto extends SelfValidating<ReceiptOCRDto> { | |
| @JsonProperty("store_name") | |
| private final String storeName; | |
| @JsonProperty("store_address") | |
| private final String storeAddress; | |
| @JsonProperty("payment_date") | |
| private final String paymentDate; | |
| @JsonProperty("payment_amount") | |
| private final String paymentAmount; | |
| /** | |
| * Response DTO for OCR receipt data containing store and payment information. | |
| */ | |
| public class ReceiptOCRResponseDto extends SelfValidating<ReceiptOCRDto> { | |
| /** The name of the store from the receipt. */ | |
| @NotNull(message = "Store name cannot be null") | |
| @JsonProperty("store_name") | |
| private final String storeName; | |
| /** The address of the store from the receipt. */ | |
| @NotNull(message = "Store address cannot be null") | |
| @JsonProperty("store_address") | |
| private final String storeAddress; | |
| /** The payment date in formatted string (e.g., YYYY-MM-DD). */ | |
| @NotNull(message = "Payment date cannot be null") | |
| @Pattern(regexp = "\\d{4}-\\d{2}-\\d{2}", message = "Invalid date format") | |
| @JsonProperty("payment_date") | |
| private final String paymentDate; | |
| /** The payment amount from the receipt. */ | |
| @NotNull(message = "Payment amount cannot be null") | |
| @JsonProperty("payment_amount") | |
| private final BigDecimal paymentAmount; |
| public JSONObject sendMultipartFormDataPostMethod(String url, HttpHeaders headers, MultiValueMap<String, Object> body) { | ||
| try { | ||
| return new JSONObject(Objects.requireNonNull(restClient.post() | ||
| .uri(url) | ||
| .headers(httpHeaders -> httpHeaders.addAll(headers)) | ||
| .contentType(MediaType.MULTIPART_FORM_DATA) | ||
| .body(body) | ||
| .retrieve() | ||
| .onStatus(HttpStatusCode::is4xxClientError, (request, response) -> { | ||
| throw new CommonException(ErrorCode.INVALID_ARGUMENT); | ||
| }) | ||
| .onStatus(HttpStatusCode::is5xxServerError, (request, response) -> { | ||
| throw new CommonException(ErrorCode.INTERNAL_SERVER_ERROR); | ||
| }) | ||
| .toEntity(JSONObject.class).getBody())); | ||
| } catch (Exception e) { | ||
| throw new CommonException(ErrorCode.INTERNAL_SERVER_ERROR); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring common error handling logic
The error handling code is duplicated across all methods in this class.
Extract the common error handling into a separate method:
private <T> T handleResponse(ResponseSpec responseSpec) {
return responseSpec
.onStatus(HttpStatusCode::is4xxClientError,
(request, response) -> {
throw new CommonException(ErrorCode.INVALID_ARGUMENT);
})
.onStatus(HttpStatusCode::is5xxServerError,
(request, response) -> {
throw new CommonException(ErrorCode.INTERNAL_SERVER_ERROR);
})
.toEntity(JSONObject.class)
.getBody();
}Then simplify the methods to:
public JSONObject sendMultipartFormDataPostMethod(...) {
try {
return new JSONObject(Objects.requireNonNull(
handleResponse(restClient.post()
.uri(url)
.headers(httpHeaders -> httpHeaders.addAll(headers))
.contentType(MediaType.MULTIPART_FORM_DATA)
.body(body)
.retrieve())
));
} catch (Exception e) {
log.error("Failed to process multipart request", e);
throw new CommonException(ErrorCode.INTERNAL_SERVER_ERROR, e);
}
}| } catch (Exception e) { | ||
| throw new CommonException(ErrorCode.INTERNAL_SERVER_ERROR); | ||
| } |
There was a problem hiding this comment.
Improve error handling to preserve context
The generic catch block swallows the original exception details and always throws INTERNAL_SERVER_ERROR. This makes debugging harder.
Consider:
- } catch (Exception e) {
- throw new CommonException(ErrorCode.INTERNAL_SERVER_ERROR);
+ } catch (Exception e) {
+ log.error("Failed to process multipart request", e);
+ throw new CommonException(ErrorCode.INTERNAL_SERVER_ERROR, e);Committable suggestion skipped: line range outside the PR's diff.
| /** | ||
| * 날짜 문자열을 특정 포맷으로 변환 | ||
| * | ||
| * @param dateString 날짜 문자열 | ||
| * @return String | ||
| */ | ||
| public static String formatDateString(String dateString) { | ||
| for (DateTimeFormatter formatter : DATE_FORMATTERS) { | ||
| try { | ||
| LocalDate date = LocalDate.parse(dateString, formatter); | ||
| return date.format(ISODateFormatter); | ||
| } catch (DateTimeParseException e) { | ||
| // Ignore and try the next format | ||
| } | ||
| } | ||
| throw new IllegalArgumentException("Unrecognized date format: " + dateString); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and documentation.
The method implementation is solid, but consider these improvements:
- Add input validation
- Enhance documentation with examples
- Consider using a custom exception for better error handling
/**
* 날짜 문자열을 특정 포맷으로 변환
*
* @param dateString 날짜 문자열
+ * @return ISO formatted date string (yyyy-MM-dd)
+ * @throws IllegalArgumentException if the date string is null, empty, or in an unrecognized format
+ * @example
+ * formatDateString("20231101") returns "2023-11-01"
+ * formatDateString("2023년 11월 01일") returns "2023-11-01"
*/
public static String formatDateString(String dateString) {
+ if (dateString == null || dateString.trim().isEmpty()) {
+ throw new IllegalArgumentException("Date string cannot be null or empty");
+ }
+
+ dateString = dateString.trim();
for (DateTimeFormatter formatter : DATE_FORMATTERS) {
try {
LocalDate date = LocalDate.parse(dateString, formatter);
return date.format(ISODateFormatter);
} catch (DateTimeParseException e) {
- // Ignore and try the next format
+ // Continue to the next format
}
}
- throw new IllegalArgumentException("Unrecognized date format: " + dateString);
+ throw new IllegalArgumentException(String.format(
+ "Unrecognized date format: '%s'. Expected formats: yyyy-MM-dd, yyyyMMdd, etc.",
+ dateString
+ ));
}📝 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.
| /** | |
| * 날짜 문자열을 특정 포맷으로 변환 | |
| * | |
| * @param dateString 날짜 문자열 | |
| * @return String | |
| */ | |
| public static String formatDateString(String dateString) { | |
| for (DateTimeFormatter formatter : DATE_FORMATTERS) { | |
| try { | |
| LocalDate date = LocalDate.parse(dateString, formatter); | |
| return date.format(ISODateFormatter); | |
| } catch (DateTimeParseException e) { | |
| // Ignore and try the next format | |
| } | |
| } | |
| throw new IllegalArgumentException("Unrecognized date format: " + dateString); | |
| } | |
| /** | |
| * 날짜 문자열을 특정 포맷으로 변환 | |
| * | |
| * @param dateString 날짜 문자열 | |
| * @return ISO formatted date string (yyyy-MM-dd) | |
| * @throws IllegalArgumentException if the date string is null, empty, or in an unrecognized format | |
| * @example | |
| * formatDateString("20231101") returns "2023-11-01" | |
| * formatDateString("2023년 11월 01일") returns "2023-11-01" | |
| */ | |
| public static String formatDateString(String dateString) { | |
| if (dateString == null || dateString.trim().isEmpty()) { | |
| throw new IllegalArgumentException("Date string cannot be null or empty"); | |
| } | |
| dateString = dateString.trim(); | |
| for (DateTimeFormatter formatter : DATE_FORMATTERS) { | |
| try { | |
| LocalDate date = LocalDate.parse(dateString, formatter); | |
| return date.format(ISODateFormatter); | |
| } catch (DateTimeParseException e) { | |
| // Continue to the next format | |
| } | |
| } | |
| throw new IllegalArgumentException(String.format( | |
| "Unrecognized date format: '%s'. Expected formats: yyyy-MM-dd, yyyyMMdd, etc.", | |
| dateString | |
| )); | |
| } |
💡 Codebase verification
Missing test coverage for DateTimeUtil.formatDateString
The verification reveals:
- No test class exists for DateTimeUtil
- The method is only used in ReceiptOCRResponseDto for parsing dates from OCR results
- No test cases cover the date formatting functionality
Action items:
- Add unit tests for DateTimeUtil.formatDateString to verify:
- Successful parsing of various date formats
- Error handling for invalid date strings
- Correct ISO format output
- Add integration tests for ReceiptOCRResponseDto to verify OCR date parsing
🔗 Analysis chain
Verify test coverage and usage in OCR feature.
Since this method is crucial for the OCR receipt feature, let's verify its integration and test coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test coverage and usage in OCR feature
# Check for test cases
echo "Checking for test cases..."
fd -e java "DateTimeUtilTest" src/test
# Check for usage in OCR related files
echo "Checking usage in OCR feature..."
rg "formatDateString" -A 3 -B 3
# Check for potential date patterns in existing receipt tests
echo "Checking date patterns in receipt tests..."
rg -g "**/*Test.java" -g "**/*Receipt*.java" "(19|20)\d{2}[-/.년]\s*\d{1,2}[-/.월]\s*\d{1,2}"
Length of output: 1977
Script:
#!/bin/bash
# Let's check for test coverage more thoroughly
# Check for any test files related to DateTimeUtil
echo "Checking for DateTimeUtil tests..."
rg -g "**/*Test*.java" "class.*DateTimeUtil.*Test" -A 5
# Check for test methods that might test date formatting
echo "Checking for date format test methods..."
rg -g "**/*Test*.java" "@Test.*format.*Date|test.*format.*Date" -A 10
# Check for any test data with dates in OCR related tests
echo "Checking OCR test data..."
rg -g "**/*Test*.java" -g "**/*OCR*.java" "\"date\".*:.*\".*\"|date.*=.*\".*\"" -A 2
# Check for more usage contexts of formatDateString
echo "Checking all formatDateString usages with wider context..."
rg "formatDateString" -A 5 -B 5 --no-filename
Length of output: 1580
| public record TextBlock( | ||
| @JsonProperty("text") | ||
| String text, | ||
|
|
||
| @JsonProperty("formatted") | ||
| FormattedText formatted, | ||
|
|
||
| @JsonProperty("keyText") | ||
| String keyText, | ||
|
|
||
| @JsonProperty("confidenceScore") | ||
| Double confidenceScore, | ||
|
|
||
| @JsonProperty("boundingPolys") | ||
| List<Polygon> boundingPolys, | ||
|
|
||
| @JsonProperty("maskingPolys") | ||
| List<Polygon> maskingPolys | ||
| ) {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use unmodifiable lists to preserve record immutability
Fields like List<Polygon> boundingPolys (line 229) and List<Polygon> maskingPolys (line 232) in the TextBlock record are mutable collections. To ensure the immutability of records, consider wrapping these lists with Collections.unmodifiableList() or using immutable collections like those provided by Guava or Java's own List.copyOf() method.
Apply this modification within the constructor or factory method:
public TextBlock(
String text,
FormattedText formatted,
String keyText,
Double confidenceScore,
List<Polygon> boundingPolys,
List<Polygon> maskingPolys
) {
this.text = text;
this.formatted = formatted;
this.keyText = keyText;
this.confidenceScore = confidenceScore;
this.boundingPolys = List.copyOf(boundingPolys);
this.maskingPolys = List.copyOf(maskingPolys);
}| @JsonProperty("company") | ||
| TextBlock company, | ||
|
|
||
| @JsonProperty("number") | ||
| TextBlock number | ||
| ) {} |
There was a problem hiding this comment.
Handle potential null values in CardInfo fields
In the CardInfo record (lines 174-179), fields company and number may not always be present in OCR results. Ensure that your application logic can handle null values gracefully to prevent NullPointerException.
Co-authored-by: Eunji <[email protected]>
Related issue 🛠
closed #27
어떤 변경사항이 있었나요?
CheckPoint ✅
PR이 다음 요구 사항을 충족하는지 확인하세요.
Work Description ✏️
Uncompleted Tasks 😅
N/A
To Reviewers 📢
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores