[DEV-265/BE] feat: 회사를 검색하는 API 개발#393
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Summary of ChangesHello @lja3723, 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은 회사 검색 기능을 구현하기 위해 백엔드에 새로운 API 엔드포인트를 추가하고 관련 로직을 개선합니다. 특히, 한글 검색의 정확도를 높이기 위해 한글을 초성, 중성, 종성 단위로 분리하는 유틸리티를 도입하고, 이 분리된 문자열을 저장하는 Highlights
Changelog
Activity
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}
| public class CompanyFixture { | ||
|
|
||
| public static final Company TEST_COMPANY = Company.create("test company", "test.com/logo.png", true); | ||
| public static final Company TEST_COMPANY = Company.create("test company", new HangulUtil().decompose("test company"), "test.com/logo.png"); |
There was a problem hiding this comment.
The change in the Company.create factory method's signature has implicitly set isSearchAllowed to false for TEST_COMPANY. This introduces a hardcoded business logic value within the factory method, which can lead to unexpected test failures if other tests rely on TEST_COMPANY being searchable (isSearchAllowed=true).
According to the rule 'Avoid hardcoding business logic values in entity factory methods. Pass them as parameters to improve clarity and flexibility, leaving business rule enforcement to service layers.', it is recommended to modify the Company.create factory method to explicitly accept isSearchAllowed as a parameter. This approach improves clarity and flexibility, allowing the desired state to be set directly during object creation, rather than relying on post-creation calls like allowSearch() or implicit defaults.
References
- Avoid hardcoding business logic values in entity factory methods. Pass them as parameters to improve clarity and flexibility, leaving business rule enforcement to service layers.
There was a problem hiding this comment.
Pull request overview
This PR implements a company search API that enables users to search for companies using Korean text with automatic jamo (character component) decomposition for flexible search matching. The implementation adds a HangulUtil utility class to decompose Korean characters into their constituent jamo components (초성, 중성, 종성), stores this decomposed form in a new searchName field, and provides a paginated search endpoint.
Changes:
- Added
HangulUtilcomponent for Korean character decomposition with comprehensive test coverage - Extended
Companyentity withsearchNamefield to store decomposed Korean text for search optimization - Implemented company search API endpoint with pagination support and query filtering based on decomposed text
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/main/java/com/shyashyashya/refit/global/util/HangulUtil.java | New utility class for decomposing Korean characters into jamo components (초성, 중성, 종성) |
| backend/src/test/java/com/shyashyashya/refit/global/util/HangulUtilTest.java | Comprehensive test suite for HangulUtil covering edge cases and various Korean character combinations |
| backend/src/main/java/com/shyashyashya/refit/domain/company/model/Company.java | Added searchName field and allowSearch() method; updated factory method signature |
| backend/src/main/java/com/shyashyashya/refit/domain/company/repository/CompanyRepository.java | Added findAllBySearchQuery method with JPQL query for prefix-based search on decomposed text |
| backend/src/main/java/com/shyashyashya/refit/domain/company/service/CompanyService.java | New service implementing company search logic with null handling and query decomposition |
| backend/src/main/java/com/shyashyashya/refit/domain/company/api/CompanyController.java | New REST controller exposing GET /company endpoint for paginated company search |
| backend/src/main/java/com/shyashyashya/refit/domain/company/api/response/CompanyResponse.java | New response DTO for company search results |
| backend/src/main/java/com/shyashyashya/refit/domain/interview/service/InterviewService.java | Updated company creation to include searchName decomposition |
| backend/src/test/java/com/shyashyashya/refit/fixture/CompanyFixture.java | Updated fixture to include searchName field |
| backend/src/test/java/com/shyashyashya/refit/core/IntegrationTest.java | Added helper methods for creating companies with searchName and isSearchAllowed flag |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @RequestParam(required = false) String q, @ParameterObject Pageable pageable) { | ||
| var response = companyService.findCompanies(q, pageable); |
There was a problem hiding this comment.
The query parameter name 'q' is not descriptive and could be confusing for API consumers. Consider using a more meaningful parameter name like 'query' or 'searchQuery' to improve API clarity. While this is a common shorthand in some APIs, the codebase doesn't show a consistent pattern for abbreviated parameter names in similar search endpoints.
| @RequestParam(required = false) String q, @ParameterObject Pageable pageable) { | |
| var response = companyService.findCompanies(q, pageable); | |
| @RequestParam(name = "query", required = false) String query, @ParameterObject Pageable pageable) { | |
| var response = companyService.findCompanies(query, pageable); |
| @Column(name = "search_name", columnDefinition = "varchar(80)") | ||
| private String searchName; |
There was a problem hiding this comment.
The searchName field has a maximum length of 80 characters (varchar(80)), but the name field is only 20 characters (varchar(20)). Since Korean characters decompose into 2-3 jamo characters each, a 20-character Korean company name could expand to approximately 40-60 characters when decomposed. However, the 80-character limit should be sufficient. Consider adding validation to ensure that the decomposed searchName doesn't exceed this limit, or documenting why 80 characters is sufficient for the expected use case.
| @RestController | ||
| @RequiredArgsConstructor | ||
| @RequestMapping("/company") | ||
| public class CompanyController { | ||
|
|
||
| private final CompanyService companyService; | ||
|
|
||
| @GetMapping | ||
| public ResponseEntity<ApiResponse<Page<CompanyResponse>>> findCompanies( | ||
| @RequestParam(required = false) String q, @ParameterObject Pageable pageable) { | ||
| var response = companyService.findCompanies(q, pageable); | ||
| var body = ApiResponse.success(COMMON200, response); | ||
| return ResponseEntity.ok(body); | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a significant discrepancy between the PR title/description and the mentioned issue #265. The PR is about implementing a company search API (DEV-265/BE), but the mentioned issue #265 is about "스크랩 폴더 목록 조회하는 API 구현" (DEV-191/BE - retrieving scrap folder lists with QnA set inclusion status). This appears to be a mismatch in the issue reference. Please verify that the correct issue is linked to this PR.
| public Page<CompanyResponse> findCompanies(String query, Pageable pageable) { | ||
| query = (query == null || query.isBlank()) ? "" : hangulUtil.decompose(query); | ||
| return companyRepository.findAllBySearchQuery(query, pageable); | ||
| } |
There was a problem hiding this comment.
The new company search API endpoint lacks integration test coverage. The codebase has extensive integration tests for similar GET endpoints (e.g., InterviewIntegrationTest, QnaSetIntegrationTest). Consider adding integration tests to verify:
- Successful company search with a valid query
- Search with empty/null query returns all allowed companies
- Search correctly filters by searchName prefix
- Search respects isSearchAllowed flag (only returns companies where isSearchAllowed=true)
- Pagination works correctly
- Hangul decomposition is properly applied to the search query
| public class CompanyFixture { | ||
|
|
||
| public static final Company TEST_COMPANY = Company.create("test company", "test.com/logo.png", true); | ||
| public static final Company TEST_COMPANY = Company.create("test company", new HangulUtil().decompose("test company"), "test.com/logo.png"); |
There was a problem hiding this comment.
The CompanyFixture creates a new HangulUtil instance directly instead of using dependency injection. This is inconsistent with how HangulUtil is used elsewhere in the codebase (as a Spring-managed @component). While this works for test fixtures, it's better practice to either:
- Make HangulUtil methods static if they don't require any dependencies (since it's a pure utility), or
- Pass HangulUtil as a parameter to a factory method in the fixture
Consider refactoring HangulUtil to have static methods since it has no dependencies and is purely computational, which would make it easier to use in static fixture contexts.
| public static final Company TEST_COMPANY = Company.create("test company", new HangulUtil().decompose("test company"), "test.com/logo.png"); | |
| public static Company createTestCompany(HangulUtil hangulUtil) { | |
| return Company.create("test company", hangulUtil.decompose("test company"), "test.com/logo.png"); | |
| } |
| @Column(name = "search_name", columnDefinition = "varchar(80)") | ||
| private String searchName; |
There was a problem hiding this comment.
The searchName field is used in a LIKE query with a prefix pattern (CONCAT(:query, '%')) but lacks a database index. For optimal search performance, especially as the number of companies grows, consider adding an index on the searchName column. This would significantly improve query performance for prefix searches. You can add this using JPA's @table annotation with @Index, or through a database migration script.
| @@ -0,0 +1,3 @@ | |||
| package com.shyashyashya.refit.domain.company.api.response; | |||
|
|
|||
| public record CompanyResponse(Long companyId, String companyName) {} | |||
There was a problem hiding this comment.
The CompanyResponse record doesn't include the logoUrl field, while the existing CompanyDto does include it (companyLogoUrl). Consider whether the company logo URL should be included in the search results, as it would likely be useful for displaying company logos in the UI when showing search results. If this omission is intentional, it's acceptable, but it's worth confirming the expected use case.
| public record CompanyResponse(Long companyId, String companyName) {} | |
| public record CompanyResponse(Long companyId, String companyName, String companyLogoUrl) {} |
| @RestController | ||
| @RequiredArgsConstructor | ||
| @RequestMapping("/company") | ||
| public class CompanyController { | ||
|
|
||
| private final CompanyService companyService; | ||
|
|
||
| @GetMapping |
There was a problem hiding this comment.
The CompanyController is missing API documentation annotations. According to the codebase conventions, all controllers should have @tag annotation at the class level and @operation annotation at the method level to provide proper Swagger/OpenAPI documentation. Please add:
- @tag annotation with name and description at the class level
- @operation annotation with a summary (and optionally description) at the method level
Example based on similar controllers in the codebase:
@Tag(name = "Company API", description = "회사 관련 API 입니다.")
and
@Operation(summary = "회사 목록을 검색합니다.")
| public ResponseEntity<ApiResponse<Page<CompanyResponse>>> findCompanies( | ||
| @RequestParam(required = false) String q, @ParameterObject Pageable pageable) { |
There was a problem hiding this comment.
변수명 q 보다 구체적으로 적어주시면 가독성이 더 좋을 것 같습니다!
There was a problem hiding this comment.
api 사용시 /company?q=캌 처럼 검색되는 걸 원하긴 했는데, 그러면 그냥 어노테이션에만 q로 명시하고 변수명은 구체적으로 써도 괜찮나요?
| @Query(value = """ | ||
| SELECT new com.shyashyashya.refit.domain.company.api.response.CompanyResponse(c.id, c.name) | ||
| FROM Company c | ||
| WHERE LOWER(c.searchName) LIKE LOWER(CONCAT(:query, '%')) | ||
| AND c.isSearchAllowed = TRUE | ||
| """, countQuery = """ | ||
| SELECT COUNT(c) | ||
| FROM Company c | ||
| WHERE LOWER(c.searchName) LIKE LOWER(CONCAT(:query, '%')) | ||
| AND c.isSearchAllowed = TRUE | ||
| """) | ||
| Page<CompanyResponse> findAllBySearchQuery(String query, Pageable pageable); |
There was a problem hiding this comment.
쿼리 dsl 로 프로젝션하면 좋을 것 같습니다!
There was a problem hiding this comment.
++ 만약에 후속에서 하시겠다고 하면 투두 주석 남겨두면 좋을 것 같아요
There was a problem hiding this comment.
자세한 테스트코드 좋은데요~
혹시 한글 + 공백 + 한글 과 같은 조합, 공백이 포함된 한글 + 숫자 조합, 한글 + 영어 + 숫자 조합도 테스트 하면 어떨까요? 회사 이름에 공백도 충분히 들어갈 수 있을 것 같아서요!
There was a problem hiding this comment.
좋은 제안 감사합니다. 반영하겠습니다
| public static Company create(String name, String logoUrl, boolean isSearchAllowed) { | ||
| public static Company create(String name, String searchName, String logoUrl) { |
There was a problem hiding this comment.
검색용 이름이라고 하니까 비즈니스 로직을 도메인이 알고 있는 느낌이 드는데, 혹시 decomposedName 과 같은 변수명은 어떻게 생각하시나요?
| query = (query == null || query.isBlank()) ? "" : hangulUtil.decompose(query); | ||
| return companyRepository.findAllBySearchQuery(query, pageable); |
There was a problem hiding this comment.
null, blank 처리를 decompose 내부에서 빈문자 반환 처리하는 건 어떤 것 같으세요?
2 participants
관련 이슈
close #390
작업한 내용
hangulUtil.decompose('두찜닭스쉼') -> "ㄷㅜㅉㅣㅁㄷㅏㄺㅅㅡㅅㅟㅁ"searchName)PR 리뷰시 참고할 사항
추후 ㅟ, ㄺ 같은 것도 분리 고려