Feat/#147 예산 수정 - 예산 변경 이력 추적#148
Open
kingmingyu wants to merge 10 commits into
Open
Conversation
1 task
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
워크스루전체적으로 예산 변경 이력을 추적하고 조회하는 기능이 일관되게 잘 설계된 PR입니다. 네이버 API 통합부터 로컬 DB 동기화, 그리고 대시보드/타임라인 조회까지 기능이 체계적으로 전개되어 있습니다. 변경 사항예산 변경 이력 추적 기능
상세 검토 포인트✅ 잘된 부분
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 32.76% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | PR 제목이 핵심 변경사항을 명확하게 요약합니다. 예산 변경 이력 추적이라는 메인 목표를 간결하게 표현했습니다. |
| Linked Issues check | ✅ Passed | PR의 구현이 #147 이슈의 요구사항을 충족합니다. BudgetHistory 엔티티 생성, 예산 수정 시 히스토리 저장, 대시보드/타임라인에 반영 모두 구현되었습니다. |
| Out of Scope Changes check | ✅ Passed | 모든 변경사항이 예산 변경 이력 추적이라는 범위 내에 있습니다. 불필요한 리팩토링이나 무관한 변경은 발견되지 않습니다. |
| Description check | ✅ Passed | PR 설명이 기본 템플릿 구조를 잘 따르고 있으며, 개요, 작업 내용, 스크린샷, 체크리스트를 모두 제공하고 있습니다. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
feat/#147
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
src/main/java/com/whereyouad/WhereYouAd/domains/dashboard/presentation/docs/DashboardControllerDocs.java (1)
118-133: ⚡ Quick win📝 API 문서 누락: 400 응답 코드 추가 필요
다른 대시보드 API들(
getRoasRankingLine 72,getPlatformMetricFactsLine 107)에서는 잘못된 파라미터에 대한 400 에러 응답을 문서화하고 있는데, 이 메서드에는 빠져있네요.특히
DashboardServiceImpl에 날짜 검증 로직이 추가되면(위에서 제안한 대로), "시작일이 종료일보다 늦은 경우" 또는 "미래 날짜 입력" 등의 400 에러 케이스를 명시해야 프론트엔드 개발자가 에러 핸들링을 제대로 구현할 수 있어요.📋 문서 보완 제안
`@ApiResponses`({ `@ApiResponse`(responseCode = "200", description = "성공"), + `@ApiResponse`(responseCode = "400", description = "시작일이 종료일보다 늦거나 미래 날짜인 경우"), `@ApiResponse`(responseCode = "403", description = "해당 조직에 대한 접근 권한이 없는 경우"), `@ApiResponse`(responseCode = "404", description = "존재하지 않는 조직") })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/whereyouad/WhereYouAd/domains/dashboard/presentation/docs/DashboardControllerDocs.java` around lines 118 - 133, Add a 400 ApiResponse to the getBudgetHistory method in DashboardControllerDocs (method: getBudgetHistory) to document invalid-parameter errors (e.g., startDate after endDate, future date) similar to getRoasRanking and getPlatformMetricFacts; update the `@ApiResponses` array to include `@ApiResponse`(responseCode = "400", description = "잘못된 요청: 날짜 파라미터 검증 실패(시작일>종료일, 미래 날짜 등)") so frontend can handle those validation errors (ensure this matches any validation/exception cases thrown by DashboardServiceImpl date validation).src/main/java/com/whereyouad/WhereYouAd/infrastructure/client/naver/converter/NaverConverter.java (1)
63-64: ⚡ Quick win반복되는 일일 예산 추출 로직을 헬퍼 메서드로 분리하면 좋겠습니다.
dto.useDailyBudget() != null && dto.useDailyBudget() ? dto.dailyBudget() : null패턴이 여러 곳에서 반복됩니다:
- Line 63 (AdGroup 생성)
- Line 76 (AdGroup 업데이트)
- Line 42 (AdCampaign 업데이트)
이를 헬퍼 메서드로 추출하면 코드 가독성과 유지보수성이 향상됩니다.
♻️ 리팩토링 제안
// NaverConverter 클래스 하단에 추가 private static Long extractDailyBudgetIfUsed(Boolean useDailyBudget, Long dailyBudget) { return Boolean.TRUE.equals(useDailyBudget) ? dailyBudget : null; }그러면 사용하는 곳에서:
// Line 63-64 .budget(extractDailyBudgetIfUsed(dto.useDailyBudget(), dto.dailyBudget())) .bidAmount(dto.bidAmt()) // Line 76-77 entity.updateBudget( extractDailyBudgetIfUsed(dto.useDailyBudget(), dto.dailyBudget()), dto.bidAmt() );추가 개선: Java 17의 Record pattern을 활용하면 더 깔끔하게 만들 수도 있습니다:
private static Long extractDailyBudgetIfUsed(NaverDTO.AdGroupResponse dto) { return Boolean.TRUE.equals(dto.useDailyBudget()) ? dto.dailyBudget() : null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/whereyouad/WhereYouAd/infrastructure/client/naver/converter/NaverConverter.java` around lines 63 - 64, Extract the repeated daily-budget conditional into a private helper on NaverConverter (e.g., private static Long extractDailyBudgetIfUsed(Boolean useDailyBudget, Long dailyBudget) or an overload taking the DTO) and replace all occurrences of the pattern dto.useDailyBudget() != null && dto.useDailyBudget() ? dto.dailyBudget() : null with calls to extractDailyBudgetIfUsed(dto.useDailyBudget(), dto.dailyBudget()); update both the .budget(...) call in the AdGroup builder and the entity.updateBudget(...) call (and the AdCampaign update site) to use this helper to improve readability and avoid duplication.src/main/java/com/whereyouad/WhereYouAd/infrastructure/client/naver/dto/NaverDTO.java (1)
137-163: 💤 Low value[선택적 개선] DTO 레벨에서 validation 어노테이션 추가 고려
현재는 서비스 계층(NaverAdApiService)에서 예산 범위와 10 단위 배수를 검증하고 있는데, 컨트롤러 입력용 Request DTO에
@NotNull,@Min,@Max등의 validation 어노테이션을 추가하면 더 명확하고 빠른 실패(fail-fast)가 가능합니다.예시:
public record UpdateCampaignBudgetRequest( `@NotNull` Boolean useDailyBudget, `@Min`(50) `@Max`(1_000_000_000) Long dailyBudget ) {}다만 10 단위 배수 검증은 커스텀 validator가 필요하므로, 현재처럼 서비스 레이어에서 처리하는 것도 합리적입니다. 선택적으로 개선을 고려해보세요!
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/whereyouad/WhereYouAd/infrastructure/client/naver/dto/NaverDTO.java` around lines 137 - 163, Add bean validation annotations to controller-facing request records to enable fail-fast validation: annotate UpdateCampaignBudgetRequest.useDailyBudget with `@NotNull` and UpdateCampaignBudgetRequest.dailyBudget with `@NotNull` plus `@Min/`@Max bounds; do the same for UpdateAdGroupBudgetRequest (useDailyBudget, dailyBudget, bidAmt) with appropriate `@NotNull/`@Min/@Max constraints. Keep 10-unit multiple checks in NaverAdApiService as noted (or implement a custom constraint if you want that moved to DTOs). Ensure imports for javax/ jakarta validation annotations are added and that controllers trigger validation (e.g., `@Valid` on parameters).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/NaverAdApiService.java`:
- Around line 239-274: The current NaverAdApiService method that calls
naverClient.updateAdGroupBudget twice can produce partial-success
inconsistencies: wrap each naverClient.updateAdGroupBudget call in its own
try-catch and track per-operation flags (e.g., budgetUpdated, bidAmtUpdated) so
you can log/handle partial success before throwing; after each successful remote
update, persist corresponding history via budgetHistoryRepository (use the
adGroupRepository.findByAdCampaign_PlatformAccountAndExternalGroupId(...) result
and avoid silent skips by replacing ifPresent with explicit handling — either
orElseThrow or log a warning when absent) and on failure log that the other side
was already changed (include adgroupId and which flag is true) then rethrow
AdvertisementHandler(NaverAdErrorCode.NAVER_AD_GROUP_BUDGET_UPDATE_FAILED) as
appropriate.
- Around line 203-210: The current updateCampaignBudget() only updates budget
and saves history inside
adCampaignRepository.findByPlatformAccountAndExternalCampaignId(...).ifPresent(...),
so when the local AdCampaign is missing the Naver API call can succeed but no
local budget/history is recorded; change this to use ifPresentOrElse on the
Optional returned by findByPlatformAccountAndExternalCampaignId(...) and in the
present branch perform campaign.updateBudget(...) and
budgetHistoryRepository.save(...), and in the else branch either log a warning
with identifying info and trigger NaverAdSyncService.syncAllMetadata() (or
enqueue a sync/retry) or throw a clear exception so callers can handle it—ensure
the branch includes userId and Provider.NAVER context in logs/operations.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/entity/AdCampaign.java`:
- Around line 98-100: AdCampaign.updateBudget currently assigns budget directly
and should mirror AdGroup.updateBudget's defensive null check; modify
AdCampaign.updateBudget so it only sets this.budget when the passed budget is
not null (i.e., add an if (budget != null) guard) to prevent unintentionally
overwriting the existing budget.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/entity/BudgetHistory.java`:
- Around line 39-46: BudgetHistory currently allows both adCampaign and adGroup
to be null or both set; add a validation to enforce exactly one parent:
implement two factory/static constructors (e.g., BudgetHistory.forCampaign(...)
and BudgetHistory.forAdGroup(...)) that validate the non-null parent and set
only the corresponding field, or add a check in the existing
BudgetHistory.builder()/build() to throw IllegalArgumentException if (adCampaign
== null && adGroup == null) or (adCampaign != null && adGroup != null);
alternatively (or in addition) add a DB CHECK constraint named like
check_single_parent to enforce (ad_campaign_id IS NOT NULL AND ad_group_id IS
NULL) OR (ad_campaign_id IS NULL AND ad_group_id IS NOT NULL).
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/repository/BudgetHistoryRepository.java`:
- Around line 13-25: The current repository method findByOrgAndPeriod in
BudgetHistoryRepository uses an OR on ac.organization.id and agc.organization.id
which hurts index usage; replace it by either (A) two repository
methods—findCampaignHistoryByOrgAndPeriod (JOIN bh.adCampaign ac WHERE
ac.organization.id = :orgId AND bh.createdAt BETWEEN :start AND :end) and
findAdGroupHistoryByOrgAndPeriod (JOIN bh.adGroup ag JOIN ag.adCampaign agc
WHERE agc.organization.id = :orgId AND bh.createdAt BETWEEN :start AND :end)—and
merge+sort results by BudgetHistory::getCreatedAt desc in the service, or (B)
use a native SQL UNION ALL query that selects both sets and orders by created_at
desc if you prefer a single DB call; implement one of these in place of
findByOrgAndPeriod to avoid the OR condition.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/dashboard/application/mapper/DashboardConverter.java`:
- Around line 56-70: The toBudgetHistoryItems method can NPE when both
adCampaign and adGroup are null; add a defensive check in
DashboardConverter.toBudgetHistoryItems to handle that case (e.g., if
bh.getAdCampaign()==null && bh.getAdGroup()==null) and either throw a clear
IllegalStateException including the BudgetHistory identifier (or set a safe
default like "UNKNOWN_TARGET") instead of calling getName() on a null, and
additionally add entity-level validation in BudgetHistory (e.g., a
`@PrePersist/`@PreUpdate method) to assert that at least one of adCampaign or
adGroup is non-null to prevent invalid persisted state.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/dashboard/domain/service/DashboardServiceImpl.java`:
- Around line 414-431: Add the same date validation used in getRoasRanking and
getOngoingAdCountByProvider to getBudgetHistory: after org/orgMember checks,
validate that neither startDate nor endDate is in the future (compare to
LocalDate.now()) and that startDate.isAfter(endDate) is not true; when a check
fails, throw a DashboardException with the same error codes used by the other
methods (use the same DashboardErrorCode constants for future-date and
invalid-range errors) so the method consistently rejects invalid date inputs
before querying budgetHistoryRepository.
- Around line 422-426: In DashboardServiceImpl#getBudgetHistory add the same
date validation used in getRoasRanking to reject or swap requests where
startDate is after endDate (preventing future/invalid ranges) and return/throw
accordingly; then update BudgetHistoryRepository#findByOrgAndPeriod to eagerly
fetch related entities (either convert the JPQL/Criteria to use JOIN FETCH for
adCampaign and adGroup or add an `@EntityGraph` including those associations) so
that DashboardConverter.toBudgetHistoryItems accessing
bh.getAdCampaign().getName() and bh.getAdGroup().getName() does not trigger N+1
lazy loads.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/timeline/application/dto/response/TimelineResponse.java`:
- Around line 48-55: Extract the duplicated record into a shared DTO (e.g.,
create BudgetHistoryDTO record with fields BudgetFieldType fieldType, String
targetName, Long previousValue, Long newValue, LocalDateTime changedAt, Provider
provider) and replace TimelineResponse.BudgetHistoryItem and
DashboardResponse.BudgetHistoryItem usages with this shared BudgetHistoryDTO,
updating imports and any constructor/factory code that referenced the old nested
records so both TimelineResponse and DashboardResponse reuse the common DTO.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/timeline/application/mapper/TimelineConverter.java`:
- Around line 78-92: Refactor the duplicated budget-history mapping by
extracting the common logic used in TimelineConverter.toBudgetHistoryItems and
DashboardConverter.toBudgetHistoryItems into a shared utility (or add a helper
method on the BudgetHistory entity like getTargetName()); update both converters
to call that helper. Also harden against NPEs by ensuring the helper (or the
mapping) checks both bh.getAdCampaign() and bh.getAdGroup() and returns a safe
fallback (e.g., empty string or "unknown") if both are null before constructing
TimelineResponse.BudgetHistoryItem, leaving other fields unchanged.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/timeline/domain/service/TimelineServiceImpl.java`:
- Around line 260-266: The BudgetHistory fetch causes an N+1 because
TimelineServiceImpl calls budgetHistoryRepository.findByOrgAndPeriod and then
TimelineConverter.toBudgetHistoryItems accesses bh.getAdCampaign().getName() and
bh.getAdGroup().getName(); update BudgetHistoryRepository.findByOrgAndPeriod to
eagerly load associations (e.g., add LEFT JOIN FETCH bh.adCampaign and LEFT JOIN
FETCH bh.adGroup — and if adGroup references campaign add LEFT JOIN FETCH
ag.adCampaign) or annotate the repository method with an `@EntityGraph` that
includes adCampaign and adGroup, ensuring the original WHERE predicate that
filters by organization (currently tied to ac/agc) is preserved.
---
Nitpick comments:
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/dashboard/presentation/docs/DashboardControllerDocs.java`:
- Around line 118-133: Add a 400 ApiResponse to the getBudgetHistory method in
DashboardControllerDocs (method: getBudgetHistory) to document invalid-parameter
errors (e.g., startDate after endDate, future date) similar to getRoasRanking
and getPlatformMetricFacts; update the `@ApiResponses` array to include
`@ApiResponse`(responseCode = "400", description = "잘못된 요청: 날짜 파라미터 검증 실패(시작일>종료일,
미래 날짜 등)") so frontend can handle those validation errors (ensure this matches
any validation/exception cases thrown by DashboardServiceImpl date validation).
In
`@src/main/java/com/whereyouad/WhereYouAd/infrastructure/client/naver/converter/NaverConverter.java`:
- Around line 63-64: Extract the repeated daily-budget conditional into a
private helper on NaverConverter (e.g., private static Long
extractDailyBudgetIfUsed(Boolean useDailyBudget, Long dailyBudget) or an
overload taking the DTO) and replace all occurrences of the pattern
dto.useDailyBudget() != null && dto.useDailyBudget() ? dto.dailyBudget() : null
with calls to extractDailyBudgetIfUsed(dto.useDailyBudget(), dto.dailyBudget());
update both the .budget(...) call in the AdGroup builder and the
entity.updateBudget(...) call (and the AdCampaign update site) to use this
helper to improve readability and avoid duplication.
In
`@src/main/java/com/whereyouad/WhereYouAd/infrastructure/client/naver/dto/NaverDTO.java`:
- Around line 137-163: Add bean validation annotations to controller-facing
request records to enable fail-fast validation: annotate
UpdateCampaignBudgetRequest.useDailyBudget with `@NotNull` and
UpdateCampaignBudgetRequest.dailyBudget with `@NotNull` plus `@Min/`@Max bounds; do
the same for UpdateAdGroupBudgetRequest (useDailyBudget, dailyBudget, bidAmt)
with appropriate `@NotNull/`@Min/@Max constraints. Keep 10-unit multiple checks in
NaverAdApiService as noted (or implement a custom constraint if you want that
moved to DTOs). Ensure imports for javax/ jakarta validation annotations are
added and that controllers trigger validation (e.g., `@Valid` on parameters).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5fd30de-8ba8-41e1-80c9-f403efecb49b
📒 Files selected for processing (22)
src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/application/mapper/AdvertisementConverter.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/constant/BudgetFieldType.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/NaverAdApiService.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/exception/code/NaverAdErrorCode.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/entity/AdCampaign.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/entity/AdGroup.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/entity/BudgetHistory.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/repository/BudgetHistoryRepository.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/presentation/NaverAdApiController.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/presentation/docs/NaverAdApiControllerDocs.javasrc/main/java/com/whereyouad/WhereYouAd/domains/dashboard/application/dto/response/DashboardResponse.javasrc/main/java/com/whereyouad/WhereYouAd/domains/dashboard/application/mapper/DashboardConverter.javasrc/main/java/com/whereyouad/WhereYouAd/domains/dashboard/domain/service/DashboardService.javasrc/main/java/com/whereyouad/WhereYouAd/domains/dashboard/domain/service/DashboardServiceImpl.javasrc/main/java/com/whereyouad/WhereYouAd/domains/dashboard/presentation/DashboardController.javasrc/main/java/com/whereyouad/WhereYouAd/domains/dashboard/presentation/docs/DashboardControllerDocs.javasrc/main/java/com/whereyouad/WhereYouAd/domains/timeline/application/dto/response/TimelineResponse.javasrc/main/java/com/whereyouad/WhereYouAd/domains/timeline/application/mapper/TimelineConverter.javasrc/main/java/com/whereyouad/WhereYouAd/domains/timeline/domain/service/TimelineServiceImpl.javasrc/main/java/com/whereyouad/WhereYouAd/infrastructure/client/naver/client/NaverClient.javasrc/main/java/com/whereyouad/WhereYouAd/infrastructure/client/naver/converter/NaverConverter.javasrc/main/java/com/whereyouad/WhereYouAd/infrastructure/client/naver/dto/NaverDTO.java
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 11
🧹 Nitpick comments (3)
src/main/java/com/whereyouad/WhereYouAd/domains/dashboard/presentation/docs/DashboardControllerDocs.java (1)
118-133: ⚡ Quick win📝 API 문서 누락: 400 응답 코드 추가 필요
다른 대시보드 API들(
getRoasRankingLine 72,getPlatformMetricFactsLine 107)에서는 잘못된 파라미터에 대한 400 에러 응답을 문서화하고 있는데, 이 메서드에는 빠져있네요.특히
DashboardServiceImpl에 날짜 검증 로직이 추가되면(위에서 제안한 대로), "시작일이 종료일보다 늦은 경우" 또는 "미래 날짜 입력" 등의 400 에러 케이스를 명시해야 프론트엔드 개발자가 에러 핸들링을 제대로 구현할 수 있어요.📋 문서 보완 제안
`@ApiResponses`({ `@ApiResponse`(responseCode = "200", description = "성공"), + `@ApiResponse`(responseCode = "400", description = "시작일이 종료일보다 늦거나 미래 날짜인 경우"), `@ApiResponse`(responseCode = "403", description = "해당 조직에 대한 접근 권한이 없는 경우"), `@ApiResponse`(responseCode = "404", description = "존재하지 않는 조직") })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/whereyouad/WhereYouAd/domains/dashboard/presentation/docs/DashboardControllerDocs.java` around lines 118 - 133, Add a 400 ApiResponse to the getBudgetHistory method in DashboardControllerDocs (method: getBudgetHistory) to document invalid-parameter errors (e.g., startDate after endDate, future date) similar to getRoasRanking and getPlatformMetricFacts; update the `@ApiResponses` array to include `@ApiResponse`(responseCode = "400", description = "잘못된 요청: 날짜 파라미터 검증 실패(시작일>종료일, 미래 날짜 등)") so frontend can handle those validation errors (ensure this matches any validation/exception cases thrown by DashboardServiceImpl date validation).src/main/java/com/whereyouad/WhereYouAd/infrastructure/client/naver/converter/NaverConverter.java (1)
63-64: ⚡ Quick win반복되는 일일 예산 추출 로직을 헬퍼 메서드로 분리하면 좋겠습니다.
dto.useDailyBudget() != null && dto.useDailyBudget() ? dto.dailyBudget() : null패턴이 여러 곳에서 반복됩니다:
- Line 63 (AdGroup 생성)
- Line 76 (AdGroup 업데이트)
- Line 42 (AdCampaign 업데이트)
이를 헬퍼 메서드로 추출하면 코드 가독성과 유지보수성이 향상됩니다.
♻️ 리팩토링 제안
// NaverConverter 클래스 하단에 추가 private static Long extractDailyBudgetIfUsed(Boolean useDailyBudget, Long dailyBudget) { return Boolean.TRUE.equals(useDailyBudget) ? dailyBudget : null; }그러면 사용하는 곳에서:
// Line 63-64 .budget(extractDailyBudgetIfUsed(dto.useDailyBudget(), dto.dailyBudget())) .bidAmount(dto.bidAmt()) // Line 76-77 entity.updateBudget( extractDailyBudgetIfUsed(dto.useDailyBudget(), dto.dailyBudget()), dto.bidAmt() );추가 개선: Java 17의 Record pattern을 활용하면 더 깔끔하게 만들 수도 있습니다:
private static Long extractDailyBudgetIfUsed(NaverDTO.AdGroupResponse dto) { return Boolean.TRUE.equals(dto.useDailyBudget()) ? dto.dailyBudget() : null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/whereyouad/WhereYouAd/infrastructure/client/naver/converter/NaverConverter.java` around lines 63 - 64, Extract the repeated daily-budget conditional into a private helper on NaverConverter (e.g., private static Long extractDailyBudgetIfUsed(Boolean useDailyBudget, Long dailyBudget) or an overload taking the DTO) and replace all occurrences of the pattern dto.useDailyBudget() != null && dto.useDailyBudget() ? dto.dailyBudget() : null with calls to extractDailyBudgetIfUsed(dto.useDailyBudget(), dto.dailyBudget()); update both the .budget(...) call in the AdGroup builder and the entity.updateBudget(...) call (and the AdCampaign update site) to use this helper to improve readability and avoid duplication.src/main/java/com/whereyouad/WhereYouAd/infrastructure/client/naver/dto/NaverDTO.java (1)
137-163: 💤 Low value[선택적 개선] DTO 레벨에서 validation 어노테이션 추가 고려
현재는 서비스 계층(NaverAdApiService)에서 예산 범위와 10 단위 배수를 검증하고 있는데, 컨트롤러 입력용 Request DTO에
@NotNull,@Min,@Max등의 validation 어노테이션을 추가하면 더 명확하고 빠른 실패(fail-fast)가 가능합니다.예시:
public record UpdateCampaignBudgetRequest( `@NotNull` Boolean useDailyBudget, `@Min`(50) `@Max`(1_000_000_000) Long dailyBudget ) {}다만 10 단위 배수 검증은 커스텀 validator가 필요하므로, 현재처럼 서비스 레이어에서 처리하는 것도 합리적입니다. 선택적으로 개선을 고려해보세요!
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/whereyouad/WhereYouAd/infrastructure/client/naver/dto/NaverDTO.java` around lines 137 - 163, Add bean validation annotations to controller-facing request records to enable fail-fast validation: annotate UpdateCampaignBudgetRequest.useDailyBudget with `@NotNull` and UpdateCampaignBudgetRequest.dailyBudget with `@NotNull` plus `@Min/`@Max bounds; do the same for UpdateAdGroupBudgetRequest (useDailyBudget, dailyBudget, bidAmt) with appropriate `@NotNull/`@Min/@Max constraints. Keep 10-unit multiple checks in NaverAdApiService as noted (or implement a custom constraint if you want that moved to DTOs). Ensure imports for javax/ jakarta validation annotations are added and that controllers trigger validation (e.g., `@Valid` on parameters).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/NaverAdApiService.java`:
- Around line 239-274: The current NaverAdApiService method that calls
naverClient.updateAdGroupBudget twice can produce partial-success
inconsistencies: wrap each naverClient.updateAdGroupBudget call in its own
try-catch and track per-operation flags (e.g., budgetUpdated, bidAmtUpdated) so
you can log/handle partial success before throwing; after each successful remote
update, persist corresponding history via budgetHistoryRepository (use the
adGroupRepository.findByAdCampaign_PlatformAccountAndExternalGroupId(...) result
and avoid silent skips by replacing ifPresent with explicit handling — either
orElseThrow or log a warning when absent) and on failure log that the other side
was already changed (include adgroupId and which flag is true) then rethrow
AdvertisementHandler(NaverAdErrorCode.NAVER_AD_GROUP_BUDGET_UPDATE_FAILED) as
appropriate.
- Around line 203-210: The current updateCampaignBudget() only updates budget
and saves history inside
adCampaignRepository.findByPlatformAccountAndExternalCampaignId(...).ifPresent(...),
so when the local AdCampaign is missing the Naver API call can succeed but no
local budget/history is recorded; change this to use ifPresentOrElse on the
Optional returned by findByPlatformAccountAndExternalCampaignId(...) and in the
present branch perform campaign.updateBudget(...) and
budgetHistoryRepository.save(...), and in the else branch either log a warning
with identifying info and trigger NaverAdSyncService.syncAllMetadata() (or
enqueue a sync/retry) or throw a clear exception so callers can handle it—ensure
the branch includes userId and Provider.NAVER context in logs/operations.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/entity/AdCampaign.java`:
- Around line 98-100: AdCampaign.updateBudget currently assigns budget directly
and should mirror AdGroup.updateBudget's defensive null check; modify
AdCampaign.updateBudget so it only sets this.budget when the passed budget is
not null (i.e., add an if (budget != null) guard) to prevent unintentionally
overwriting the existing budget.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/entity/BudgetHistory.java`:
- Around line 39-46: BudgetHistory currently allows both adCampaign and adGroup
to be null or both set; add a validation to enforce exactly one parent:
implement two factory/static constructors (e.g., BudgetHistory.forCampaign(...)
and BudgetHistory.forAdGroup(...)) that validate the non-null parent and set
only the corresponding field, or add a check in the existing
BudgetHistory.builder()/build() to throw IllegalArgumentException if (adCampaign
== null && adGroup == null) or (adCampaign != null && adGroup != null);
alternatively (or in addition) add a DB CHECK constraint named like
check_single_parent to enforce (ad_campaign_id IS NOT NULL AND ad_group_id IS
NULL) OR (ad_campaign_id IS NULL AND ad_group_id IS NOT NULL).
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/repository/BudgetHistoryRepository.java`:
- Around line 13-25: The current repository method findByOrgAndPeriod in
BudgetHistoryRepository uses an OR on ac.organization.id and agc.organization.id
which hurts index usage; replace it by either (A) two repository
methods—findCampaignHistoryByOrgAndPeriod (JOIN bh.adCampaign ac WHERE
ac.organization.id = :orgId AND bh.createdAt BETWEEN :start AND :end) and
findAdGroupHistoryByOrgAndPeriod (JOIN bh.adGroup ag JOIN ag.adCampaign agc
WHERE agc.organization.id = :orgId AND bh.createdAt BETWEEN :start AND :end)—and
merge+sort results by BudgetHistory::getCreatedAt desc in the service, or (B)
use a native SQL UNION ALL query that selects both sets and orders by created_at
desc if you prefer a single DB call; implement one of these in place of
findByOrgAndPeriod to avoid the OR condition.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/dashboard/application/mapper/DashboardConverter.java`:
- Around line 56-70: The toBudgetHistoryItems method can NPE when both
adCampaign and adGroup are null; add a defensive check in
DashboardConverter.toBudgetHistoryItems to handle that case (e.g., if
bh.getAdCampaign()==null && bh.getAdGroup()==null) and either throw a clear
IllegalStateException including the BudgetHistory identifier (or set a safe
default like "UNKNOWN_TARGET") instead of calling getName() on a null, and
additionally add entity-level validation in BudgetHistory (e.g., a
`@PrePersist/`@PreUpdate method) to assert that at least one of adCampaign or
adGroup is non-null to prevent invalid persisted state.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/dashboard/domain/service/DashboardServiceImpl.java`:
- Around line 414-431: Add the same date validation used in getRoasRanking and
getOngoingAdCountByProvider to getBudgetHistory: after org/orgMember checks,
validate that neither startDate nor endDate is in the future (compare to
LocalDate.now()) and that startDate.isAfter(endDate) is not true; when a check
fails, throw a DashboardException with the same error codes used by the other
methods (use the same DashboardErrorCode constants for future-date and
invalid-range errors) so the method consistently rejects invalid date inputs
before querying budgetHistoryRepository.
- Around line 422-426: In DashboardServiceImpl#getBudgetHistory add the same
date validation used in getRoasRanking to reject or swap requests where
startDate is after endDate (preventing future/invalid ranges) and return/throw
accordingly; then update BudgetHistoryRepository#findByOrgAndPeriod to eagerly
fetch related entities (either convert the JPQL/Criteria to use JOIN FETCH for
adCampaign and adGroup or add an `@EntityGraph` including those associations) so
that DashboardConverter.toBudgetHistoryItems accessing
bh.getAdCampaign().getName() and bh.getAdGroup().getName() does not trigger N+1
lazy loads.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/timeline/application/dto/response/TimelineResponse.java`:
- Around line 48-55: Extract the duplicated record into a shared DTO (e.g.,
create BudgetHistoryDTO record with fields BudgetFieldType fieldType, String
targetName, Long previousValue, Long newValue, LocalDateTime changedAt, Provider
provider) and replace TimelineResponse.BudgetHistoryItem and
DashboardResponse.BudgetHistoryItem usages with this shared BudgetHistoryDTO,
updating imports and any constructor/factory code that referenced the old nested
records so both TimelineResponse and DashboardResponse reuse the common DTO.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/timeline/application/mapper/TimelineConverter.java`:
- Around line 78-92: Refactor the duplicated budget-history mapping by
extracting the common logic used in TimelineConverter.toBudgetHistoryItems and
DashboardConverter.toBudgetHistoryItems into a shared utility (or add a helper
method on the BudgetHistory entity like getTargetName()); update both converters
to call that helper. Also harden against NPEs by ensuring the helper (or the
mapping) checks both bh.getAdCampaign() and bh.getAdGroup() and returns a safe
fallback (e.g., empty string or "unknown") if both are null before constructing
TimelineResponse.BudgetHistoryItem, leaving other fields unchanged.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/timeline/domain/service/TimelineServiceImpl.java`:
- Around line 260-266: The BudgetHistory fetch causes an N+1 because
TimelineServiceImpl calls budgetHistoryRepository.findByOrgAndPeriod and then
TimelineConverter.toBudgetHistoryItems accesses bh.getAdCampaign().getName() and
bh.getAdGroup().getName(); update BudgetHistoryRepository.findByOrgAndPeriod to
eagerly load associations (e.g., add LEFT JOIN FETCH bh.adCampaign and LEFT JOIN
FETCH bh.adGroup — and if adGroup references campaign add LEFT JOIN FETCH
ag.adCampaign) or annotate the repository method with an `@EntityGraph` that
includes adCampaign and adGroup, ensuring the original WHERE predicate that
filters by organization (currently tied to ac/agc) is preserved.
---
Nitpick comments:
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/dashboard/presentation/docs/DashboardControllerDocs.java`:
- Around line 118-133: Add a 400 ApiResponse to the getBudgetHistory method in
DashboardControllerDocs (method: getBudgetHistory) to document invalid-parameter
errors (e.g., startDate after endDate, future date) similar to getRoasRanking
and getPlatformMetricFacts; update the `@ApiResponses` array to include
`@ApiResponse`(responseCode = "400", description = "잘못된 요청: 날짜 파라미터 검증 실패(시작일>종료일,
미래 날짜 등)") so frontend can handle those validation errors (ensure this matches
any validation/exception cases thrown by DashboardServiceImpl date validation).
In
`@src/main/java/com/whereyouad/WhereYouAd/infrastructure/client/naver/converter/NaverConverter.java`:
- Around line 63-64: Extract the repeated daily-budget conditional into a
private helper on NaverConverter (e.g., private static Long
extractDailyBudgetIfUsed(Boolean useDailyBudget, Long dailyBudget) or an
overload taking the DTO) and replace all occurrences of the pattern
dto.useDailyBudget() != null && dto.useDailyBudget() ? dto.dailyBudget() : null
with calls to extractDailyBudgetIfUsed(dto.useDailyBudget(), dto.dailyBudget());
update both the .budget(...) call in the AdGroup builder and the
entity.updateBudget(...) call (and the AdCampaign update site) to use this
helper to improve readability and avoid duplication.
In
`@src/main/java/com/whereyouad/WhereYouAd/infrastructure/client/naver/dto/NaverDTO.java`:
- Around line 137-163: Add bean validation annotations to controller-facing
request records to enable fail-fast validation: annotate
UpdateCampaignBudgetRequest.useDailyBudget with `@NotNull` and
UpdateCampaignBudgetRequest.dailyBudget with `@NotNull` plus `@Min/`@Max bounds; do
the same for UpdateAdGroupBudgetRequest (useDailyBudget, dailyBudget, bidAmt)
with appropriate `@NotNull/`@Min/@Max constraints. Keep 10-unit multiple checks in
NaverAdApiService as noted (or implement a custom constraint if you want that
moved to DTOs). Ensure imports for javax/ jakarta validation annotations are
added and that controllers trigger validation (e.g., `@Valid` on parameters).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5fd30de-8ba8-41e1-80c9-f403efecb49b
📒 Files selected for processing (22)
src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/application/mapper/AdvertisementConverter.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/constant/BudgetFieldType.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/NaverAdApiService.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/exception/code/NaverAdErrorCode.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/entity/AdCampaign.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/entity/AdGroup.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/entity/BudgetHistory.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/repository/BudgetHistoryRepository.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/presentation/NaverAdApiController.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/presentation/docs/NaverAdApiControllerDocs.javasrc/main/java/com/whereyouad/WhereYouAd/domains/dashboard/application/dto/response/DashboardResponse.javasrc/main/java/com/whereyouad/WhereYouAd/domains/dashboard/application/mapper/DashboardConverter.javasrc/main/java/com/whereyouad/WhereYouAd/domains/dashboard/domain/service/DashboardService.javasrc/main/java/com/whereyouad/WhereYouAd/domains/dashboard/domain/service/DashboardServiceImpl.javasrc/main/java/com/whereyouad/WhereYouAd/domains/dashboard/presentation/DashboardController.javasrc/main/java/com/whereyouad/WhereYouAd/domains/dashboard/presentation/docs/DashboardControllerDocs.javasrc/main/java/com/whereyouad/WhereYouAd/domains/timeline/application/dto/response/TimelineResponse.javasrc/main/java/com/whereyouad/WhereYouAd/domains/timeline/application/mapper/TimelineConverter.javasrc/main/java/com/whereyouad/WhereYouAd/domains/timeline/domain/service/TimelineServiceImpl.javasrc/main/java/com/whereyouad/WhereYouAd/infrastructure/client/naver/client/NaverClient.javasrc/main/java/com/whereyouad/WhereYouAd/infrastructure/client/naver/converter/NaverConverter.javasrc/main/java/com/whereyouad/WhereYouAd/infrastructure/client/naver/dto/NaverDTO.java
🛑 Comments failed to post (11)
src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/NaverAdApiService.java (2)
203-210:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # 캠페인 예산 수정 API 호출 흐름 확인 rg -nP --type=java -C3 'updateCampaignBudget.*connectionId.*campaignId'Repository: WhereYouAd/WhereYouAd-Backend
Length of output: 2171
🏁 Script executed:
#!/bin/bash set -euo pipefail # 1) updateCampaignBudget 메서드 본문(203-210 포함) 확인 sed -n '160,260p' src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/NaverAdApiService.java # 2) 메타데이터 동기화가 캠페인을 upsert 하는지 확인(관련 서비스/스케줄러) rg -n --hidden -S "class NaverAdSyncService|syncAllMetadata\\(" src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service rg -n --hidden -S "class NaverAdScheduler|syncAllMetadata\\(" src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service # 3) syncAllMetadata 쪽에서 AdCampaign을 찾고 없으면 저장( upsert )하는지 확인 sed -n '1,260p' src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/NaverAdSyncService.java # 4) ifPresent로 인한 누락이 다른 곳에서도 동일 패턴인지(선택) 확인 rg -n "findByPlatformAccountAndExternalCampaignId\\(.*\\)\\s*\\.ifPresent" src/main/java/com/whereyouad/WhereYouAd/domains/advertisement -SRepository: WhereYouAd/WhereYouAd-Backend
Length of output: 19578
🏁 Script executed:
#!/bin/bash set -euo pipefail # 1) updateCampaignBudget 메서드 본문(203-210 포함) 확인 sed -n '160,260p' src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/NaverAdApiService.java # 2) 메타데이터 동기화가 캠페인을 upsert 하는지 확인(관련 서비스/스케줄러) rg -n --hidden -S "class NaverAdSyncService|syncAllMetadata\\(" src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service rg -n --hidden -S "class NaverAdScheduler|syncAllMetadata\\(" src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service # 3) syncAllMetadata 쪽에서 AdCampaign을 찾고 없으면 저장( upsert )하는지 확인 sed -n '1,260p' src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/NaverAdSyncService.java # 4) ifPresent로 인한 누락이 다른 곳에서도 동일 패턴인지(선택) 확인 rg -n "findByPlatformAccountAndExternalCampaignId\\(.*\\)\\s*\\.ifPresent" src/main/java/com/whereyouad/WhereYouAd/domains/advertisement -SRepository: WhereYouAd/WhereYouAd-Backend
Length of output: 19578
[NaverAdApiService] 캠페인 미존재 시 예산 변경 이력 누락 방지 필요
updateCampaignBudget()은 네이버 API 호출 후adCampaignRepository.findByPlatformAccountAndExternalCampaignId(...).ifPresent(...)안에서만campaign.updateBudget(...)과budgetHistoryRepository.save(...)가 실행됩니다. 즉 로컬에AdCampaign이 없으면 네이버는 성공해도 예산/변경 이력이 조용히 누락되고 그대로result만 반환됩니다.NaverAdSyncService.syncAllMetadata()는 upsert로 없으면 신규 저장까지 하지만, 이 동기화 흐름은 예산 수정 호출과 선행/보장 관계가 없어서 “성공했는데 대시보드/타임라인에 이력 없음”이 발생할 수 있습니다.ifPresentOrElse(미발견 시 경고 로그/예외/동기화 트리거/재시도 등)로 동작을 명확히 분기해 주세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/NaverAdApiService.java` around lines 203 - 210, The current updateCampaignBudget() only updates budget and saves history inside adCampaignRepository.findByPlatformAccountAndExternalCampaignId(...).ifPresent(...), so when the local AdCampaign is missing the Naver API call can succeed but no local budget/history is recorded; change this to use ifPresentOrElse on the Optional returned by findByPlatformAccountAndExternalCampaignId(...) and in the present branch perform campaign.updateBudget(...) and budgetHistoryRepository.save(...), and in the else branch either log a warning with identifying info and trigger NaverAdSyncService.syncAllMetadata() (or enqueue a sync/retry) or throw a clear exception so callers can handle it—ensure the branch includes userId and Provider.NAVER context in logs/operations.
239-274:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift[주의] 부분 실패 시나리오와 트랜잭션 일관성 검토 필요
네이버 API 제약으로 예산과 입찰가를 별도로 호출하는 것은 합리적이지만, 다음 시나리오를 고려해야 합니다:
문제 상황:
- 예산 업데이트 API 성공 (line 245-246)
- 입찰가 업데이트 API 실패 (line 249-250)
- 전체 catch 블록에서 예외 던짐 (line 272) → 트랜잭션 롤백
- 하지만 네이버 측에는 예산 변경이 이미 반영됨 → 로컬 DB와 불일치
추가 고려 사항:
- Lines 256-265:
request.dailyBudget()null 체크로 이력 저장 여부를 결정하는데, 만약 예산 API는 성공했지만 입찰가 API가 실패하면 이력이 저장되지 않습니다.- Line 253의
ifPresent패턴도 동일하게 광고그룹 미발견 시 조용히 스킵됩니다.권장사항:
- 두 API 호출 사이에 부분 성공 상태를 추적하고, 실패 시 이미 성공한 변경 사항을 로그에 명시
- 또는 각 API 호출을 별도 try-catch로 감싸서 부분 성공도 반영하도록 처리
- ifPresent 대신 orElseThrow 또는 최소한 경고 로그 추가
예시:
NaverDTO.AdGroupResponse result = null; boolean budgetUpdated = false; try { if (request.dailyBudget() != null || request.useDailyBudget() != null) { result = naverClient.updateAdGroupBudget(headers, adgroupId, "budget", new NaverDTO.UpdateAdGroupBudgetBody(adgroupId, request.useDailyBudget(), request.dailyBudget(), null)); budgetUpdated = true; } if (request.bidAmt() != null) { result = naverClient.updateAdGroupBudget(headers, adgroupId, "bidAmt", new NaverDTO.UpdateAdGroupBudgetBody(adgroupId, null, null, request.bidAmt())); } } catch (Exception e) { if (budgetUpdated) { log.error("[NAVER] 입찰가 수정 실패하였으나 예산은 이미 수정됨 - adgroupId={}, 수동 동기화 필요", adgroupId, e); } throw new AdvertisementHandler(NaverAdErrorCode.NAVER_AD_GROUP_BUDGET_UPDATE_FAILED); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/NaverAdApiService.java` around lines 239 - 274, The current NaverAdApiService method that calls naverClient.updateAdGroupBudget twice can produce partial-success inconsistencies: wrap each naverClient.updateAdGroupBudget call in its own try-catch and track per-operation flags (e.g., budgetUpdated, bidAmtUpdated) so you can log/handle partial success before throwing; after each successful remote update, persist corresponding history via budgetHistoryRepository (use the adGroupRepository.findByAdCampaign_PlatformAccountAndExternalGroupId(...) result and avoid silent skips by replacing ifPresent with explicit handling — either orElseThrow or log a warning when absent) and on failure log that the other side was already changed (include adgroupId and which flag is true) then rethrow AdvertisementHandler(NaverAdErrorCode.NAVER_AD_GROUP_BUDGET_UPDATE_FAILED) as appropriate.src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/entity/AdCampaign.java (1)
98-100:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdGroup.updateBudget과의 일관성을 위해 null 체크를 추가하는 것이 좋습니다.
AdGroup.updateBudget(Line 66)에서는if (budget != null)체크를 하지만, 여기서는 null 체크 없이 직접 할당합니다.두 엔티티가 유사한 역할의 메서드를 제공하므로, 동일한 방어 로직을 적용하는 것이 유지보수에 유리합니다:
public void updateBudget(Long budget) { if (budget != null) { this.budget = budget; } }이렇게 하면 null 값으로 의도치 않게 예산을 덮어쓰는 실수를 방지할 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/entity/AdCampaign.java` around lines 98 - 100, AdCampaign.updateBudget currently assigns budget directly and should mirror AdGroup.updateBudget's defensive null check; modify AdCampaign.updateBudget so it only sets this.budget when the passed budget is not null (i.e., add an if (budget != null) guard) to prevent unintentionally overwriting the existing budget.src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/entity/BudgetHistory.java (1)
39-46:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift예산 이력이 캠페인 또는 광고그룹 중 정확히 하나에만 연결되도록 보장하는 제약이 필요합니다.
주석에 "2개 중에 1개만 연결 가능"이라고 명시했지만, 현재 구조로는 다음 경우들을 막을 수 없습니다:
- 둘 다 null인 경우
- 둘 다 값이 있는 경우
다음 중 하나를 적용하는 것을 권장합니다:
해결 방안 1 (권장): Builder나 팩토리 메서드에서 검증 로직 추가
public static BudgetHistory forCampaign(BudgetFieldType fieldType, Long previousValue, Long newValue, Long changedBy, Provider provider, AdCampaign adCampaign) { if (adCampaign == null) { throw new IllegalArgumentException("Campaign budget history must have a campaign"); } return BudgetHistory.builder() .fieldType(fieldType) .previousValue(previousValue) .newValue(newValue) .changedBy(changedBy) .provider(provider) .adCampaign(adCampaign) .build(); } public static BudgetHistory forAdGroup(BudgetFieldType fieldType, Long previousValue, Long newValue, Long changedBy, Provider provider, AdGroup adGroup) { if (adGroup == null) { throw new IllegalArgumentException("AdGroup budget history must have an adGroup"); } return BudgetHistory.builder() .fieldType(fieldType) .previousValue(previousValue) .newValue(newValue) .changedBy(changedBy) .provider(provider) .adGroup(adGroup) .build(); }해결 방안 2: DB 레벨 CHECK 제약 조건 (PostgreSQL 예시)
ALTER TABLE budget_history ADD CONSTRAINT check_single_parent CHECK ( (ad_campaign_id IS NOT NULL AND ad_group_id IS NULL) OR (ad_campaign_id IS NULL AND ad_group_id IS NOT NULL) );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/entity/BudgetHistory.java` around lines 39 - 46, BudgetHistory currently allows both adCampaign and adGroup to be null or both set; add a validation to enforce exactly one parent: implement two factory/static constructors (e.g., BudgetHistory.forCampaign(...) and BudgetHistory.forAdGroup(...)) that validate the non-null parent and set only the corresponding field, or add a check in the existing BudgetHistory.builder()/build() to throw IllegalArgumentException if (adCampaign == null && adGroup == null) or (adCampaign != null && adGroup != null); alternatively (or in addition) add a DB CHECK constraint named like check_single_parent to enforce (ad_campaign_id IS NOT NULL AND ad_group_id IS NULL) OR (ad_campaign_id IS NULL AND ad_group_id IS NOT NULL).Source: Coding guidelines
src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/repository/BudgetHistoryRepository.java (1)
13-25:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftOR 조건 사용으로 인한 쿼리 성능 이슈가 예상됩니다.
현재 쿼리는
WHERE (ac.organization.id = :orgId OR agc.organization.id = :orgId)형태로 OR 조건을 사용하고 있습니다. 이 경우 다음 문제가 발생할 수 있습니다:
- 인덱스 최적화 어려움: 데이터베이스가 두 조건 중 어느 인덱스를 사용할지 판단하기 어려워 Full Table Scan이 발생할 수 있습니다.
- 대용량 데이터 처리 시 성능 저하: 조직의 예산 이력이 많아질수록 쿼리 성능이 급격히 나빠질 수 있습니다.
🔧 UNION ALL을 사용한 개선된 쿼리
`@Query`(""" SELECT bh FROM BudgetHistory bh JOIN bh.adCampaign ac WHERE ac.organization.id = :orgId AND bh.createdAt BETWEEN :start AND :end UNION ALL SELECT bh FROM BudgetHistory bh JOIN bh.adGroup ag JOIN ag.adCampaign agc WHERE agc.organization.id = :orgId AND bh.createdAt BETWEEN :start AND :end ORDER BY createdAt DESC """)이 방식은 각 서브쿼리가 독립적으로 인덱스를 활용할 수 있어 성능이 훨씬 좋습니다. 다만 JPQL에서 UNION ALL 지원 여부는 Hibernate 버전에 따라 다를 수 있으니, 네이티브 쿼리로 전환하거나 두 개의 메서드로 분리 후 애플리케이션 레벨에서 합치는 것도 고려해보세요.
대안: 메서드 분리
`@Query`(""" SELECT bh FROM BudgetHistory bh JOIN bh.adCampaign ac WHERE ac.organization.id = :orgId AND bh.createdAt BETWEEN :start AND :end """) List<BudgetHistory> findCampaignHistoryByOrgAndPeriod( `@Param`("orgId") Long orgId, `@Param`("start") LocalDateTime start, `@Param`("end") LocalDateTime end); `@Query`(""" SELECT bh FROM BudgetHistory bh JOIN bh.adGroup ag JOIN ag.adCampaign agc WHERE agc.organization.id = :orgId AND bh.createdAt BETWEEN :start AND :end """) List<BudgetHistory> findAdGroupHistoryByOrgAndPeriod( `@Param`("orgId") Long orgId, `@Param`("start") LocalDateTime start, `@Param`("end") LocalDateTime end); // Service에서: // List<BudgetHistory> all = new ArrayList<>(); // all.addAll(repository.findCampaignHistoryByOrgAndPeriod(...)); // all.addAll(repository.findAdGroupHistoryByOrgAndPeriod(...)); // all.sort(Comparator.comparing(BudgetHistory::getCreatedAt).reversed());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/repository/BudgetHistoryRepository.java` around lines 13 - 25, The current repository method findByOrgAndPeriod in BudgetHistoryRepository uses an OR on ac.organization.id and agc.organization.id which hurts index usage; replace it by either (A) two repository methods—findCampaignHistoryByOrgAndPeriod (JOIN bh.adCampaign ac WHERE ac.organization.id = :orgId AND bh.createdAt BETWEEN :start AND :end) and findAdGroupHistoryByOrgAndPeriod (JOIN bh.adGroup ag JOIN ag.adCampaign agc WHERE agc.organization.id = :orgId AND bh.createdAt BETWEEN :start AND :end)—and merge+sort results by BudgetHistory::getCreatedAt desc in the service, or (B) use a native SQL UNION ALL query that selects both sets and orders by created_at desc if you prefer a single DB call; implement one of these in place of findByOrgAndPeriod to avoid the OR condition.Source: Coding guidelines
src/main/java/com/whereyouad/WhereYouAd/domains/dashboard/application/mapper/DashboardConverter.java (1)
56-70:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick win🚨 NPE 위험: 캠페인과 광고그룹이 모두 null일 경우 처리 필요
Line 58-60에서
adCampaign이 null이면adGroup.getName()을 호출하는데, 둘 다 null인 경우 NPE가 발생합니다.
BudgetHistory엔티티 주석에는 "2개 중에 1개만 연결 가능"이라고 되어 있지만, 데이터베이스 제약조건이나 엔티티 검증으로 "둘 중 최소 하나는 반드시 non-null"임을 보장하지 않으면 런타임에 크래시될 수 있어요.해결 방안:
- (권장)
BudgetHistory엔티티에@PrePersist검증 추가하여 둘 중 하나는 반드시 존재하도록 보장- 또는 이 메서드에서 방어 코드 추가
🛡️ 방어 코드 예시
public static List<DashboardResponse.BudgetHistoryItem> toBudgetHistoryItems(List<BudgetHistory> histories) { return histories.stream().map(bh -> { - String targetName = bh.getAdCampaign() != null - ? bh.getAdCampaign().getName() - : bh.getAdGroup().getName(); + String targetName; + if (bh.getAdCampaign() != null) { + targetName = bh.getAdCampaign().getName(); + } else if (bh.getAdGroup() != null) { + targetName = bh.getAdGroup().getName(); + } else { + throw new IllegalStateException("BudgetHistory must have either adCampaign or adGroup"); + } return new DashboardResponse.BudgetHistoryItem(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/whereyouad/WhereYouAd/domains/dashboard/application/mapper/DashboardConverter.java` around lines 56 - 70, The toBudgetHistoryItems method can NPE when both adCampaign and adGroup are null; add a defensive check in DashboardConverter.toBudgetHistoryItems to handle that case (e.g., if bh.getAdCampaign()==null && bh.getAdGroup()==null) and either throw a clear IllegalStateException including the BudgetHistory identifier (or set a safe default like "UNKNOWN_TARGET") instead of calling getName() on a null, and additionally add entity-level validation in BudgetHistory (e.g., a `@PrePersist/`@PreUpdate method) to assert that at least one of adCampaign or adGroup is non-null to prevent invalid persisted state.src/main/java/com/whereyouad/WhereYouAd/domains/dashboard/domain/service/DashboardServiceImpl.java (2)
414-431:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win📅 날짜 유효성 검증 누락 - 일관성 문제
같은 클래스의 다른 메서드들(
getRoasRanking,getOngoingAdCountByProvider)에서는 날짜 검증을 수행하는데, 이 메서드에는 빠져있어요:
- 미래 날짜 체크:
startDate또는endDate가 미래인 경우 (Lines 194-196, 283-285 참고)- 날짜 순서 체크:
startDate.isAfter(endDate)검증 (Lines 198-200, 287-289 참고)이런 검증이 없으면 사용자가 잘못된 날짜를 입력해도 에러 없이 빈 결과를 반환하게 되어 UX가 나빠지고 디버깅도 어려워집니다.
✅ 검증 로직 추가 제안
public DashboardResponse.BudgetHistoryListResponse getBudgetHistory( Long userId, Long orgId, LocalDate startDate, LocalDate endDate) { + // 날짜 유효성 검사 + if (startDate.isAfter(LocalDate.now()) || endDate.isAfter(LocalDate.now())) { + throw new DashboardException(DashboardErrorCode.INVALID_DATE_RANGE); + } + if (startDate.isAfter(endDate)) { + throw new DashboardException(DashboardErrorCode.INVALID_DATE_RANGE); + } + orgRepository.findById(orgId) .orElseThrow(() -> new DashboardException(OrgErrorCode.ORG_NOT_FOUND));📝 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 DashboardResponse.BudgetHistoryListResponse getBudgetHistory( Long userId, Long orgId, LocalDate startDate, LocalDate endDate) { // 날짜 유효성 검사 if (startDate.isAfter(LocalDate.now()) || endDate.isAfter(LocalDate.now())) { throw new DashboardException(DashboardErrorCode.INVALID_DATE_RANGE); } if (startDate.isAfter(endDate)) { throw new DashboardException(DashboardErrorCode.INVALID_DATE_RANGE); } orgRepository.findById(orgId) .orElseThrow(() -> new DashboardException(OrgErrorCode.ORG_NOT_FOUND)); orgMemberRepository.findByUserIdAndOrgId(userId, orgId) .orElseThrow(() -> new DashboardException(DashboardErrorCode.ACCESS_FORBIDDEN)); List<BudgetHistory> histories = budgetHistoryRepository.findByOrgAndPeriod( orgId, startDate.atStartOfDay(), endDate.plusDays(1).atStartOfDay() ); List<DashboardResponse.BudgetHistoryItem> items = DashboardConverter.toBudgetHistoryItems(histories); return new DashboardResponse.BudgetHistoryListResponse(startDate, endDate, items); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/whereyouad/WhereYouAd/domains/dashboard/domain/service/DashboardServiceImpl.java` around lines 414 - 431, Add the same date validation used in getRoasRanking and getOngoingAdCountByProvider to getBudgetHistory: after org/orgMember checks, validate that neither startDate nor endDate is in the future (compare to LocalDate.now()) and that startDate.isAfter(endDate) is not true; when a check fails, throw a DashboardException with the same error codes used by the other methods (use the same DashboardErrorCode constants for future-date and invalid-range errors) so the method consistently rejects invalid date inputs before querying budgetHistoryRepository.Source: Coding guidelines
422-426:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # BudgetHistoryRepository에서 fetch join 사용 여부 확인 rg -A 10 "findByOrgAndPeriod" --type javaRepository: WhereYouAd/WhereYouAd-Backend
Length of output: 9933
🏁 Script executed:
#!/bin/bash set -euo pipefail # 1) BudgetHistoryRepository 쿼리/JPQL/SQL 존재 여부 확인 fd -a "BudgetHistoryRepository.java" src/main/java | head -n 20 for f in $(fd -a "BudgetHistoryRepository.java" src/main/java); do echo "===== $f =====" cat -n "$f" | sed -n '1,220p' done # 2) DashboardConverter 내 toBudgetHistoryItems()가 어떤 필드를 접근하는지 확인 fd -a "DashboardConverter.java" src/main/java | head -n 20 for f in $(fd -a "DashboardConverter.java" src/main/java); do echo "===== $f =====" rg -n "toBudgetHistoryItems" "$f" || true # 메서드 주변만 출력(너무 길면 범위 축소) rg -n "toBudgetHistoryItems" "$f" -n --context 5 || true done # 3) BudgetHistory 엔티티의 adCampaign/adGroup 매핑 및 FetchType 확인 fd -a "BudgetHistory.java" src/main/java | head -n 20 for f in $(fd -a "BudgetHistory.java" src/main/java); do echo "===== $f =====" cat -n "$f" | sed -n '1,260p' done # 4) DashboardServiceImpl getBudgetHistory() date validation 존재 여부 확인 fd -a "DashboardServiceImpl.java" src/main/java | head -n 20 for f in $(fd -a "DashboardServiceImpl.java" src/main/java); do echo "===== $f =====" # getBudgetHistory 메서드 범위만 대략 스캔 rg -n "getBudgetHistory" "$f" rg -n "getBudgetHistory" "$f" --context 60 done # 5) getRoasRanking 쪽에서 어떤 validation을 하는지(비교 기준) 확인 for f in $(fd -a "DashboardServiceImpl.java" src/main/java); do echo "===== $f (getRoasRanking 주변) =====" rg -n "getRoasRanking" "$f" rg -n "getRoasRanking" "$f" --context 60 doneRepository: WhereYouAd/WhereYouAd-Backend
Length of output: 15389
⚠️ getBudgetHistory기간 검증 누락 +BudgetHistory연관 로딩에 따른 N+1 위험
DashboardServiceImpl#getBudgetHistory(라인 414-431)는getRoasRanking처럼 미래 날짜/기간 역전(startDate > endDate) 검증이 없어 비정상 조회 범위가 그대로 들어갈 수 있습니다.BudgetHistoryRepository#findByOrgAndPeriod(라인 13-25)는LEFT JOIN만 있고JOIN FETCH/EntityGraph가 아니라서,DashboardConverter.toBudgetHistoryItems()에서bh.getAdCampaign().getName()/bh.getAdGroup().getName()접근 시FetchType.LAZY초기화로 **히스토리 개수만큼 추가 쿼리(N+1)**가 발생할 수 있습니다(라인 56-61). →JOIN FETCH또는 fetch graph 적용을 권장합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/whereyouad/WhereYouAd/domains/dashboard/domain/service/DashboardServiceImpl.java` around lines 422 - 426, In DashboardServiceImpl#getBudgetHistory add the same date validation used in getRoasRanking to reject or swap requests where startDate is after endDate (preventing future/invalid ranges) and return/throw accordingly; then update BudgetHistoryRepository#findByOrgAndPeriod to eagerly fetch related entities (either convert the JPQL/Criteria to use JOIN FETCH for adCampaign and adGroup or add an `@EntityGraph` including those associations) so that DashboardConverter.toBudgetHistoryItems accessing bh.getAdCampaign().getName() and bh.getAdGroup().getName() does not trigger N+1 lazy loads.src/main/java/com/whereyouad/WhereYouAd/domains/timeline/application/dto/response/TimelineResponse.java (1)
48-55: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
BudgetHistoryItem 레코드가 DashboardResponse에도 동일하게 정의되어 있어요.
현재
TimelineResponse.BudgetHistoryItem과DashboardResponse.BudgetHistoryItem이 완전히 동일한 구조로 중복 정의되어 있습니다. 두 도메인에서 같은 예산 변경 이력 정보를 사용하고 있으니, 공통 DTO로 추출하는 것을 권장해요.예시:
// 공통 패키지로 추출 package com.whereyouad.WhereYouAd.domains.advertisement.application.dto.response; public record BudgetHistoryDTO( BudgetFieldType fieldType, String targetName, Long previousValue, Long newValue, LocalDateTime changedAt, Provider provider ) {}그 후
DashboardResponse와TimelineResponse에서 이 공통 DTO를 재사용하면 유지보수가 훨씬 편해집니다. 필드 구조가 바뀔 때 한 곳만 수정하면 되거든요!🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/whereyouad/WhereYouAd/domains/timeline/application/dto/response/TimelineResponse.java` around lines 48 - 55, Extract the duplicated record into a shared DTO (e.g., create BudgetHistoryDTO record with fields BudgetFieldType fieldType, String targetName, Long previousValue, Long newValue, LocalDateTime changedAt, Provider provider) and replace TimelineResponse.BudgetHistoryItem and DashboardResponse.BudgetHistoryItem usages with this shared BudgetHistoryDTO, updating imports and any constructor/factory code that referenced the old nested records so both TimelineResponse and DashboardResponse reuse the common DTO.Source: Coding guidelines
src/main/java/com/whereyouad/WhereYouAd/domains/timeline/application/mapper/TimelineConverter.java (1)
78-92:
⚠️ Potential issue | 🟠 Major | ⚡ Quick wintoBudgetHistoryItems 메서드가 DashboardConverter와 중복되고, NPE 위험이 있어요.
두 가지 개선점을 발견했습니다:
코드 중복:
DashboardConverter.toBudgetHistoryItems(L56-69)와 완전히 동일한 로직이에요. 두 컨버터가 같은 변환 로직을 사용하고 있으니, 공통 유틸리티 메서드로 추출하는 것을 권장합니다.NPE 위험: 80-82번 줄에서
adCampaign이 null이면adGroup.getName()을 호출하는데, 만약 둘 다 null이라면 NPE가 발생합니다. DB 제약조건으로 최소 하나는 항상 존재한다고 보장되나요? 그렇지 않다면 방어 코드를 추가해야 해요.제안:
public static List<TimelineResponse.BudgetHistoryItem> toBudgetHistoryItems(List<BudgetHistory> histories) { return histories.stream().map(bh -> { - String targetName = bh.getAdCampaign() != null - ? bh.getAdCampaign().getName() - : bh.getAdGroup().getName(); + String targetName = bh.getAdCampaign() != null + ? bh.getAdCampaign().getName() + : (bh.getAdGroup() != null ? bh.getAdGroup().getName() : "Unknown"); return new TimelineResponse.BudgetHistoryItem(더 좋은 방법은
BudgetHistory엔티티 레벨에서getTargetName()메서드를 추가하거나, 공통 컨버터를 만드는 거예요!🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/whereyouad/WhereYouAd/domains/timeline/application/mapper/TimelineConverter.java` around lines 78 - 92, Refactor the duplicated budget-history mapping by extracting the common logic used in TimelineConverter.toBudgetHistoryItems and DashboardConverter.toBudgetHistoryItems into a shared utility (or add a helper method on the BudgetHistory entity like getTargetName()); update both converters to call that helper. Also harden against NPEs by ensuring the helper (or the mapping) checks both bh.getAdCampaign() and bh.getAdGroup() and returns a safe fallback (e.g., empty string or "unknown") if both are null before constructing TimelineResponse.BudgetHistoryItem, leaving other fields unchanged.Source: Coding guidelines
src/main/java/com/whereyouad/WhereYouAd/domains/timeline/domain/service/TimelineServiceImpl.java (1)
260-266:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # BudgetHistoryRepository.findByOrgAndPeriod 메서드가 join fetch를 사용하는지 확인 rg -nP -A10 'findByOrgAndPeriod' --type=java -g '*BudgetHistoryRepository*'Repository: WhereYouAd/WhereYouAd-Backend
Length of output: 839
🏁 Script executed:
#!/bin/bash set -euo pipefail # 1) BudgetHistoryRepository 메서드 선언/어노테이션 확인 repo_file="src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/repository/BudgetHistoryRepository.java" if [ -f "$repo_file" ]; then echo "---- $repo_file ----" nl -ba "$repo_file" | sed -n '1,140p' else echo "Missing: $repo_file" >&2 fi # 2) BudgetHistory 엔티티의 관계 매핑(fetch type) 확인 # 파일명으로 찾기 echo "---- BudgetHistory entity candidates ----" fd -a "BudgetHistory*.java" src/main/java | head -n 20 bh_file="$(fd -a "BudgetHistory.java" src/main/java | head -n 1 || true)" if [ -n "${bh_file:-}" ] && [ -f "$bh_file" ]; then echo "---- $bh_file (mapping relevant part) ----" nl -ba "$bh_file" | sed -n '1,260p' else echo "BudgetHistory.java not found (or multiple). Trying to locate mapping via rg..." rg -n "class BudgetHistory|adCampaign|adGroup" src/main/java -S -g '*BudgetHistory*.java' fi # 3) TimelineConverter.toBudgetHistoryItems 구현 확인 echo "---- TimelineConverter.toBudgetHistoryItems ----" converter_files="$(fd -a "TimelineConverter*.java" src/main/java | head -n 20)" echo "$converter_files" for f in $converter_files; do if rg -q "toBudgetHistoryItems" "$f"; then echo "---- $f ----" nl -ba "$f" | rg -n "toBudgetHistoryItems" -n --context 20 || true nl -ba "$f" | sed -n '1,220p' fi done # 4) TimelineServiceImpl 호출부 주변(260-266 확인) service_file="src/main/java/com/whereyouad/WhereYouAd/domains/timeline/domain/service/TimelineServiceImpl.java" if [ -f "$service_file" ]; then echo "---- $service_file (260-290) ----" nl -ba "$service_file" | sed -n '230,310p' else echo "Missing: $service_file" >&2 fiRepository: WhereYouAd/WhereYouAd-Backend
Length of output: 258
🏁 Script executed:
#!/bin/bash set -euo pipefail # 5) 메서드가 derived query인지, `@EntityGraph/`@Query가 없는지 빠르게 추가 확인 rg -n "findByOrgAndPeriod\s*\(" src/main/java -S rg -n "`@Query`|`@EntityGraph`" src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/repository/BudgetHistoryRepository.java -SRepository: WhereYouAd/WhereYouAd-Backend
Length of output: 624
🏁 Script executed:
#!/bin/bash set -euo pipefail print_with_numbers() { # usage: print_with_numbers file start end local f="$1" start="$2" end="$3" awk -v s="$start" -v e="$end" '{ if(NR>=s && NR<=e) printf "%6d\t%s\n", NR, $0 }' "$f" } repo_file="src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/repository/BudgetHistoryRepository.java" if [ -f "$repo_file" ]; then echo "---- $repo_file (1-140) ----" print_with_numbers "$repo_file" 1 140 else echo "Missing: $repo_file" >&2 fi # locate BudgetHistory entity echo "---- Locate BudgetHistory entity file ----" bh_files="$(fd -a "BudgetHistory*.java" src/main/java | head -n 20)" echo "$bh_files" bh_file="$(fd -a "BudgetHistory.java" src/main/java | head -n 5 | head -n 1 || true)" if [ -n "${bh_file:-}" ] && [ -f "$bh_file" ]; then echo "---- $bh_file (1-260) ----" print_with_numbers "$bh_file" 1 260 echo "---- $bh_file (mapping-focused search) ----" rg -n "adCampaign|adGroup|`@ManyToOne`|fetch\s*=\s*FetchType\.LAZY|FetchType" "$bh_file" || true else echo "BudgetHistory.java not found; fallback search for entity mappings." rg -n "class\s+BudgetHistory|`@Entity`\s+.*BudgetHistory|BudgetHistory\s*$" src/main/java -S || true fi # locate TimelineConverter and inspect toBudgetHistoryItems echo "---- Locate TimelineConverter files ----" conv_files="$(fd -a "TimelineConverter*.java" src/main/java | head -n 50)" echo "$conv_files" for f in $conv_files; do if rg -q "toBudgetHistoryItems" "$f"; then echo "---- $f (toBudgetHistoryItems region) ----" # print around method body rg -n "toBudgetHistoryItems" "$f" # naive range print; adjust if needed print_with_numbers "$f" 1 260 rg -n "getAdCampaign|getAdGroup|adCampaign|adGroup" "$f" || true fi done # show TimelineServiceImpl around 250-290 service_file="src/main/java/com/whereyouad/WhereYouAd/domains/timeline/domain/service/TimelineServiceImpl.java" if [ -f "$service_file" ]; then echo "---- $service_file (240-295) ----" print_with_numbers "$service_file" 240 295 fiRepository: WhereYouAd/WhereYouAd-Backend
Length of output: 12695
예산 변경 이력 조회에서 N+1 가능성(major)
TimelineServiceImpl(260-266)에서
findByOrgAndPeriod로BudgetHistory를 가져온 뒤,TimelineConverter.toBudgetHistoryItems가bh.getAdCampaign().getName()/bh.getAdGroup().getName()을 호출합니다. 그런데BudgetHistory의adCampaign과adGroup은 둘 다FetchType.LAZY이고,BudgetHistoryRepository의 JPQL은LEFT JOIN만 있고JOIN FETCH가 없습니다. → 결과적으로 항목 수만큼 추가 SELECT가 터질 수 있어요.해결:
findByOrgAndPeriod쪽을LEFT JOIN FETCH bh.adCampaign/LEFT JOIN FETCH bh.adGroup(필요 시LEFT JOIN FETCH ag.adCampaign)로 바꾸거나@EntityGraph로 필요한 연관을 함께 로딩하세요. (현재 WHERE는bh.organization.id가 아니라ac.organization.id또는agc.organization.id기준이라, 그 조건을 그대로 유지해야 합니다.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/whereyouad/WhereYouAd/domains/timeline/domain/service/TimelineServiceImpl.java` around lines 260 - 266, The BudgetHistory fetch causes an N+1 because TimelineServiceImpl calls budgetHistoryRepository.findByOrgAndPeriod and then TimelineConverter.toBudgetHistoryItems accesses bh.getAdCampaign().getName() and bh.getAdGroup().getName(); update BudgetHistoryRepository.findByOrgAndPeriod to eagerly load associations (e.g., add LEFT JOIN FETCH bh.adCampaign and LEFT JOIN FETCH bh.adGroup — and if adGroup references campaign add LEFT JOIN FETCH ag.adCampaign) or annotate the repository method with an `@EntityGraph` that includes adCampaign and adGroup, ensuring the original WHERE predicate that filters by organization (currently tied to ac/agc) is preserved.Source: Coding guidelines
ojy0903
requested changes
Jun 11, 2026
ojy0903
left a comment
ojy0903
left a comment
Collaborator
There was a problem hiding this comment.
P3: 고생하셨어요! 제가 보기엔 로직은 괜찮아 보이고, 메타랑 구글에서 예산 수정 로직 추가할 때 이 BudgetHistory 엔티티 관련 처리를 추가로 해주는 것만 기억해두고 작업하면 좋을 듯 합니다...!!
그런데 제가 이해한거로는 BudgetHistory 가 AdCampaign 이나 AdGroup 중 하나를 부모로 참조하는 구조인거 같은데 맞을까요?? 맞다면 AdCampaign 이나 AdGroup 이 삭제될 때 이 BudgetHistory 엔티티가 남아있을 가능성이 있을 것 같아서 OnDelete 어노테이션으로 연관된 AdCampaign 이나 AdGroup 이 삭제되면 BudgetHistory 도 함께 삭제되도록 하면 좋을 것 같습니다!
ojy0903
approved these changes
Jun 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📌 관련 이슈
🚀 개요
📄 작업 내용
📸 스크린샷 / 테스트 결과 (선택)
예산 수정 시 DB 저장
대시보드 예산 변경 이력 조회 API
타임라인 상세 조회
✅ 체크리스트
🔍 리뷰 포인트 (Review Points)
P4: 예산 변경을 필드로만 관리하면 가장 최근 값 밖에 조회하지 못하고 수정 시점을 추적하기 위해서는 따로 테이블로 분리해서 관리하는 방법밖에 없을 것 같아서 따로 테이블로 분리해서 구현해봤습니다!
P4: 기존 브랜치(#142 )에 작업하면 pr 단위가 커질 것 같아서 따로 올렸습니다!
P2: 변경 이력을 또 적용할 다른 도메인이 있을까요?
P4: 타임라인 엔티티 AI 요약에 추가하는 건 따로 이슈파서 진행하겠습니다!
Summary by CodeRabbit
릴리스 노트