Conversation
Summary of ChangesHello @unikal1, 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은 그룹 관리 기능에 중요한 개선 사항을 도입합니다. 주요 변경 사항은 해시태그 및 기타 조건으로 그룹을 검색할 수 있는 새로운 기능을 추가하고, 그룹 상세 및 목록 조회 시 해시태그 정보를 함께 제공하는 것입니다. 또한, 리포지토리 패키지 구조를 재정비하여 코드의 모듈성을 높였으며, 커서 기반 페이지네이션 시스템을 확장하여 더 다양한 정렬 기준을 지원하도록 개선했습니다. Highlights
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
|
There was a problem hiding this comment.
Code Review
이 Pull Request는 그룹 검색 기능과 커서 기반 페이지네이션을 도입하는 변경 사항을 담고 있습니다. 전반적으로 페이지네이션 로직과 Argument Resolver 개선 등 좋은 변경 사항이 많습니다. 하지만 Querydsl 구현에서 성능 저하를 유발할 수 있는 심각한 문제가 발견되었으며, 코드 명확성과 유지보수성을 높일 수 있는 몇 가지 개선점이 보입니다. 오해의 소지가 있는 메서드 이름 변경, Javadoc 템플릿 내용 채우기, 일부 로직을 더 간결하게 리팩토링하는 것에 대한 구체적인 의견을 남겼습니다.
...ain/java/com/studypals/domain/groupManage/dao/groupRepository/GroupCustomRepositoryImpl.java
Show resolved
Hide resolved
src/main/java/com/studypals/domain/groupManage/api/GroupController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/studypals/domain/groupManage/api/GroupController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/studypals/domain/groupManage/dao/groupRepository/GroupCustomRepository.java
Show resolved
Hide resolved
...ain/java/com/studypals/domain/groupManage/dao/groupRepository/GroupCustomRepositoryImpl.java
Show resolved
Hide resolved
src/main/java/com/studypals/domain/groupManage/dao/groupRepository/GroupSortType.java
Show resolved
Hide resolved
src/main/java/com/studypals/domain/groupManage/service/GroupServiceImpl.java
Show resolved
Hide resolved
src/main/java/com/studypals/domain/groupManage/worker/GroupHashTagWorker.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e9be1aca8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
...ain/java/com/studypals/domain/groupManage/dao/groupRepository/GroupCustomRepositoryImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/studypals/domain/groupManage/dao/GroupHashTagRepository.java
Show resolved
Hide resolved
...ain/java/com/studypals/domain/groupManage/dao/groupRepository/GroupCustomRepositoryImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/studypals/domain/groupManage/dao/groupRepository/GroupSortType.java
Outdated
Show resolved
Hide resolved
src/main/java/com/studypals/domain/groupManage/dao/groupRepository/GroupCustomRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/com/studypals/domain/groupManage/worker/GroupHashTagWorker.java
Show resolved
Hide resolved
src/main/java/com/studypals/domain/groupManage/service/GroupServiceImpl.java
Outdated
Show resolved
Hide resolved
...ain/java/com/studypals/domain/groupManage/dao/groupRepository/GroupCustomRepositoryImpl.java
Outdated
Show resolved
Hide resolved
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
안녕하세요. 그룹 해시태그 조회 및 검색 기능 추가 PR에 대한 리뷰입니다. 전반적으로 커서 기반 페이징, QueryDSL을 활용한 동적 검색, exists 서브쿼리를 이용한 성능 최적화 등 새로운 기능 구현이 매우 잘 이루어졌습니다. 특히 AbstractPagingRepository와 SortTypeResolver 리팩토링을 통해 코드의 타입 안정성과 재사용성을 높인 점이 인상적입니다. 몇 가지 API 안정성 및 필터 로직 정확성 개선을 위한 제안 사항을 리뷰 댓글로 남겼으니 확인 부탁드립니다. 좋은 코드를 작성해주셔서 감사합니다!
src/main/java/com/studypals/domain/groupManage/api/GroupController.java
Outdated
Show resolved
Hide resolved
...ain/java/com/studypals/domain/groupManage/dao/groupRepository/GroupCustomRepositoryImpl.java
Outdated
Show resolved
Hide resolved
...ain/java/com/studypals/domain/groupManage/dao/groupRepository/GroupCustomRepositoryImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39ff738bab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/main/java/com/studypals/domain/groupManage/api/GroupController.java
Outdated
Show resolved
Hide resolved
sleepyhoon
left a comment
There was a problem hiding this comment.
1차 리뷰 입니다. AbstractPagingRepository의 경우 이해도가 부족해 리뷰하지 못했습니다. 기존 방식과 어떤 차이점이 있는지 조금더 설명해주세요.
...n/java/com/studypals/domain/groupManage/dao/groupMemberRepository/GroupMemberRepository.java
Show resolved
Hide resolved
| @GetMapping("/search") | ||
| public ResponseEntity<CursorResponse<GetGroupsRes>> searchGroups( | ||
| @CursorDefault(sortType = GroupSortType.class, cursor = 0, size = 5, sort = "POPULAR") Cursor cursor, | ||
| @RequestParam(required = false, name = "hashTag") String hashTag, | ||
| @RequestParam(required = false, name = "tag") String tag, | ||
| @RequestParam(required = false, name = "name") String name, | ||
| @RequestParam(required = false, name = "open") Boolean open, | ||
| @RequestParam(required = false, name = "approval") Boolean approval) { |
There was a problem hiding this comment.
[P3]
필드수가 많은데 hashTag, tag, name, open, approval 5개 필드를 포함한 dto로 필드를 받는 것은 어떤가요?
...ypals/domain/groupManage/dao/groupEntryRepository/GroupEntryRequestCustomRepositoryImpl.java
Show resolved
Hide resolved
.../studypals/domain/groupManage/dao/groupMemberRepository/GroupMemberCustomRepositoryImpl.java
Show resolved
Hide resolved
| try { | ||
| dto.validate(); | ||
| } catch (IllegalArgumentException e) { | ||
| throw new GroupException( | ||
| GroupErrorCode.GROUP_SEARCH_FAIL, | ||
| e.getMessage(), | ||
| "[GroupServiceImpl#search] validate dto fail : " + e.getMessage()); | ||
| } |
There was a problem hiding this comment.
실제 내부 validate 구현을 보시면 Validator 기반의 어노테이션으로 검증이 불가능함을 알 수 있습니다. 해당 검증 로직은 hashtag/tag/title 중 2개 이상의 값이 들어오는 경우에 대한 필터링입니다.
There was a problem hiding this comment.
제 말을 잘못했네요. 주된 의미는 dto 안에서 유효성 검증을 하는건 어떨까였습니다. 예를 들어 생성자 내에서 validate 로직을 수행하면 서비스 코드에서 검증할 필요가 없으니 더 좋다고 생각했습니다.
There was a problem hiding this comment.
그러면 앞서 이야기하셨던, 입력을 dto 로 받자는 거랑 연관하여 같이 해보겠습니다.
| @SuppressWarnings({"rawtypes", "unchecked"}) | ||
| protected <E> OrderSpecifier<?> getOrderSpecifier(EntityPath<E> root, SortType sortType) { | ||
| String field = sortType.getField(); | ||
| Sort.Direction direction = sortType.getDirection(); | ||
|
|
||
| EntityMetadata metadata = getEntityMetadata(entityClass); | ||
| EntityMetadata metadata = getEntityMetadata(root.getType()); | ||
| Class<?> sortFieldType = metadata.getFieldType(field); | ||
| CACHE.put(entityClass, metadata); | ||
|
|
||
| PathBuilder<T> path = new PathBuilder<>(entityClass, metadata.getEntityName()); | ||
| Order order = direction == Sort.Direction.ASC ? Order.ASC : Order.DESC; | ||
| // alias를 문자열로 만들지 말고 root의 metadata를 그대로 사용 | ||
| PathMetadata rootMetadata = root.getMetadata(); | ||
| PathBuilder<E> path = new PathBuilder<>((Class<E>) root.getType(), rootMetadata); | ||
|
|
||
| Order order = direction == Sort.Direction.ASC ? Order.ASC : Order.DESC; | ||
| return new OrderSpecifier<>(order, (Expression<? extends Comparable>) path.get(field, sortFieldType)); | ||
| } | ||
|
|
||
| private EntityMetadata getEntityMetadata(Class<T> entityClass) { | ||
| if (CACHE.containsKey(entityClass)) return CACHE.get(entityClass); | ||
| @SuppressWarnings({"rawtypes", "unchecked"}) | ||
| protected <E> OrderSpecifier<?>[] getOrderSpecifierWithId(EntityPath<E> root, SortType type) { | ||
| OrderSpecifier<?> primary = getOrderSpecifier(root, type); | ||
|
|
||
| Sort.Direction direction = type.getDirection(); | ||
| Order order = direction == Sort.Direction.ASC ? Order.ASC : Order.DESC; | ||
|
|
||
| EntityMetadata metadata = getEntityMetadata(root.getType()); | ||
| PathBuilder<E> path = new PathBuilder<>((Class<E>) root.getType(), root.getMetadata()); | ||
|
|
||
| // querydsl 에서 필요한 건 Class 명 자체가 아니라 QEntity 의 static final 변수명 | ||
| // ex) QGroup.group => "group" | ||
| String className = entityClass.getSimpleName(); | ||
| String entityName = Character.toLowerCase(className.charAt(0)) + className.substring(1); | ||
| return CACHE.put(entityClass, new EntityMetadata(entityClass, entityName)); | ||
| OrderSpecifier<?> secondary = new OrderSpecifier<>(order, (Expression<? extends Comparable>) | ||
| path.get(metadata.getIdFieldName(), metadata.getIdFieldType())); | ||
|
|
||
| return new OrderSpecifier<?>[] {primary, secondary}; | ||
| } |
There was a problem hiding this comment.
[P1]
추가 설명을 부탁드립니다. 리뷰가 불가능합니다.
There was a problem hiding this comment.
사실 이건 저도 이해하는데 오래 걸리긴 했습니다(원본이 제가 작성한게 아니라서요) 시간 되실 때 디스코드로 직접 이야기 하는 편이 빠를 것 같긴 합니다. 그럼에도 기본적인 것만 말해보겠습니다.
일단 AbstractPageRepository 는 오로지 OrderSpecifier 를 만드는데 초점을 두고 있습니다. 즉, 어떤 방법으로 정렬할지에 대한 기준을 정의하는 추상 클래스입니다. 그렇다면 단순히 정렬일 뿐인데 왜 이렇게 복잡한 로직이 필요하냐? 라는 의문이 들 수 있습니다. 그 이유는 "문자열"기반의 정렬 기준을 실제 엔티티의 필드로 치환하기 위함입니다.
이렇게 말하면 체감이 잘 안될텐데 한가지 예를 들어보겠습니다.
GroupSortType 에는 POPULAR 이 있습니다. 여기에 정의된 String field 값은 totalMember 입니다. 그리고 이 이름은 실제 group 엔티티의 필드이죠.
다만 문제는, 이런 "문자열"을 실제 엔티티의 "필드의 타입으로 바꾸는 과정은 그리 호락호락 하지 않다는 것입니다. 리플렉션을 사용하는 것은 당연하고요, 매 요청마다 리플렉션이 일어나면 좀 번거로우니 캐싱을 사용하여 엔티티의 메타데이터를 메모리에 저장하는 과정까지도 추가할 수 있겠죠.
기본적으로 queryDSL 은 엔티티에 대해 Q 타입의 객체를 생성합니다. 그리고 해당 클래스에서 입력되는 class 는 이러한 Q 타입이죠. 이러한 Q 타입을 받는 entityPath 는 getType 메서드를 통해 원본 엔티티를 뽑을 수 있습니다.
내부 클래스인 EntityMetaData 는 이러한 엔티티 객체에 대해 다음 정보를 추출 및 기록합니다.
- 해당 엔티티의 이름
- 해당 엔티티의 아이디 이름 및 타입
- 해당 엔티티의 필드 이름 및 타입(여러 개)
이후 해당하는 EntityMetaData 를 CACHE 에 저장하죠. 이는 getEntityMetaData()메서드에서 관리합니다.
입력된 타입에 대해 -> 캐시에서 검색 -> 있으면 있는걸 반환 -> 없으면 새로 생성해서 반환하고 캐시에 저장
그리고 getOrderSpecifier 는 EntityPath 및 SortType 을 파라미터로 받습니다.
EntityPath 는 쉽게 말해 Q 타입 엔티티의 조상 클래스이며 SortType 은 Cursor 에서 지정한 구현체가 오겠죠.
이후 해당 EntityPath(root) 의 메타데이터를 가져옵니다. 가져온 메타데이터에 대해, SortType 에 명시된 String field 를 사용해 EntityMeta 의 fieldClasses 를 조회하는 getFieldType 메서드를 호출하여, 실제 엔티티의 필드를 가져오게 됩니다.
여기까지가 앞서 말한 "문자열"로 주어진 필드의 타입을 구하는 과정입니다.
OrderSpecifier 를 구성할 때는, 정렬 방향(Order), PathBuilder 를 받고 PathBuilder 는 정렬 기준 대상 필드의 "필드 이름 - 문자열" 및 "필드 타입"이 필요합니다. 이러한 필드 타입을 얻기 위해 앞서 과정을 수행한 것입니다.
getOrdreSpecifierWithId 의 경우 orderSpecifier 를 2개 생성합니다. 왜 그런지에 대해, 아래 Cursor.java 에 대해 설명할 때 같이 말해보겠습니다.
There was a problem hiding this comment.
확인했습니다. 관련 주석을 간단하게 작성해두면 좋겠습니다.
| * @since 2025-06-05 | ||
| */ | ||
| public record Cursor(long cursor, int size, SortType sort) { | ||
| public record Cursor(Long cursor, String value, int size, SortType sort) { |
There was a problem hiding this comment.
[P2]
String value의 추가 이유가 무엇인가요?
There was a problem hiding this comment.
정렬 기준이 POPULAR, 그러니까 group 엔티티의 totalMember 라 해보겠습니다.
클라이언트는 가장 인기있는 순으로 데이터를 요청했고, 그럼에 따라 총 멤버 수가 높은 순으로 정렬이 되어 왔습니다. 일단 한 페이지에 5개만 왔다고 해보죠.
(totalMember == ttm)
groupA / ttm = 30
groupB / ttm = 20
groupC / ttm = 20
groupD / ttm = 15
groupE / ttm = 15
근데 사실 ttm = 15 인 친구가 2개가 아닌 5개 였습니다.
groupF / ttm = 15
groupG / ttm = 15
groupH / ttm = 15
이런 식으로요.
ttm 으로 조회를 할 때, 항상 저렇게 순서대로 오면 상관이 없겠지만 어느날은 첫 페이지에 groupD / groupE 가 오고, 어느 날은 groupG, groupD 가 올 수도 있습니다. 조건만 맞으면 뒤죽박죽이라는거죠.
이거는 다음 페이지에서도 마찬가지인데, 기존에 cursor 만 존재했을 때는 groupE 의 id(5) 를 건내줬을 겁니다. 다음 페이지를 해당 값으로 요청 시 db 입장에서는 id = 5 보다 큰 친구들에 대해 ttm 에 대해 정렬하여 반환하겠죠.
근데 첫번째 페이지에 사실 groupD(id = 4) 가 아닌 groupF(id = 6) 이 들어갔다는 겁니다.
혹은, groupG, groupH 가 들어가느라 groupH 에 대한 id 로 다음 페이지 요청을 보내어 C, D, E, F 가 전부 씹힐 수도 있고요.
따라서 이러한 문제를 보안하기 위해 ttm 과 id 를 같이 건내주도록 하여 애초에 정렬을 두가지 기준으로 하고, 데이터 역시도 해당 값을 기준으로 가져오는 것이죠.
결론적으로 정렬 기준 필드와 value 가 같은 경우, id 에 대해 2차적으로 정렬되어 있을 테니 해당 id 보다 큰 값만 가져오기 위함입니다.
WHERE (g.totalMember < :total) OR (g.totalMember = :total AND g.id < :cursorId)이런 식으로요
There was a problem hiding this comment.
들어주신 예시를 이용하자면 cursor = totalMember 이고, value = id 인가요?
There was a problem hiding this comment.
아니요 그 반대입니다. cursor 가 id 이고, value 가 totalMember 입니다.
close [FEATURE] 그룹 해시태그 조회/검색 #164
✨ 구현 기능 명세
1. 그룹 해시태그 저장/정규화
그룹 생성 시 입력된 해시태그는
StringUtils.normalize로 정규화하여 DB에는 검색 친화적인 형태로 저장하고, 사용자에게 보여줄 문자열은displayTag로 분리해 보관했습니다._,#제거, 특수문자 제거, 소문자 변환, 연속_축약, 앞뒤_제거usedCount를 bulk 증가시키고, 신규 태그는 생성한 뒤GroupHashTag매핑을 저장합니다.예시
"#Spring Boot","Spring__Boot"," spring-boot ""spring_boot"(모두 동일)사용자 노출은 항상
displayTag(입력 원본)에 맞춰 반환됩니다.2. 그룹 목록/상세 조회 응답에 hashTags 포함
GroupHashTagWorker를 통해 groupId별 해시태그 목록을 조회하고, 응답 DTO에hashTags를 추가했습니다.GetGroupsRes.hashTagsGetGroupDetailRes.hashTags예시
{ "groupId": 12, "name": "알고리즘 스터디", "hashTags": ["#DP", "#그래프", "#Spring Boot"] }3. 그룹 검색 API (해시태그/태그/이름)
GET /groups/search에서 tag/name/hashTag 중 하나만 허용되도록 제약을 추가했고,open/approval필터 및 커서 기반 페이징을 지원했습니다.open=true,approval=true요청 예시
4. 검색 구현(QueryDSL) - hashTag는 exists 서브쿼리
N:M 조인 중복 row를 피하기 위해 hashTag 검색은
exists서브쿼리로 필터링했습니다.코드 예시
5. 커서 페이지네이션 + deterministic ordering
size + 1조회로hasNext를 판단하고, 정렬 필드 +idtie-breaker로 결과 누락/중복을 방지했습니다.totalMember desc, id desccreatedDate desc, id desccreatedDate asc, id ascCursor.value 의미
totalMembercreatedDate예시 (POPULAR)
✅ PR Point
exists서브쿼리로 처리해 N:M 조인 중복을 제거하고, "해시태그 존재 여부"만 빠르게 확인하도록 구성했습니다.idtie-breaker를 추가해 동일 정렬값에서 페이지 이동 시 결과 누락/중복을 방지했습니다.어려웠던 점
tag/name/hashTag중 하나만 허용하는 제약을 유지하면서도, 필터(open/approval) + 커서 + 정렬까지 한 쿼리에 조합하는 로직 구성이 까다로웠습니다.