Conversation
Summary of ChangesHello, 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! 이 Pull Request는 매니저가 선수 정보를 수동으로 한 명씩 입력하거나 다양한 형식의 텍스트를 파싱하는 데 겪었던 불편함을 해소하기 위해 자연어 기반의 선수 벌크 등록 기능을 도입합니다. Gemini Function Calling을 활용하여 유연하게 선수 정보를 추출하고, 2단계 프로세스를 통해 등록 전 검증 및 미리보기를 제공함으로써 사용자 경험을 개선하고 데이터 정확성을 높이는 데 중점을 두었습니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 implements an NL-based bulk player registration API, featuring well-structured Gemini API integration and clear business logic. However, critical vulnerabilities were identified, including prompt injection via the user-controlled history parameter and potential cross-site scripting (XSS) from unsanitized LLM output. Beyond security, there are opportunities to enhance performance, stability, and maintainability, particularly concerning blocking behavior during Gemini API calls, the safety of DTOs, and the efficiency of certain logic. Addressing these security concerns by restricting roles in conversation history and implementing proper sanitization for all LLM-extracted data is crucial, alongside reviewing the identified performance and maintainability aspects.
src/main/java/com/sports/server/command/nl/infra/GeminiFunctionCallResponse.java
Show resolved
Hide resolved
| .bodyValue(body) | ||
| .retrieve() | ||
| .bodyToMono(GeminiFunctionCallResponse.class) | ||
| .block(Duration.ofSeconds(30)); |
There was a problem hiding this comment.
block(Duration.ofSeconds(30)) 호출은 HTTP 요청을 처리하는 스레드를 최대 30초 동안 차단할 수 있습니다. Gemini API 응답이 지연될 경우, 웹 서버의 스레드 풀이 고갈되어 전체 서비스의 응답성에 심각한 영향을 줄 수 있습니다.
이 기능을 사용하는 동시 사용자 수가 많아질 경우를 대비하여, @Async와 CompletableFuture를 사용한 비동기 처리 방식을 도입하여 요청 처리 스레드가 즉시 반환되도록 하는 것을 고려해보세요. 이는 시스템의 확장성과 안정성을 크게 향상시킬 것입니다.
There was a problem hiding this comment.
현재 사용자 수로는 이상 없을 것 같습니당
| for (Map<String, String> entry : history) { | ||
| String role = entry.get("role"); | ||
| String content = entry.get("content"); | ||
| if (role != null && content != null) { | ||
| String geminiRole = "assistant".equals(role) ? "model" : "user"; | ||
| contents.add(Map.of( | ||
| "role", geminiRole, | ||
| "parts", List.of(Map.of("text", content)) | ||
| )); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The history parameter in the NlProcessRequest is directly used to construct the conversation history sent to the Gemini LLM. The application allows the user to specify the role for each history entry, including the assistant role (which is mapped to the LLM's model role). This enables a malicious user to spoof previous model responses, potentially bypassing system instructions or manipulating the LLM into returning incorrect data (e.g., associating a different name with a valid student number). While the application validates student numbers against the original message, other fields like the player's name remain vulnerable to manipulation.
| failedLines.add(new FailedLine( | ||
| i + 1, | ||
| name != null ? name + " " + studentNumber : "라인 " + (i + 1), | ||
| StudentNumber.isInvalid(studentNumber) | ||
| ? NlErrorMessages.STUDENT_NUMBER_INVALID | ||
| : NlErrorMessages.STUDENT_NUMBER_NOT_IN_ORIGINAL | ||
| )); |
There was a problem hiding this comment.
The process method returns a list of failedLines that includes the name and studentNumber extracted by the LLM from the user-provided message. These values are reflected back to the user in the response without sanitization. If an attacker crafts a message that causes the LLM to return a malicious script in the name field, and this script is rendered by the frontend application, it could lead to reflected XSS.
| playerId = playerService.register( | ||
| new PlayerRequest.Register(playerData.name(), playerData.studentNumber()) | ||
| ); |
There was a problem hiding this comment.
The execute method registers new players using names and student numbers provided in the request. These values are typically derived from the LLM output in the previous process step. The application does not sanitize the player's name before passing it to playerService.register, which stores it in the database. If an attacker (or a malicious manager) provides a name containing a script, it will be stored and potentially executed when other users view the player's information in the application.
src/main/java/com/sports/server/command/nl/application/NlService.java
Outdated
Show resolved
Hide resolved
| int newCount = (int) playerPreviews.stream().filter(p -> "NEW".equals(p.status())).count(); | ||
| int existsCount = (int) playerPreviews.stream().filter(p -> "EXISTS".equals(p.status())).count(); | ||
| int alreadyInTeamCount = (int) playerPreviews.stream().filter(p -> "ALREADY_IN_TEAM".equals(p.status())).count(); | ||
|
|
||
| Summary summary = new Summary(playerPreviews.size(), newCount, existsCount, alreadyInTeamCount); |
There was a problem hiding this comment.
현재 Summary를 생성하기 위해 playerPreviews 리스트를 여러 번 순회하고 있습니다. 이는 리스트가 길어질 경우 성능 저하의 원인이 될 수 있습니다. Collectors.groupingBy를 사용하여 한 번의 순회로 모든 카운트를 계산하는 것이 더 효율적입니다.
| int newCount = (int) playerPreviews.stream().filter(p -> "NEW".equals(p.status())).count(); | |
| int existsCount = (int) playerPreviews.stream().filter(p -> "EXISTS".equals(p.status())).count(); | |
| int alreadyInTeamCount = (int) playerPreviews.stream().filter(p -> "ALREADY_IN_TEAM".equals(p.status())).count(); | |
| Summary summary = new Summary(playerPreviews.size(), newCount, existsCount, alreadyInTeamCount); | |
| Map<String, Long> counts = playerPreviews.stream() | |
| .collect(Collectors.groupingBy(PlayerPreview::status, Collectors.counting())); | |
| int newCount = counts.getOrDefault("NEW", 0L).intValue(); | |
| int existsCount = counts.getOrDefault("EXISTS", 0L).intValue(); | |
| int alreadyInTeamCount = counts.getOrDefault("ALREADY_IN_TEAM", 0L).intValue(); | |
| Summary summary = new Summary(playerPreviews.size(), newCount, existsCount, alreadyInTeamCount); |
| @RequiredArgsConstructor | ||
| public class NlService { | ||
|
|
||
| private final NlGeminiClient nlGeminiClient; |
There was a problem hiding this comment.
개인적인 생각이긴 한데, NlClient 로 추상화 하고 gemini 라는 구체적인 맥락은 추상화하는 게 어떨까요?
요금 정책이나.. 여러가지 상황에 따라 어떤 LLM 을 사용하는지는 실제로 상황에 따라 다양하게 달라지지 않을까 싶습니당
클라이언트 뿐만 아니라 응답 자체도 지금 gemini 와 맥락이 깊게 연관돼 있는 것 같은데 한층 추상화 해둬주시면 좋을 것 같아요~!
| if (!geminiResponse.hasFunctionCall()) { | ||
| return new NlProcessResponse( | ||
| geminiResponse.getText().isEmpty() | ||
| ? NlErrorMessages.PARSE_FAILED | ||
| : geminiResponse.getText(), | ||
| null | ||
| ); |
There was a problem hiding this comment.
개인적인 생각이긴한데, 삼항연산자 가독성이 좀 많이 떨어지는 것 같아요.
이정도는 if 문으로 얼마든지 커버 가능하니.. 개선해보심이 어떨까 싶어요.
클린코드에서도 실제로 삼항연산자는 지양하라는 내용이 있어요.
삼항 연산자
기본적으로 클린코드 책에서는 삼항 연산자 사용을 지양한다.
단, 삼항 연산 자체가 null 체크 등 명확할 경우 사용해도 괜찮다.
| @@ -0,0 +1,250 @@ | |||
| package com.sports.server.command.nl.application; | |||
There was a problem hiding this comment.
---
전체 구조 개요
2단계 API로 구성됩니다. process(미리보기) → execute(실제 등록) 순서로 호출합니다.
---
1단계: POST /nl/process — 미리보기
요청 형태:
{
"leagueId": 186,
"teamId": 1,
"history": [], // 이전 대화 히스토리 (multi-turn 지원)
"message": "홍길동 202600001 10\n김철수 202600002 7"
}
흐름:
[클라이언트] 자연어 텍스트 전송
↓
[NlController.process()]
↓
[NlService.process()]
① 권한 검증: league 멤버인지, team이 해당 league 소속인지 확인
↓
② NlGeminiClient.parsePlayers(message, history) 호출
- Gemini API에 System Prompt + 대화 히스토리 + 현재 메시지 전송
- Function Calling 모드로 강제 호출 (mode: "ANY")
- Gemini가 parse_players({players: [...]}) 형태로 응답
↓
③ Gemini가 Function Call을 안 했으면 → 텍스트 메시지 그대로 반환
(예: "선수 정보를 입력해주세요.")
↓
④ 원본 텍스트에서 9자리 숫자 직접 추출 (정규식)
→ Gemini가 학번을 "보정"하는 hallucination 방지용 대조 집합
↓
⑤ 파싱된 선수 목록 순회하며 각각 상태 분류:
- 원본에 없는 학번 / 9자리 아님 → parseFailedLines에 추가
- 입력 내 중복 학번 → DUPLICATE_IN_INPUT
- DB에 없음 → NEW
- DB에 있지만 팀에 없음 → EXISTS (existingPlayerId 포함)
- DB에 있고 이미 팀 소속 → ALREADY_IN_TEAM
↓
⑥ 미리보기 응답 반환
응답 형태:
{
"displayMessage": "정치외교학과 DPS에 2명의 선수를 등록합니다. 확인해주세요.",
"preview": {
"type": "REGISTER_PLAYERS_BULK",
"teamId": 1,
"teamName": "정치외교학과 DPS",
"players": [
{ "name": "홍길동", "studentNumber": "202600001", "jerseyNumber": 10, "status": "NEW", "existingPlayerId": null },
{ "name": "김철수", "studentNumber": "202600002", "jerseyNumber": 7, "status": "EXISTS", "existingPlayerId": 42 }
],
"summary": { "total": 2, "newPlayers": 1, "existingPlayers": 1, "alreadyInTeam": 0 },
"parseFailedLines": []
}
}
---
2단계: POST /nl/execute — 실제 등록
클라이언트가 미리보기를 사용자에게 보여주고, 확인 후 이 API를 호출합니다.
요청 형태:
{
"leagueId": 186,
"teamId": 1,
"players": [
{ "name": "홍길동", "studentNumber": "202600001", "jerseyNumber": 10 },
{ "name": "김철수", "studentNumber": "202600002", "jerseyNumber": 7 }
]
}
흐름:
[클라이언트] 확인된 선수 목록 전송
↓
[NlService.execute()]
① 동일한 권한 검증
↓
② 학번 목록으로 DB 일괄 조회 (N+1 방지)
↓
③ 선수 목록 순회:
- 중복 학번 → skipped++
- DB에 없음 → PlayerService.register() 로 신규 생성 → created++
- DB에 있고 이미 팀 소속 → skipped++
- DB에 있고 팀 미소속 → 기존 ID 사용
→ teamPlayerRegisters 리스트에 (playerId, jerseyNumber) 추가 → assigned++
↓
④ TeamService.addPlayersToTeam() 으로 일괄 팀 배정
↓
⑤ 결과 반환
응답 형태:
{
"displayMessage": "정치외교학과 DPS에 2명의 선수가 등록되었습니다.",
"result": { "created": 1, "assigned": 2, "skipped": 0 }
}
---
Gemini 연동 핵심 포인트
NlGeminiClient가 Gemini API와 통신하는 방식입니다.
요청 구조:
systemInstruction: "너는 선수 등록 어시스턴트야..." (고정)
contents: [ ...history, { role: "user", parts: [message] } ]
tools: [ parse_players 함수 스키마 ]
toolConfig: { mode: "ANY" } ← 반드시 함수 호출하도록 강제
응답 구조 (GeminiFunctionCallResponse):
candidates[0].content.parts[0].functionCall
.name: "parse_players"
.args: { players: [ {name, studentNumber, jerseyNumber}, ... ] }
mode: "ANY" 덕분에 Gemini는 항상 parse_players를 호출해야 합니다. 만약 텍스트로만 응답하면 hasFunctionCall() == false가 되어 에러 메시지를 반환합니다.
---
학번 원본 대조 검증의 역할
입력: "홍길동 20260001 10" ← 8자리 (오타)
↓
Gemini가 "202600001"로 보정해서 반환할 수 있음 (hallucination)
↓
원본에서 9자리 숫자 추출 → {} (비어있음)
↓
"202600001" ∉ 원본 집합 → parseFailedLines에 추가
LLM이 임의로 학번을 만들어내는 것을 원본 텍스트와 대조해서 차단하는 안전장치입니다.
이런 식으로 플로우를 정리해서 문서화의 측면에서.. PR 본문에 남겨주심 좋을 것 같아요
| @@ -0,0 +1,250 @@ | |||
| package com.sports.server.command.nl.application; | |||
|
|
|||
There was a problem hiding this comment.
전체적으로 메서드 하나가 너무 길고 메서드별로 분리가 안 돼 있어서 가독성이 너무 떨어져요 ㅠㅠ 신경 써주심 좋을 것 같아요!
| Map<String, Object> args = geminiResponse.getFunctionCall().args(); | ||
| @SuppressWarnings("unchecked") | ||
| List<Map<String, Object>> parsedPlayers = (List<Map<String, Object>>) args.get("players"); | ||
|
|
There was a problem hiding this comment.
이거 warning 을 supress 하도록 하지 말고, Map, Object 를 묶어서 적절한 dto 로 매핑해서 타입 안정성을 지켜보심이 어떨까 싶어요!
예시..
GeminiFunctionCallArgs args = geminiResponse.getArgsAs(objectMapper, GeminiFunctionCallArgs.class);
List<GeminiFunctionCallArgs.ParsedPlayer> parsedPlayers = args.players();
// 필드 접근도 타입 안전
String name = parsed.name();
String studentNumber = parsed.studentNumber();
Integer jerseyNumber = parsed.jerseyNumber(); // Jackson이 Integer로 바로 바인딩| @@ -0,0 +1,121 @@ | |||
| package com.sports.server.command.nl.infra; | |||
There was a problem hiding this comment.
테스트에서 프린트문보다는 assert문을 활용해보는 게 어떨까요~
디버깅 할 때 이외에는 프린트문을 제거해주시면 좋을 것 같아요!
| 너는 스포츠 리그 관리 시스템의 선수 등록 어시스턴트야. | ||
| 사용자가 입력한 텍스트에서 선수 정보를 추출하는 것이 너의 역할이야. | ||
|
|
||
| 각 선수에 대해 다음 정보를 추출해: | ||
| - name: 선수 이름 (한글) | ||
| - studentNumber: 학번 (정확히 9자리 숫자) | ||
| - jerseyNumber: 등번호 (1~99 사이 숫자, 없으면 생략) | ||
|
|
||
| 입력 텍스트는 다양한 형태일 수 있어: | ||
| - 공백/탭/쉼표로 구분된 형태 | ||
| - 괄호나 슬래시가 포함된 형태 | ||
| - 순서가 뒤바뀐 형태 (학번이 먼저 올 수도 있음) | ||
| - 등번호가 없는 경우도 있음 | ||
|
|
||
| 중요 규칙: | ||
| - studentNumber는 반드시 원본 텍스트에 존재하는 9자리 연속 숫자여야 해 | ||
| - 9자리가 아닌 숫자를 학번으로 추측하거나 보정하지 마 | ||
| - 빈 줄이나 의미 없는 텍스트는 무시해 | ||
| - 파싱할 수 있는 선수 정보만 추출해 | ||
|
|
||
| 반드시 parse_players 함수를 호출해서 응답해. |
There was a problem hiding this comment.
요론 것들도 환경변수로 빼봅시다~
그렇게 되면 매번 코드 상의 수정을 거치지 않아도 프롬프트를 깎을 수 있다는 장점이 있어요
There was a problem hiding this comment.
이거 빼서 be-config에 pr 올려뒀습니당
| @@ -0,0 +1,395 @@ | |||
| package com.sports.server.command.nl.application; | |||
There was a problem hiding this comment.
- 실제로 gemini client 호출이 잘 되는지
- 자연어 기반의 요청을 어라만 이해하는지
- DB 에 정확히 잘 반영하는지
지금 테스트들이 전체적으로 모킹하는 방식이라, 위의 세 가지에 대한 검증을 장담할 수 없을 것 같아요.
- 실제로 API 호출하는 코드를 작성하고
@Disabled를 붙여서 매번 돌지 않게 하거나, - 실제로 API 요청 해보시고 관련된 요청 / 응답값을 공유해주시면 좋을 것 같아요.
저는 전자의 방식을 더 선호하는데요, 코드를 읽는 사람 입장에서도 이 기능이 어떤 식으로 요청이 오가고 동작하는지 더 빠르게 이해할 수 있을 것 같아요. 빈약하지만 일종의 예시..
There was a problem hiding this comment.
@Disabled로 ManualTest 작성해뒀습니다! 추가로 pr 본문에 응답/요청 예시도 적어뒀어요
TAEW00KIM
force-pushed
the
feature/nl-bulk-player-register
branch
from
43f2779 to
caeed00
Compare
Jin409
left a comment
Jin409
left a comment
There was a problem hiding this comment.
이 코멘트들만 개선해주시면 한번 머지해보고 더 프롬프트 깎아 봐요..!
| } | ||
|
|
||
| @Transactional | ||
| public NlExecuteResponse execute(NlExecuteRequest request, Member member) { |
There was a problem hiding this comment.
execute 쪽도 메서드 분리가 필요해 보여요~!
| return new NlExecuteResponse(displayMessage, new NlExecuteResponse.Result(created, assigned, skipped)); | ||
| } | ||
|
|
||
| private NlProcessResponse buildPreview(NlProcessRequest request, Team team, List<ParsedPlayer> parsedPlayers) { |
2 participants
이슈 배경
기존의 문제
해결 방식
POST /nl/process(파싱+검증+프리뷰) →POST /nl/execute(등록 실행)전체 구조 개요
2단계 API로 구성됩니다. process(미리보기) → execute(실제 등록) 순서로 호출합니다.
1단계: POST /nl/process — 미리보기
요청:
{ "leagueId": 186, "teamId": 1, "history": [], "message": "홍길동 202600001 10\n김철수 202600002 7" }흐름:
[클라이언트] 자연어 텍스트 전송
↓
[NlController.process()]
↓
[NlService.process()]
① 권한 검증: league 멤버인지, team이 해당 league 소속인지 확인
↓
② NlClient.parsePlayers(message, history) 호출
- NlClient 인터페이스를 통해 LLM에 파싱 요청
- 현재 구현체: NlGeminiClient (Gemini Function Calling)
- NlParseResult로 변환하여 반환 (LLM 구현체 무관)
↓
③ 파싱 실패 시 → 텍스트 메시지 그대로 반환
(예: "선수 정보를 입력해주세요.")
↓
④ 원본 텍스트에서 9자리 숫자 직접 추출 (정규식)
→ LLM이 학번을 "보정"하는 hallucination 방지용 대조 집합
↓
⑤ 파싱된 선수 목록 순회하며 각각 상태 분류:
- 원본에 없는 학번 / 9자리 아님 → parseFailedLines에 추가
- 입력 내 중복 학번 → DUPLICATE_IN_INPUT
- DB에 없음 → NEW
- DB에 있지만 팀에 없음 → EXISTS (existingPlayerId 포함)
- DB에 있고 이미 팀 소속 → ALREADY_IN_TEAM
↓
⑥ 미리보기 응답 반환
응답:
{ "displayMessage": "정치외교학과 DPS에 2명의 선수를 등록합니다. 확인해주세요.", "preview": { "type": "REGISTER_PLAYERS_BULK", "teamId": 1, "teamName": "정치외교학과 DPS", "players": [ { "name": "홍길동", "studentNumber": "202600001", "jerseyNumber": 10, "status": "NEW", "existingPlayerId": null }, { "name": "김철수", "studentNumber": "202600002", "jerseyNumber": 7, "status": "EXISTS", "existingPlayerId": 42 } ], "summary": { "total": 2, "newPlayers": 1, "existingPlayers": 1, "alreadyInTeam": 0 }, "parseFailedLines": [] } }2단계: POST /nl/execute — 실제 등록
클라이언트가 미리보기를 사용자에게 보여주고, 확인 후 이 API를 호출합니다.
요청:
{ "leagueId": 186, "teamId": 1, "players": [ { "name": "홍길동", "studentNumber": "202600001", "jerseyNumber": 10 }, { "name": "김철수", "studentNumber": "202600002", "jerseyNumber": 7 } ] }흐름:
[클라이언트] 확인된 선수 목록 전송
↓
[NlService.execute()]
① 동일한 권한 검증
↓
② 학번 목록으로 DB 일괄 조회 (N+1 방지)
↓
③ 선수 목록 순회:
- 중복 학번 → skipped++
- DB에 없음 → PlayerService.register()로 신규 생성 → created++
- DB에 있고 이미 팀 소속 → skipped++
- DB에 있고 팀 미소속 → 기존 ID 사용 → assigned++
↓
④ TeamService.addPlayersToTeam()으로 일괄 팀 배정
↓
⑤ 결과 반환
응답:
{
"displayMessage": "정치외교학과 DPS에 2명의 선수가 등록되었습니다.",
"result": { "created": 1, "assigned": 2, "skipped": 0 }
}
학번 원본 대조 검증
입력: "홍길동 20260001 10" ← 8자리 (오타)
↓
LLM이 "202600001"로 보정해서 반환할 수 있음 (hallucination)
↓
원본에서 9자리 숫자 추출 → {} (비어있음)
↓
"202600001" ∉ 원본 집합 → parseFailedLines에 추가
LLM이 임의로 학번을 만들어내는 것을 원본 텍스트와 대조해서 차단하는 안전장치입니다.
설계 포인트
확인해야 할 부분
영향 범위 / 사이드 이펙트