Feat/#131 Google Ads 캠페인 조회 수정 및 예산 수정 API 구현#166
Merged
Conversation
- 기존 환경 변수로 설정해두었던 계정만 접근 가능하였던 방식에서 변경
WalkthroughGoogle Ads 캠페인 예산 수정 PATCH API가 추가됐고, 요청/응답 DTO와 서비스/클라이언트 연동이 확장됐습니다. 동기화는 하위 클라이언트 계정 기준으로 바뀌었고, 인증 헤더는 연동 계정 정보에서 동적으로 구성되도록 변경됐습니다. Changes구글 예산 수정 및 하위 계정 동기화
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GoogleAdController
participant GoogleBudgetService
participant GoogleAdWebClient
participant GoogleAdsAPI
Client->>GoogleAdController: PATCH /api/google/campaigns/{adCampaignId}/budget
GoogleAdController->>GoogleBudgetService: updateCampaignBudget(userId, adCampaignId, request)
GoogleBudgetService->>GoogleAdWebClient: getCampaignBudgetResourceName(...)
GoogleAdWebClient->>GoogleAdsAPI: googleAds:search
GoogleAdsAPI-->>GoogleAdWebClient: campaignBudget resourceName
GoogleBudgetService->>GoogleAdWebClient: mutateCampaignBudget(...)
GoogleAdWebClient->>GoogleAdsAPI: campaignBudgets:mutate
GoogleAdsAPI-->>GoogleAdWebClient: mutate result
GoogleBudgetService-->>GoogleAdController: BudgetUpdateResponse
GoogleAdController-->>Client: DataResponse(BudgetUpdateResponse)
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/adapi/google/GoogleAdService.java (1)
53-98: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift두 동기화 메서드 모두
@Transactional안에서 계정 수만큼 블로킹 호출을 반복해요.
createAllAdInfos,syncAllGoogleAdsData둘 다 클라이언트 계정 개수 × 4회(캠페인/그룹/소재/통계)의.block()HTTP 호출을 트랜잭션 안에서 순차적으로 수행합니다. MCC 계정 하위에 클라이언트가 많을수록 DB 커넥션을 오래 붙잡게 되고,syncAllGoogleAdsData는 스케줄러로 주기 실행되는 것으로 보이는데(이미지에 로그 타임스탬프가 반복 노출) 이 상태면 커넥션 풀 고갈 위험이 커집니다.Based on learnings, 이런 패턴은 이전 리뷰에서도 지적된 적이 있습니다 — "to avoid holding a DB connection during synchronous outbound HTTP calls... explicitly annotate that method with
Transactional(propagation = Propagation.NOT_SUPPORTED)" 같은 접근을 참고해서, 외부 호출과 DB 반영 트랜잭션을 분리하는 구조를 검토해보시면 좋겠습니다.🤖 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/adapi/google/GoogleAdService.java` around lines 53 - 98, Both GoogleAdService sync entrypoints are holding a transaction open while performing many blocking outbound calls. Update createAllAdInfos and syncAllGoogleAdsData so the fetchAccessibleClientAccounts / syncAdCampaigns / syncAdGroups / syncAdContents / syncMetricFacts flow does not run inside an active `@Transactional` boundary; use a non-transactional propagation or split the external API calls from the DB-write transaction. Keep the transaction only around the persistence step in GoogleAdService, and preserve the existing exception handling/logging around each clientAccountId.Source: Learnings
♻️ Duplicate comments (1)
src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/adapi/google/GoogleAdService.java (1)
100-117: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
GoogleBudgetService.fetchAccessibleClientAccountsLocal과 중복되는 로직이에요.동일한 파싱/필터링 로직이 두 곳에 존재하고, 실패 시 fallback 값도 서로 다릅니다(여기는 빈 리스트,
GoogleBudgetService는List.of(customerId)). 공통 유틸리티나 도메인 서비스로 추출해서 한 곳에서 관리하시는 걸 추천드려요. (자세한 내용은GoogleBudgetService.java143-162번째 줄 코멘트 참고)🤖 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/adapi/google/GoogleAdService.java` around lines 100 - 117, `GoogleAdService.fetchAccessibleClientAccounts` contains the same accessible-client parsing/filtering logic as `GoogleBudgetService.fetchAccessibleClientAccountsLocal`, but the fallback behavior differs and will drift further. Extract the shared Google Ads client-account parsing into a common helper or domain service used by both methods, and make the fallback strategy consistent there (for example, centralize whether to return an empty list or `List.of(customerId)`). Keep the filtering and ID mapping logic in one place so both `GoogleAdService` and `GoogleBudgetService` call the same implementation.
🧹 Nitpick comments (2)
src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/application/dto/request/AdvertisementRequest.java (1)
51-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
MetaBudgetUpdateRequest와 100% 동일한 구조 - 공통 인터페이스로 추출을 고려해보세요.
GoogleBudgetUpdateRequest(51-73줄)가 바로 위MetaBudgetUpdateRequest(28-49줄)와 필드명, 검증 로직(isExactlyOne), 변환 로직(amount,budgetType)이 완전히 동일합니다. 나중에 검증 메시지나 로직이 바뀔 때 두 곳을 동시에 수정해야 하는 부담이 생겨요.Java 17의 record는 인터페이스를 구현할 수 있고 default 메서드로 공통 로직을 제공할 수 있으니, 아래처럼 공통 인터페이스로 뽑아내면 중복을 없앨 수 있습니다.
♻️ 제안: 공통 인터페이스 추출
public interface BudgetUpdateFields { Long dailyBudget(); Long lifetimeBudget(); default boolean isExactlyOne() { return (dailyBudget() == null) ^ (lifetimeBudget() == null); } default Long amount() { return lifetimeBudget() != null ? lifetimeBudget() : dailyBudget(); } default BudgetType budgetType() { return lifetimeBudget() != null ? BudgetType.TOTAL : BudgetType.DAILY; } }public record GoogleBudgetUpdateRequest( `@Positive`(message = "일일 예산은 0보다 커야 합니다.") Long dailyBudget, `@Positive`(message = "총 예산은 0보다 커야 합니다.") Long lifetimeBudget - ) { + ) implements BudgetUpdateFields { `@JsonIgnore` `@AssertTrue`(message = "일일 예산과 총 예산 중 정확히 하나만 입력해야 합니다.") public boolean isExactlyOne() { return (dailyBudget == null) ^ (lifetimeBudget == null); } - - `@JsonIgnore` - public Long amount() { - return lifetimeBudget != null ? lifetimeBudget : dailyBudget; - } - - `@JsonIgnore` - public BudgetType budgetType() { - return lifetimeBudget != null ? BudgetType.TOTAL : BudgetType.DAILY; - } }(단,
@AssertTrue/@JsonIgnore어노테이션은 record 내부에 재선언이 필요할 수 있으니 실제 적용 전 컴파일 확인 권장)당장 기능에 문제는 없어서 급하진 않지만, 플랫폼이 하나 더 늘어나면 중복이 세 배가 될 수 있으니 여유 있을 때 반영해보시는 걸 추천드려요.
🤖 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/application/dto/request/AdvertisementRequest.java` around lines 51 - 73, GoogleBudgetUpdateRequest duplicates MetaBudgetUpdateRequest in its fields and helper logic, so extract the shared budget contract into a common interface and have both records implement it. Move the reusable validation/conversion behavior for isExactlyOne, amount, and budgetType into the interface as default methods, while keeping any record-specific annotations like `@AssertTrue` and `@JsonIgnore` on each record as needed. Use the existing GoogleBudgetUpdateRequest and MetaBudgetUpdateRequest record names to align their shared dailyBudget and lifetimeBudget structure without duplicating logic.Source: Path instructions
src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/adapi/google/GoogleAdService.java (1)
61-72: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift하위 계정 순회로 인해 기존 N+1 조회 패턴이 계정 수만큼 곱해져요.
syncAdCampaigns/syncAdGroups/syncAdContents(하단 119~233번째 줄)는 결과 아이템마다findByPlatformAccountAndExternalCampaignId등 개별 쿼리를 날리는 구조인데, 이번 변경으로 이 로직이 계정당 1회가 아니라clientAccountIds개수만큼 반복 호출됩니다. MCC 계정에 하위 클라이언트가 많으면 쿼리 수가 크게 늘어날 수 있어요. 배치 조회(예: 캠페인 ID 목록으로IN쿼리 후 메모리에서 매칭)로 리팩터링하는 걸 검토해보시면 좋겠습니다.As per path instructions, "JPA 사용 시 N+1 문제나 불필요한 쿼리가 발생하지 않는지... 체크하라."
🤖 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/adapi/google/GoogleAdService.java` around lines 61 - 72, `GoogleAdService`의 하위 계정 순회가 `syncAdCampaigns`/`syncAdGroups`/`syncAdContents`/`syncMetricFacts`를 `clientAccountIds`마다 반복 호출해 기존 N+1 조회가 계정 수만큼 더 커지고 있습니다. 각 동기화 메서드에서 `findByPlatformAccountAndExternalCampaignId` 같은 개별 조회를 유지하지 말고, 먼저 외부 ID 목록을 모아 한 번에 조회한 뒤 메모리에서 매칭하는 배치 방식으로 리팩터링하세요. 특히 `fetchAccessibleClientAccounts` 이후 루프 구조와 각 `sync*` 메서드 내부의 repository 조회를 함께 점검해 불필요한 쿼리가 반복되지 않도록 수정하세요.Source: Path instructions
🤖 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/adapi/google/GoogleBudgetService.java`:
- Around line 143-162: The account-fetching logic in
GoogleBudgetService.fetchAccessibleClientAccountsLocal is duplicated from
GoogleAdService.fetchAccessibleClientAccounts, but their fallback behavior
differs and can drift over time. Extract the shared parsing/filtering/id-mapping
flow into a common helper or GoogleAdWebClient-based utility, and make both
callers use it so the fallback for null/blank/exception cases is centralized and
consistent. Use the existing fetchAccessibleClientAccountsLocal method and
GoogleAdService.fetchAccessibleClientAccounts as the main reference points when
refactoring.
- Around line 65-94: `updateCampaignBudget`가 `@Transactional` 안에서
`fetchAccessibleClientAccountsLocal`와
`getCampaignBudgetResourceName(...).block()` 같은 동기 외부 호출을 하위 계정 수만큼 반복하고 있습니다.
`GoogleBudgetService.updateCampaignBudget`에서 구글 API 조회/변경 로직은 트랜잭션 밖으로 분리하고,
`mutateCampaignBudget` 이후 필요한 DB 반영만 별도의 짧은 트랜잭션으로 처리하도록 구조를 바꾸세요. 필요하면 외부 호출 전용
메서드에 `Transactional(propagation = Propagation.NOT_SUPPORTED)`를 적용해
`@Transactional` 경계에서 블로킹 HTTP 호출이 수행되지 않게 하세요.
- Around line 83-86: The catch block in GoogleBudgetService is swallowing every
exception while iterating sub-accounts, so the failure reason is lost. Update
the exception handling in the sub-account search flow to log the caught
Exception with at least debug or warn level, including enough context to
identify the account/campaign lookup that failed. Keep the fallback behavior of
continuing to the next account, but do not leave the catch block empty.
- Around line 93-116: Separate the Google Ads side effect from local persistence
in GoogleBudgetService.updateBudget flow: if mutateCampaignBudget(...).block()
succeeds but campaign.updateBudget(...), campaign.applyBudgetType(...), or
budgetHistoryRepository.save(...) fails, handle the inconsistency with
compensation/retry/alerting instead of treating it as a normal rollback. Also
stop overwriting the campaign budget type via
campaign.applyBudgetType(requestType), since Google Ads CampaignBudget.type is
not mutable; keep the existing budget type unchanged in this path and only
update the amount/history fields.
---
Outside diff comments:
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/adapi/google/GoogleAdService.java`:
- Around line 53-98: Both GoogleAdService sync entrypoints are holding a
transaction open while performing many blocking outbound calls. Update
createAllAdInfos and syncAllGoogleAdsData so the fetchAccessibleClientAccounts /
syncAdCampaigns / syncAdGroups / syncAdContents / syncMetricFacts flow does not
run inside an active `@Transactional` boundary; use a non-transactional
propagation or split the external API calls from the DB-write transaction. Keep
the transaction only around the persistence step in GoogleAdService, and
preserve the existing exception handling/logging around each clientAccountId.
---
Duplicate comments:
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/adapi/google/GoogleAdService.java`:
- Around line 100-117: `GoogleAdService.fetchAccessibleClientAccounts` contains
the same accessible-client parsing/filtering logic as
`GoogleBudgetService.fetchAccessibleClientAccountsLocal`, but the fallback
behavior differs and will drift further. Extract the shared Google Ads
client-account parsing into a common helper or domain service used by both
methods, and make the fallback strategy consistent there (for example,
centralize whether to return an empty list or `List.of(customerId)`). Keep the
filtering and ID mapping logic in one place so both `GoogleAdService` and
`GoogleBudgetService` call the same implementation.
---
Nitpick comments:
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/application/dto/request/AdvertisementRequest.java`:
- Around line 51-73: GoogleBudgetUpdateRequest duplicates
MetaBudgetUpdateRequest in its fields and helper logic, so extract the shared
budget contract into a common interface and have both records implement it. Move
the reusable validation/conversion behavior for isExactlyOne, amount, and
budgetType into the interface as default methods, while keeping any
record-specific annotations like `@AssertTrue` and `@JsonIgnore` on each record as
needed. Use the existing GoogleBudgetUpdateRequest and MetaBudgetUpdateRequest
record names to align their shared dailyBudget and lifetimeBudget structure
without duplicating logic.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/adapi/google/GoogleAdService.java`:
- Around line 61-72: `GoogleAdService`의 하위 계정 순회가
`syncAdCampaigns`/`syncAdGroups`/`syncAdContents`/`syncMetricFacts`를
`clientAccountIds`마다 반복 호출해 기존 N+1 조회가 계정 수만큼 더 커지고 있습니다. 각 동기화 메서드에서
`findByPlatformAccountAndExternalCampaignId` 같은 개별 조회를 유지하지 말고, 먼저 외부 ID 목록을 모아
한 번에 조회한 뒤 메모리에서 매칭하는 배치 방식으로 리팩터링하세요. 특히 `fetchAccessibleClientAccounts` 이후 루프
구조와 각 `sync*` 메서드 내부의 repository 조회를 함께 점검해 불필요한 쿼리가 반복되지 않도록 수정하세요.
🪄 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: 6b7967d7-af91-47c0-b57b-421881e79225
📒 Files selected for processing (11)
src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/application/dto/request/AdvertisementRequest.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/application/dto/response/GoogleAdResponse.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/adapi/google/GoogleAdService.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/adapi/google/GoogleBudgetService.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/exception/code/GoogleAdErrorCode.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/presentation/GoogleAdController.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/presentation/docs/GoogleAdDocs.javasrc/main/java/com/whereyouad/WhereYouAd/global/adapi/strategy/GoogleAdAuthStrategy.javasrc/main/java/com/whereyouad/WhereYouAd/infrastructure/client/google/GoogleAdWebClient.javasrc/main/java/com/whereyouad/WhereYouAd/infrastructure/client/google/converter/GoogleConverter.javasrc/main/java/com/whereyouad/WhereYouAd/infrastructure/client/google/dto/GoogleDTO.java
Comment on lines
+65
to
+94
| try { | ||
| // 1. 캠페인의 campaign_budget 리소스 이름 조회를 위해 해당 캠페인이 위치한 정확한 Client Account ID 찾기 | ||
| String rootCustomerId = account.getExternalAccountId(); | ||
| AdAuthRequest emptyRequest = AdAuthRequest.empty(); | ||
|
|
||
| java.util.List<String> clientAccountIds = fetchAccessibleClientAccountsLocal(rootCustomerId, connection, emptyRequest); | ||
|
|
||
| String budgetResourceName = null; | ||
| String targetCustomerId = rootCustomerId; | ||
|
|
||
| for (String clientAccountId : clientAccountIds) { | ||
| try { | ||
| String jsonResponse = googleAdWebClient.getCampaignBudgetResourceName(clientAccountId, connection, emptyRequest, campaign.getExternalCampaignId()).block(); | ||
| budgetResourceName = extractBudgetResourceName(jsonResponse); | ||
| if (budgetResourceName != null) { | ||
| targetCustomerId = clientAccountId; | ||
| break; // 캠페인을 찾았으므로 중단 | ||
| } | ||
| } catch (Exception e) { | ||
| // 해당 하위 계정에 캠페인이 없거나 권한 오류일 시 다음 계정 확인 | ||
| } | ||
| } | ||
|
|
||
| if (budgetResourceName == null) { | ||
| log.error("[Google] 예산 리소스 이름 추출 실패 - 모든 하위 계정 탐색 완료, campaignId: {}", campaign.getExternalCampaignId()); | ||
| throw new AdApiHandler(AdApiErrorCode.BUDGET_UPDATE_FAILED); | ||
| } | ||
|
|
||
| // 2. 캠페인 예산 변경(Mutate) - 찾은 targetCustomerId를 대상으로 수행 | ||
| googleAdWebClient.mutateCampaignBudget(targetCustomerId, connection, emptyRequest, budgetResourceName, amountMicros).block(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
@Transactional 안에서 하위 계정 수만큼 순차적으로 블로킹 외부 호출을 수행하고 있어요.
updateCampaignBudget이 @Transactional이면서, fetchAccessibleClientAccountsLocal 및 for문 내부의 getCampaignBudgetResourceName(...).block() 호출이 하위 클라이언트 계정 개수만큼 반복 실행됩니다. MCC 하위 계정이 많아질수록 DB 커넥션을 길게 붙잡고 구글 API 응답을 기다리게 되어, 커넥션 풀 고갈이나 타임아웃 연쇄 장애로 이어질 수 있습니다.
Based on learnings, 트랜잭션 경계 안에서 동기적인 외부 HTTP 호출을 유지하는 패턴은 이전에도 지적된 적이 있습니다 — "If you need a specific method to run with no active transaction (e.g., to avoid holding a DB connection during synchronous outbound HTTP calls such as Slack/Discord webhook dispatch), explicitly annotate that method with Transactional(propagation = Propagation.NOT_SUPPORTED)" 같은 접근처럼, 외부 API 조회/변경 로직은 트랜잭션 밖에서 먼저 수행하고, DB 반영만 짧은 트랜잭션으로 분리하는 구조를 권장드려요.
🤖 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/adapi/google/GoogleBudgetService.java`
around lines 65 - 94, `updateCampaignBudget`가 `@Transactional` 안에서
`fetchAccessibleClientAccountsLocal`와
`getCampaignBudgetResourceName(...).block()` 같은 동기 외부 호출을 하위 계정 수만큼 반복하고 있습니다.
`GoogleBudgetService.updateCampaignBudget`에서 구글 API 조회/변경 로직은 트랜잭션 밖으로 분리하고,
`mutateCampaignBudget` 이후 필요한 DB 반영만 별도의 짧은 트랜잭션으로 처리하도록 구조를 바꾸세요. 필요하면 외부 호출 전용
메서드에 `Transactional(propagation = Propagation.NOT_SUPPORTED)`를 적용해
`@Transactional` 경계에서 블로킹 HTTP 호출이 수행되지 않게 하세요.
Source: Learnings
ojy0903
approved these changes
Jul 7, 2026
ojy0903
left a comment
ojy0903
left a comment
Collaborator
There was a problem hiding this comment.
P4: 고생하셨습니다! 모두의 창업 과제도 있고 하니 차근히 하셔도 될 거 같아요..!!
- 하위계정 순회가 오래 걸리는건 제 처음 생각에는 외부 API 통신을 우선 DB 트랜잭션 바깥에서 진행하게 하고 각 하위 계정 순회를 병렬식으로 진행하는 방식을 쓰면 개선되지 않을까 싶긴했습니다... 클로드로 조사하면서 리뷰해봤을 땐 아래 사진처럼 상위 계정을 PlatformConnection 엔티티에 연동 시키고, 각각의 하위 계정을 PlatformAccount 엔티티로 만드는걸 추천한다는데 이건 코드 구조가 많이 바뀔수도 있을 것 같아서... 지민님 판단에 따라 결정하는게 나을 것 같습니다.
-
애셋 그룹은 제가 구글 마케팅 관련을 정확하게 아는게 아니여서 좀 찾아보니 AI 를 통해서 광고를 자동으로 조합해서 송출해주는 방식 같은데, 혹시라도 저희 서비스 엔티티와 연동이 애매하다면 별도 이슈로 진행해도 좋을 것 같아요..!!
-
추가로 제가 궁금한건데, GoogleBudgetUpdateResponse 에서 구글 예산 수정을 일일예산과 총예산을 구분해서 받는걸로 보이는데, 구글에서는 기존에 정해져 있던 광고 캠페인 예산 유형을 다른 유형으로 바꾸는것도 가능한걸까요?? 추가 댓글로 코드 부분에 남겨놓겠습니다..!!
ojy0903
reviewed
Jul 7, 2026
kingmingyu
reviewed
Jul 8, 2026
- 반복문이 아닌 id로 바로 조회
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/adapi/google/GoogleAdService.java (1)
56-103: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift트랜잭션 안의 blocking 외부 호출을 분리하세요
createAllAdInfos와syncAllGoogleAdsData가@Transactional인 상태에서, 각 하위 계정마다fetchAccessibleClientAccounts()와syncAdCampaigns()~syncMetricFacts()가 모두.block()으로 Google Ads API를 호출하고 바로 저장합니다. 이렇게 하면 네트워크 왕복 동안 트랜잭션이 열린 채로 유지돼 커넥션 풀 고갈이나 타임아웃으로 이어질 수 있어요. 외부 호출은 트랜잭션 밖으로 빼고, 저장만 짧은 트랜잭션으로 분리하는 구조가 안전합니다.🤖 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/adapi/google/GoogleAdService.java` around lines 56 - 103, Both GoogleAdService methods are keeping a transaction open while making blocking Google Ads API calls, so the external fetch/sync work should be moved out of the transactional scope. Refactor createAllAdInfos and syncAllGoogleAdsData so fetchAccessibleClientAccounts and the syncAdCampaigns/syncAdGroups/syncAssetGroups/syncAdContents/syncMetricFacts flow run outside `@Transactional`, then wrap only the database save/update portions in short, separate transactional methods. Keep the existing GoogleAdService entry points and delegate the persistence work to a dedicated method/service to avoid holding a connection during .block() calls.Source: Learnings
src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/adapi/google/GoogleBudgetService.java (1)
44-108: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
@Transactional내에서 2회의 블로킹 HTTP 호출이 발생해 DB 커넥션을 잡고 있습니다.
updateCampaignBudget에@Transactional이 있고, 내부에서getCampaignBudgetResourceName(...).block()(Line 79)과mutateCampaignBudget(...).block()(Line 88)이 순차 실행됩니다. Google API 응답을 기다리는 동안 DB 커넥션이 계속 점유되므로, 동시 예산 수정 요청이 몰리면 커넥션 풀 고갈로 이어질 수 있습니다.이전 리뷰에서 for문 내 반복 블로킹을 지적했고, for문이 제거되어 개선되었지만 근본 원인인 "트랜잭션 내 블로킹 외부 호출"은 잔존합니다. 외부 API 호출을 트랜잭션 밖으로 분리하고, DB 반영(
campaign.updateBudget+budgetHistoryRepository.save)만 별도의 짧은 트랜잭션으로 처리하는 구조를 권장합니다.Based on learnings, 트랜잭션 경계 안에서 동기적인 외부 HTTP 호출을 유지하는 패턴은 "If you need a specific method to run with no active transaction (e.g., to avoid holding a DB connection during synchronous outbound HTTP calls), explicitly annotate that method with
Transactional(propagation = Propagation.NOT_SUPPORTED)"로 지적된 바 있습니다.🔧 제안: 외부 호출과 DB 반영 분리
`@Transactional` public GoogleAdResponse.BudgetUpdateResponse updateCampaignBudget( Long userId, Long campaignId, AdvertisementRequest.GoogleBudgetUpdateRequest request ) { AdCampaign campaign = adCampaignRepository.findById(campaignId) .orElseThrow(() -> new AdvertisementHandler(AdvertisementErrorCode.ADCAMPAIGN_NOT_FOUND)); if (campaign.getProvider() != Provider.GOOGLE) { throw new AdApiHandler(AdApiErrorCode.INVALID_PROVIDER_VALUE); } BudgetType requestType = request.budgetType(); if (campaign.getBudgetType() != requestType) { throw new AdApiHandler(GoogleAdErrorCode.INVALID_BUDGET_TYPE); } Long previousBudget = campaign.getBudget(); if (Objects.equals(previousBudget, request.amount())) { throw new AdApiHandler(GoogleAdErrorCode.SAME_BUDGET_AMOUNT); } PlatformAccount account = campaign.getPlatformAccount(); PlatformConnection connection = resolveConnection(userId, account); Long amountMicros = request.amount() * 1_000_000L; - try { - String targetCustomerId = account.getExternalAccountId(); - AdAuthRequest emptyRequest = AdAuthRequest.empty(); - String jsonResponse = googleAdWebClient.getCampaignBudgetResourceName(targetCustomerId, connection, emptyRequest, campaign.getExternalCampaignId()).block(); - String budgetResourceName = extractBudgetResourceName(jsonResponse); - if (budgetResourceName == null) { - log.error("[Google] 예산 리소스 이름 추출 실패 - 타겟 하위 계정 검색 실패, campaignId: {}", campaign.getExternalCampaignId()); - throw new AdApiHandler(AdApiErrorCode.BUDGET_UPDATE_FAILED); - } - googleAdWebClient.mutateCampaignBudget(targetCustomerId, connection, emptyRequest, budgetResourceName, amountMicros).block(); - campaign.updateBudget(request.amount()); - budgetHistoryRepository.save(AdvertisementConverter.toCampaignBudgetHistory( - campaign, previousBudget, request.amount(), userId, Provider.GOOGLE - )); - return new GoogleAdResponse.BudgetUpdateResponse( - campaign.getId(), campaign.getExternalCampaignId(), request.amount(), requestType - ); - } catch (WebClientResponseException e) { - log.error("[Google] 캠페인 예산 변경 실패 - status: {}, body: {}", e.getStatusCode(), e.getResponseBodyAsString()); - throw new AdApiHandler(AdApiErrorCode.BUDGET_UPDATE_FAILED); - } catch (Exception e) { - log.error("[Google] 캠페인 예산 변경 중 오류 발생", e); - throw new AdApiHandler(AdApiErrorCode.BUDGET_UPDATE_FAILED); - } + // 외부 API 호출은 트랜잭션 밖에서 수행 + String budgetResourceName = fetchBudgetResourceName(account, connection, campaign.getExternalCampaignId()); + if (budgetResourceName == null) { + log.error("[Google] 예산 리소스 이름 추출 실패, campaignId: {}", campaign.getExternalCampaignId()); + throw new AdApiHandler(AdApiErrorCode.BUDGET_UPDATE_FAILED); + } + callMutateBudget(account, connection, budgetResourceName, amountMicros); + + // DB 반영만 별도 트랜잭션 + return persistBudgetUpdate(campaign, previousBudget, request.amount(), requestType, userId); +} + +@Transactional(propagation = Propagation.NOT_SUPPORTED) +private String fetchBudgetResourceName(PlatformAccount account, PlatformConnection connection, String externalCampaignId) { + String targetCustomerId = account.getExternalAccountId(); + AdAuthRequest emptyRequest = AdAuthRequest.empty(); + String jsonResponse = googleAdWebClient.getCampaignBudgetResourceName(targetCustomerId, connection, emptyRequest, externalCampaignId).block(); + return extractBudgetResourceName(jsonResponse); +} + +@Transactional(propagation = Propagation.NOT_SUPPORTED) +private void callMutateBudget(PlatformAccount account, PlatformConnection connection, String budgetResourceName, Long amountMicros) { + String targetCustomerId = account.getExternalAccountId(); + AdAuthRequest emptyRequest = AdAuthRequest.empty(); + try { + googleAdWebClient.mutateCampaignBudget(targetCustomerId, connection, emptyRequest, budgetResourceName, amountMicros).block(); + } catch (WebClientResponseException e) { + log.error("[Google] 캠페인 예산 변경 실패 - status: {}, body: {}", e.getStatusCode(), e.getResponseBodyAsString()); + throw new AdApiHandler(AdApiErrorCode.BUDGET_UPDATE_FAILED); + } catch (Exception e) { + log.error("[Google] 캠페인 예산 변경 중 오류 발생", e); + throw new AdApiHandler(AdApiErrorCode.BUDGET_UPDATE_FAILED); + } +} + +@Transactional +public GoogleAdResponse.BudgetUpdateResponse persistBudgetUpdate(AdCampaign campaign, Long previousBudget, Long newAmount, BudgetType requestType, Long userId) { + campaign.updateBudget(newAmount); + budgetHistoryRepository.save(AdvertisementConverter.toCampaignBudgetHistory( + campaign, previousBudget, newAmount, userId, Provider.GOOGLE + )); + return new GoogleAdResponse.BudgetUpdateResponse( + campaign.getId(), campaign.getExternalCampaignId(), newAmount, requestType + ); }🤖 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/adapi/google/GoogleBudgetService.java` around lines 44 - 108, The issue is that `updateCampaignBudget` keeps a database transaction open while waiting on blocking Google HTTP calls. Move the `googleAdWebClient.getCampaignBudgetResourceName(...).block()` and `mutateCampaignBudget(...).block()` work out of the active transaction, and keep only `campaign.updateBudget(...)` plus `budgetHistoryRepository.save(...)` in a short transactional section. Use `GoogleBudgetService.updateCampaignBudget` as the main split point, and consider a separate method annotated with `Propagation.NOT_SUPPORTED` for the external API call path.Source: Learnings
🧹 Nitpick comments (2)
src/main/java/com/whereyouad/WhereYouAd/infrastructure/client/google/converter/GoogleConverter.java (1)
70-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
toAssetGroup이toAdGroup과 사실상 동일 — 노드 추출부만 다릅니다두 메서드가
externalGroupId/name/status/adCampaign빌드까지 완전히 같고,result.getAdGroup()vsresult.getAssetGroup()로 노드를 꺼내는 부분만 달라요. 공통 빌더 로직을buildAdGroup(id, name, status, adCampaign)같은 private 헬퍼로 뽑아두면 상태 매핑 규칙이 바뀔 때 한 곳만 고치면 됩니다. 지금 당장 문제는 아니니 여유될 때 정리하셔도 좋아요.🤖 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/google/converter/GoogleConverter.java` around lines 70 - 79, `toAssetGroup` duplicates almost all of `toAdGroup`, differing only in which node is read from `GoogleDTO.AssetGroupResult`. Extract the shared `AdGroup.builder()` construction into a private helper such as `buildAdGroup(...)` in `GoogleConverter`, and have both `toAdGroup` and `toAssetGroup` pass their respective node values into it so the status mapping and field-setting logic stay in one place.src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/adapi/google/GoogleAdService.java (1)
64-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value두 동기화 진입점의 내부 루프가 완전히 동일합니다 — 헬퍼로 묶어두면 좋아요
createAllAdInfos(64-77)와syncAllGoogleAdsData(89-102)의 "하위 계정 순회 + 5개 sync 호출 + warn 로깅" 블록이 로그 문구만 빼면 사실상 동일합니다.syncClientAccount(clientAccount, connection, emptyRequest)같은 private 헬퍼로 추출하면, 이후 sync 메서드가 추가/변경될 때 한 곳만 고치면 되어 유지보수가 편해집니다.🤖 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/adapi/google/GoogleAdService.java` around lines 64 - 103, `createAllAdInfos` and `syncAllGoogleAdsData` contain the same client-account loop with the same five sync calls and exception handling, so extract that repeated block into a private helper such as `syncClientAccount(...)`. Move the shared `syncAdCampaigns`, `syncAdGroups`, `syncAssetGroups`, `syncAdContents`, and `syncMetricFacts` sequence plus the warn logging into the helper, and have both entry points call it so future sync changes only need to be updated in one place.
🤖 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.
Outside diff comments:
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/adapi/google/GoogleAdService.java`:
- Around line 56-103: Both GoogleAdService methods are keeping a transaction
open while making blocking Google Ads API calls, so the external fetch/sync work
should be moved out of the transactional scope. Refactor createAllAdInfos and
syncAllGoogleAdsData so fetchAccessibleClientAccounts and the
syncAdCampaigns/syncAdGroups/syncAssetGroups/syncAdContents/syncMetricFacts flow
run outside `@Transactional`, then wrap only the database save/update portions in
short, separate transactional methods. Keep the existing GoogleAdService entry
points and delegate the persistence work to a dedicated method/service to avoid
holding a connection during .block() calls.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/adapi/google/GoogleBudgetService.java`:
- Around line 44-108: The issue is that `updateCampaignBudget` keeps a database
transaction open while waiting on blocking Google HTTP calls. Move the
`googleAdWebClient.getCampaignBudgetResourceName(...).block()` and
`mutateCampaignBudget(...).block()` work out of the active transaction, and keep
only `campaign.updateBudget(...)` plus `budgetHistoryRepository.save(...)` in a
short transactional section. Use `GoogleBudgetService.updateCampaignBudget` as
the main split point, and consider a separate method annotated with
`Propagation.NOT_SUPPORTED` for the external API call path.
---
Nitpick comments:
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/adapi/google/GoogleAdService.java`:
- Around line 64-103: `createAllAdInfos` and `syncAllGoogleAdsData` contain the
same client-account loop with the same five sync calls and exception handling,
so extract that repeated block into a private helper such as
`syncClientAccount(...)`. Move the shared `syncAdCampaigns`, `syncAdGroups`,
`syncAssetGroups`, `syncAdContents`, and `syncMetricFacts` sequence plus the
warn logging into the helper, and have both entry points call it so future sync
changes only need to be updated in one place.
In
`@src/main/java/com/whereyouad/WhereYouAd/infrastructure/client/google/converter/GoogleConverter.java`:
- Around line 70-79: `toAssetGroup` duplicates almost all of `toAdGroup`,
differing only in which node is read from `GoogleDTO.AssetGroupResult`. Extract
the shared `AdGroup.builder()` construction into a private helper such as
`buildAdGroup(...)` in `GoogleConverter`, and have both `toAdGroup` and
`toAssetGroup` pass their respective node values into it so the status mapping
and field-setting logic stay in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8589d13c-c545-4b32-b5cf-f3f788565251
📒 Files selected for processing (6)
src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/adapi/google/GoogleAdService.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/adapi/google/GoogleBudgetService.javasrc/main/java/com/whereyouad/WhereYouAd/domains/platform/persistence/entity/PlatformAccount.javasrc/main/java/com/whereyouad/WhereYouAd/infrastructure/client/google/GoogleAdWebClient.javasrc/main/java/com/whereyouad/WhereYouAd/infrastructure/client/google/converter/GoogleConverter.javasrc/main/java/com/whereyouad/WhereYouAd/infrastructure/client/google/dto/GoogleDTO.java
jinnieusLab
commented
Jul 8, 2026
Collaborator
Author
There was a problem hiding this comment.
@ojy0903 @kingmingyu 리뷰해주신 사항 토대로 수정 완료하였습니다!
959e2e8 ~ 1f80f44
: Google 광고 예산 조회 시 budgetType 함께 조회하여 저장하고, 예산 수정 시 DB 상에서 임의로 예산 유형을 수정할 수 없도록 변경했습니다.
04e8fd8
: 구글에서 광고 그룹 저장 시 애셋 그룹은 저장되지 않았는데, 해당 쿼리문 GoogleAdWebClient에 추가하여 광고 그룹과 동일한 흐름대로 저장되도록 수정하였습니다.
애셋 그룹도 광고 그룹 스펙에 맞게 저장된 모습
8fddba2 ~ 1a06e92
: 준영님께서 말씀해주신대로 상위 계정뿐만 아니라 하위 계정도 함께 PlatformAccount로 저장될 수 있도록 로직 수정했습니다!
다만 이전과 다르게 다음과 같은 이유로 PlatformAccount에 parentAccountId라는 필드를 추가했는데 괜찮을까요? (nullable이라 다른 플랫폼들에는 영향 가지 않을 듯 합니다!)
- 개별 하위 계정들이 구글 API를 호출할 때, 어떤 상위 계정의 인증 토큰(PlatformConnection)을 써야하는지 찾기 위함
- 기존에는 특정 캠페인의 예산 수정 시 모든 하위 계정을 for문으로 돌며 찾아야 했으나, 이제 캠페인이 직접 하위 계정(PlatformAccount)과 연결되어 반복 없이 한 번만 조회함
하위 계정도 함께 저장된 모습
Collaborator
|
P4 : 고생하셨어요! 하위계정을 각각의 PlatformAccount 로 저장하면 구조가 많이 틀어질까 싶었는데 잘 구현해주신거 같아요..!! 말하신대로 parentAccountId 는 nullable 이면 제 생각에도 다른 플랫폼에 문제는 없을 것 같습니다. 애셋 그룹도 WebClient 수정도 추가해야 해서 별도 이슈로 하는게 나을지 생각했었는데 깔끔하게 구현해주신거 같습니다 👍 |
Collaborator
|
고생하셨습니다! 준영님이 말씀해주신 것처럼 nullable 이라서 문제 없을 것 같습니다! |
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
3 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.
📌 관련 이슈
🚀 개요
Google Ads에서의 캠페인 예산을 수정하는 API를 구현하였고, 기존에 환경 변수에 등록된 계정만 Google Ads OAuth를 통해 로그인 후 광고 정보 조회(GET api/google/ad-infos)가 가능하던 것을 수정하였습니다.
📄 작업 내용
구글 연동 계정(Customer ID) 동적 주입 구조로 개선 (6effcc8)
관리자 계정 하위의 클라이언트 계정 자동 동기화 기능 추가 (f6e691e)
구글 Ads는 사진과 같이 관리자 계정은 아래에 클라이언트 계정을 따로 둘 수 있고, 이 안에 캠페인 등을 등록하는데, 상위 계정으로만 접근하여 광고가 모두 조회되지 않는 문제가 있었음.
=> GoogleAdService의 전체 동기화 로직을 수정: 하위 클라이언트 계정 리스트 조회, 모두 순회하며 캠페인, 그룹, 소재, 통계 데이터를 수집
예산 수정
PATCH /api/google/campaigns/{adCampaignId}/budgetAPI 구현5f22680 ~ f664242
📸 스크린샷 / 테스트 결과 (선택)
✅ 체크리스트
🔍 리뷰 포인트 (Review Points)
구글 예산 수정 구현을 잊고 있었다가(...) 하위 계정 예산이 수정되지 않는 문제를 해결하느라 구현에 시간이 좀 걸렸네요.. 죄송합니다!
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes