Repository navigation
[Fix] 노션 url 분석 시 selenium timeout으로 분석 실패 케이스 개선 - #142
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthrough자료 추출기가 HTTP 상태, HTML 본문, 렌더링 결과의 인증 보호 신호를 감지합니다. 인증이 필요하면 Changes자료 원문 인증 보호 처리
컨테이너 초기화 설정
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant ExternalHtmlDocumentClient
participant RenderedHtmlDocumentClient
participant ProtectedSourceDetector
Client->>ExternalHtmlDocumentClient: HTML 원문 요청
ExternalHtmlDocumentClient->>ProtectedSourceDetector: 응답 상태와 HTML 보호 여부 확인
ProtectedSourceDetector-->>ExternalHtmlDocumentClient: 공개 원문 또는 인증 필요 오류
Client->>RenderedHtmlDocumentClient: 렌더링 원문 요청
RenderedHtmlDocumentClient->>ProtectedSourceDetector: 최종 URL과 렌더링 HTML 확인
ProtectedSourceDetector-->>RenderedHtmlDocumentClient: 공개 원문 또는 인증 필요 오류
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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
🧹 Nitpick comments (7)
src/main/java/com/teaching/backend/domain/material/service/extract/GenericWebMaterialContentExtractor.java (1)
89-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win보호 문서 검증 로직이
AbstractHtmlMaterialContentExtractor와 중복됩니다.
AbstractHtmlMaterialContentExtractor.validateDocument(Line 119-126)는 동일한 검사를 수행합니다. 보안 판정 로직이 두 곳에 존재하면 한쪽만 수정되어 판정 기준이 달라질 수 있습니다.
ProtectedSourceDetector에validateOrThrow(HtmlDocument)같은 공용 메서드를 추가하고 두 곳에서 호출하도록 정리하세요.🤖 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/teaching/backend/domain/material/service/extract/GenericWebMaterialContentExtractor.java` around lines 89 - 93, 중복된 보호 문서 판정 로직을 제거하세요. ProtectedSourceDetector에 HtmlDocument를 받아 보호 문서인 경우 MATERIAL_SOURCE_AUTH_REQUIRED를 발생시키는 공용 validateOrThrow 메서드를 추가하고, GenericWebMaterialContentExtractor.validateDocument와 AbstractHtmlMaterialContentExtractor.validateDocument가 해당 메서드를 호출하도록 변경하세요.src/main/java/com/teaching/backend/domain/material/service/extract/NotionMaterialContentExtractor.java (1)
33-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value이 문구 검사는 이제 중복입니다.
ProtectedSourceDetector.hasNotionPrivateSignal은 대소문자를 구분하지 않고 "this page is private"와 "you do not have access"를 이미 검사합니다.super.validateDocument가 먼저 실행되므로 이 블록은 도달하지 않습니다. 중복 판정 기준을 한 곳으로 모으려면 이 블록을 제거하는 방안을 검토하세요.🤖 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/teaching/backend/domain/material/service/extract/NotionMaterialContentExtractor.java` around lines 33 - 35, Remove the duplicate private-page message check in NotionMaterialContentExtractor, including its local MaterialErrorCode exception, and rely on ProtectedSourceDetector.hasNotionPrivateSignal through the existing super.validateDocument flow for case-insensitive detection.src/main/java/com/teaching/backend/domain/material/service/extract/RenderedHtmlDocumentClient.java (3)
194-199: 🩺 Stability & Availability | 🔵 TrivialNotion 렌더링의 최대 소요 시간을 확인하세요.
Notion URL은 페이지 로드 타임아웃 30초에 최대 2회 시도를 수행합니다. 여기에
waitUntilReadable의 대기 시간과NOTION_SCROLL_STEPS(8) ×NOTION_SCROLL_WAIT_MS(250ms) 스크롤 시간이 더해집니다. 최악의 경우 하나의 요청이 60초 이상 렌더 퍼밋을 점유합니다.max-concurrency기본값이 2이므로, 동시 요청 2건이 장기 점유하면 나머지 요청은acquire-timeout-ms(500ms) 후 즉시 폴백 실패로 처리됩니다.전체 렌더 예산 상한을 두거나, Nginx·게이트웨이 타임아웃과 함께 값을 정하세요. PR 설명의 Nginx 타임아웃 변경 계획과 순서를 맞추는 것을 권장합니다.
🤖 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/teaching/backend/domain/material/service/extract/RenderedHtmlDocumentClient.java` around lines 194 - 199, Update the Notion rendering flow around renderAttempt and its retry settings to enforce a bounded total render-time budget, including page-load, waitUntilReadable, and scrolling delays across all attempts. Size the budget consistently with the planned Nginx or gateway timeout so long-running renders do not monopolize render permits and trigger immediate fallback failures for queued requests.
76-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNotion 타임아웃 기본값이 세 곳에 하드코딩되어 있습니다.
30000과10000이 세 생성자 위임 경로에 반복됩니다. 값 하나를 변경하면 나머지 두 곳이 누락될 수 있습니다.private static final long DEFAULT_NOTION_PAGE_LOAD_TIMEOUT_MS = 30_000L;와DEFAULT_NOTION_SCRIPT_TIMEOUT_MS = 10_000L;상수를 선언하고 참조하세요.@Value기본값과의 일치 여부도 함께 유지할 수 있습니다.Also applies to: 93-94, 143-144
🤖 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/teaching/backend/domain/material/service/extract/RenderedHtmlDocumentClient.java` at line 76, RenderedHtmlDocumentClient의 생성자 위임 경로에 반복된 Notion 페이지 로드 및 스크립트 타임아웃 리터럴을 제거하세요. 클래스에 DEFAULT_NOTION_PAGE_LOAD_TIMEOUT_MS와 DEFAULT_NOTION_SCRIPT_TIMEOUT_MS 상수를 선언하고 해당 생성자 호출부(76, 93-94, 143-144)에서 참조하며, `@Value` 기본값과 동일한 값을 유지하세요.
200-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win타임아웃 예외 처리를 하나의 블록으로 통합하세요.
ScriptTimeoutException과TimeoutException은WebDriverException의 서로 다른 하위 클래스입니다. 따라서ScriptTimeoutException은 후속catch (RuntimeException e)에서 처리됩니다.두
catch블록의 재시도·로깅·실패 처리는 중복됩니다.MaterialException처리 이후의catch (RuntimeException e)하나로 통합하세요. 현재 구조에서는isRetryableTimeout의TimeoutException검사만 도달할 수 없으며, 통합 후에는 두 예외 검사를 유지해야 합니다. 동일한 정리를 320-322행에도 적용하세요.🤖 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/teaching/backend/domain/material/service/extract/RenderedHtmlDocumentClient.java` around lines 200 - 239, Remove the dedicated TimeoutException catch block in the rendered HTML fallback flow and consolidate its retry, logging, and terminal-failure handling into the RuntimeException catch after MaterialException. Update isRetryableTimeout to recognize both TimeoutException and ScriptTimeoutException so both timeout subclasses remain retryable; apply the same consolidation at the corresponding handling around the second occurrence.src/main/java/com/teaching/backend/domain/material/service/extract/AbstractHtmlMaterialContentExtractor.java (1)
119-126: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
MATERIAL_SOURCE_AUTH_REQUIRED가 플랫폼 폴백 경로에서 가려질 수 있습니다.
validateDocument는 이제MaterialException을 던집니다.tryPlatformFallback은MaterialException을 잡아Optional.empty()로 변환합니다. 따라서CafeMaterialContentExtractor.parsePlatformFallback이 iframe 문서에 대해 인증 필요 예외를 던지면, 그 예외는 소실되고 렌더링 폴백을 거쳐 최종적으로MATERIAL_CONTENT_EMPTY가 반환됩니다. 사용자는 "로그인 필요" 대신 "본문 비어 있음" 오류를 받습니다.인증 필요 오류는 폴백 대상에서 제외하는 방안을 검토하세요.
♻️ 제안 리팩터
private Optional<ParsedHtmlContent> tryPlatformFallback( String originalUrl, HtmlDocument initialDocument ) { try { return parsePlatformFallback(originalUrl, initialDocument); } catch (MaterialException e) { if (e.getErrorCode() == MaterialErrorCode.MATERIAL_SOURCE_AUTH_REQUIRED) { throw e; } // 기존 로깅 유지 return Optional.empty(); } }🤖 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/teaching/backend/domain/material/service/extract/AbstractHtmlMaterialContentExtractor.java` around lines 119 - 126, Update tryPlatformFallback to rethrow MaterialException when its error code is MATERIAL_SOURCE_AUTH_REQUIRED, while preserving the existing Optional.empty() conversion and logging for other MaterialException values. This ensures parsePlatformFallback authentication failures remain visible instead of reaching the rendering fallback as MATERIAL_CONTENT_EMPTY.src/main/java/com/teaching/backend/domain/material/service/extract/ProtectedSourceDetector.java (1)
159-162: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
hasNotionPrivateSignal은 모든 플랫폼 문서에 적용됩니다.메서드 이름은 Notion 전용을 뜻하지만
isProtectedHtml은 URL 조건 없이 이 검사를 실행합니다. 본문 어디에든 "this page is private" 또는 "you do not have access" 문자열이 있으면 문서 전체가 거부됩니다. 예를 들어 접근 제어를 설명하는 기술 문서가 이 문구를 인용하면 오탐이 발생합니다.문구 검사를 Notion 호스트로 한정하거나, 제목·헤딩 요소로 범위를 좁히는 방법을 검토하세요.
🤖 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/teaching/backend/domain/material/service/extract/ProtectedSourceDetector.java` around lines 159 - 162, Update the protected-document detection flow around hasNotionPrivateSignal and isProtectedHtml so the Notion private-page phrases are evaluated only for Notion hosts or within title/heading elements, rather than arbitrary document body text. Preserve the existing protection behavior for genuine Notion private-page responses while preventing quoted phrases in unrelated platform content from rejecting the document.
🤖 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/teaching/backend/domain/material/service/extract/CafeMaterialContentExtractor.java`:
- Around line 36-37: Update validateDocument in
src/main/java/com/teaching/backend/domain/material/service/extract/CafeMaterialContentExtractor.java
at lines 36-37 and
src/main/java/com/teaching/backend/domain/material/service/extract/NotionMaterialContentExtractor.java
at lines 28-29: after each super.validateDocument(document) call, return
immediately when document is null before accessing document.body().
In
`@src/main/java/com/teaching/backend/domain/material/service/extract/ExternalHtmlDocumentClient.java`:
- Around line 108-113: Update the authentication-status handling in
ExternalHtmlDocumentClient so only responses that can be confidently identified
as authentication-required produce MATERIAL_SOURCE_AUTH_REQUIRED; do not
classify every 403 response this way, allowing bot, regional, and WAF blocks to
follow the generic failure path. Ensure the 401/403 handling also invokes
logExtractionFailure with at least the request host and status code before
returning or propagating the error.
In
`@src/main/java/com/teaching/backend/domain/material/service/extract/ProtectedSourceDetector.java`:
- Around line 13-19: Remove "github.com" from the AUTH_HOSTS list in
ProtectedSourceDetector, leaving the other authentication-only hosts unchanged.
Preserve GitHub login and signin detection through the existing
hasAuthPathSegment logic.
In
`@src/test/java/com/teaching/backend/domain/material/service/extract/RenderedHtmlDocumentClientTest.java`:
- Around line 531-543: Update the TestRenderedHtmlDocumentClient constructor to
delegate to the superclass overload that accepts the provided permits Semaphore,
rather than the overload that creates a new semaphore from maxConcurrency.
Ensure the permits argument is passed through so the related strategy tests
validate the semaphore they construct.
---
Nitpick comments:
In
`@src/main/java/com/teaching/backend/domain/material/service/extract/AbstractHtmlMaterialContentExtractor.java`:
- Around line 119-126: Update tryPlatformFallback to rethrow MaterialException
when its error code is MATERIAL_SOURCE_AUTH_REQUIRED, while preserving the
existing Optional.empty() conversion and logging for other MaterialException
values. This ensures parsePlatformFallback authentication failures remain
visible instead of reaching the rendering fallback as MATERIAL_CONTENT_EMPTY.
In
`@src/main/java/com/teaching/backend/domain/material/service/extract/GenericWebMaterialContentExtractor.java`:
- Around line 89-93: 중복된 보호 문서 판정 로직을 제거하세요. ProtectedSourceDetector에
HtmlDocument를 받아 보호 문서인 경우 MATERIAL_SOURCE_AUTH_REQUIRED를 발생시키는 공용
validateOrThrow 메서드를 추가하고, GenericWebMaterialContentExtractor.validateDocument와
AbstractHtmlMaterialContentExtractor.validateDocument가 해당 메서드를 호출하도록 변경하세요.
In
`@src/main/java/com/teaching/backend/domain/material/service/extract/NotionMaterialContentExtractor.java`:
- Around line 33-35: Remove the duplicate private-page message check in
NotionMaterialContentExtractor, including its local MaterialErrorCode exception,
and rely on ProtectedSourceDetector.hasNotionPrivateSignal through the existing
super.validateDocument flow for case-insensitive detection.
In
`@src/main/java/com/teaching/backend/domain/material/service/extract/ProtectedSourceDetector.java`:
- Around line 159-162: Update the protected-document detection flow around
hasNotionPrivateSignal and isProtectedHtml so the Notion private-page phrases
are evaluated only for Notion hosts or within title/heading elements, rather
than arbitrary document body text. Preserve the existing protection behavior for
genuine Notion private-page responses while preventing quoted phrases in
unrelated platform content from rejecting the document.
In
`@src/main/java/com/teaching/backend/domain/material/service/extract/RenderedHtmlDocumentClient.java`:
- Around line 194-199: Update the Notion rendering flow around renderAttempt and
its retry settings to enforce a bounded total render-time budget, including
page-load, waitUntilReadable, and scrolling delays across all attempts. Size the
budget consistently with the planned Nginx or gateway timeout so long-running
renders do not monopolize render permits and trigger immediate fallback failures
for queued requests.
- Line 76: RenderedHtmlDocumentClient의 생성자 위임 경로에 반복된 Notion 페이지 로드 및 스크립트 타임아웃
리터럴을 제거하세요. 클래스에 DEFAULT_NOTION_PAGE_LOAD_TIMEOUT_MS와
DEFAULT_NOTION_SCRIPT_TIMEOUT_MS 상수를 선언하고 해당 생성자 호출부(76, 93-94, 143-144)에서 참조하며,
`@Value` 기본값과 동일한 값을 유지하세요.
- Around line 200-239: Remove the dedicated TimeoutException catch block in the
rendered HTML fallback flow and consolidate its retry, logging, and
terminal-failure handling into the RuntimeException catch after
MaterialException. Update isRetryableTimeout to recognize both TimeoutException
and ScriptTimeoutException so both timeout subclasses remain retryable; apply
the same consolidation at the corresponding handling around the second
occurrence.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 274c9064-cc9a-4839-a97c-2a5353aa0033
📒 Files selected for processing (13)
docker-compose.ymlsrc/main/java/com/teaching/backend/domain/material/exception/MaterialErrorCode.javasrc/main/java/com/teaching/backend/domain/material/service/extract/AbstractHtmlMaterialContentExtractor.javasrc/main/java/com/teaching/backend/domain/material/service/extract/CafeMaterialContentExtractor.javasrc/main/java/com/teaching/backend/domain/material/service/extract/ExternalHtmlDocumentClient.javasrc/main/java/com/teaching/backend/domain/material/service/extract/GenericWebMaterialContentExtractor.javasrc/main/java/com/teaching/backend/domain/material/service/extract/NotionMaterialContentExtractor.javasrc/main/java/com/teaching/backend/domain/material/service/extract/ProtectedSourceDetector.javasrc/main/java/com/teaching/backend/domain/material/service/extract/RenderedHtmlDocumentClient.javasrc/test/java/com/teaching/backend/domain/material/service/extract/ExternalHtmlDocumentClientTest.javasrc/test/java/com/teaching/backend/domain/material/service/extract/HtmlMaterialContentExtractorTest.javasrc/test/java/com/teaching/backend/domain/material/service/extract/ProtectedSourceDetectorTest.javasrc/test/java/com/teaching/backend/domain/material/service/extract/RenderedHtmlDocumentClientTest.java
| if (isAuthenticationRequiredStatus(statusCode)) { | ||
| return response.releaseBody() | ||
| .then(Mono.error(new MaterialException( | ||
| MaterialErrorCode.MATERIAL_SOURCE_AUTH_REQUIRED | ||
| ))); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
403 응답을 모두 인증 필요로 분류하면 오분류가 발생합니다.
403은 인증 요구 외에도 봇 차단, 지역 차단, WAF 규칙에서 자주 사용됩니다. 이 경로는 이제 그러한 응답에도 "로그인 또는 회원가입이 필요한 페이지"라는 메시지를 반환합니다.
또한 이 분기는 logExtractionFailure를 호출하지 않습니다. 401·403 실패는 로그에 남지 않으므로 원인 분석이 어려워집니다. 최소한 호스트와 상태 코드를 기록하세요.
🤖 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/teaching/backend/domain/material/service/extract/ExternalHtmlDocumentClient.java`
around lines 108 - 113, Update the authentication-status handling in
ExternalHtmlDocumentClient so only responses that can be confidently identified
as authentication-required produce MATERIAL_SOURCE_AUTH_REQUIRED; do not
classify every 403 response this way, allowing bot, regional, and WAF blocks to
follow the generic failure path. Ensure the 401/403 handling also invokes
logExtractionFailure with at least the request host and status code before
returning or propagating the error.
m00nsh
changed the title
m00nsh
changed the title
✨ 어떤 이유로 PR를 하셨나요?
📋 세부 내용 - 왜 해당 PR이 필요한지 작업 내용을 자세하게 설명해주세요
변경 사항
1. Notion Selenium 크롤링 안정화
PageLoadStrategy.EAGER적용TimeoutException,ScriptTimeoutException발생 시 새 ChromeDriver 세션으로 1회 retrydriver.quit()수행2. Selenium child process zombie 누적 방지
docker-compose.yml의 application service에init: true적용3. 로그인/권한이 필요한 외부 페이지 감지
MATERIAL_SOURCE_AUTH_REQUIRED (MATERIAL4032)오류 추가4. 오탐 방지
login,OAuth,회원가입등의 단어가 있다는 이유만으로 차단하지 않음테스트
.\gradlew.bat compileTestJava.\gradlew.bat test.\gradlew.bat --no-daemon clean build -x testgit diff --check모두 통과
로컬 Swagger Notion 회귀 테스트
기존 테스트용 공개 Notion URL 4개를 각각 반복 분석하여 정상 분석 확인
기존에 반복적으로 발생하던
MATERIAL5022/ Selenium page load timeout 재현되지 않음참고
HTTP 200으로 서비스 공통 landing/shell 페이지를 반환하는 일부 인증 우회 케이스는
사이트별 응답 형태가 달라 일반적인 로그인 필요 감지로 완전히 판별하기 어려울 수 있습니다.
이번 PR에서는 명확한 인증/권한 신호를 안전하게 구분하는 범위까지만 적용했습니다.
Nginx request timeout 조정은 애플리케이션 코드와 별개의 서버 설정이므로
배포 후 별도 적용 예정입니다.
📸 작업 화면 스크린샷
🚨 관련 이슈 번호 [ ]
Summary by CodeRabbit