feat(planning): add data-rights contributor v1 - #179
Conversation
|
Warning Review limit reached
Next review available in: 21 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPlanning 서비스에 데이터 권리 contributor를 추가했다. Planning 및 Today 데이터를 내보내고 삭제한다. 삭제 영수증과 멱등성을 PostgreSQL에 저장한다. 삭제 완료 검증과 단위·통합 테스트를 추가했다. ChangesPlanning 데이터 권리 처리
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PlanningRuntime
participant PlanningDataRightsContributor
participant PostgreSQL
Client->>PlanningRuntime: 데이터 권리 요청
PlanningRuntime->>PlanningDataRightsContributor: handle(request)
PlanningDataRightsContributor->>PostgreSQL: export 또는 erase 실행
PostgreSQL-->>PlanningDataRightsContributor: 데이터, 삭제 수 또는 영수증
PlanningDataRightsContributor-->>PlanningRuntime: 작업별 응답
PlanningRuntime-->>Client: 데이터 권리 결과
Possibly related PRs
🚥 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
apps/planning-service/src/planning-data-rights.ts (3)
288-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win동적 테이블 식별자 대신 정적 문장을 사용하십시오.
countDeleted는ERASURE_TABLES허용 목록으로 검증하므로 현재 주입 위험은 없습니다. 다만 코딩 가이드라인은 SQL 구조를 정적으로 유지하도록 요구합니다. 테이블별로 완전한 정적DELETE문장을 상수 맵으로 정의하고, 워크스페이스 값만 바인딩하십시오. 그러면 문자열 보간이 완전히 사라집니다.가이드라인 근거: "Keep SQL structure static and parameterize dynamic values."
🤖 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 `@apps/planning-service/src/planning-data-rights.ts` around lines 288 - 306, Update countDeleted to eliminate dynamic table interpolation by introducing a static SQL DELETE statement map keyed by the validated ERASURE_TABLES values. Select the appropriate statement after allowlist validation, keep workspaceId as the only bound parameter, and preserve the existing deletion result counting and validation behavior.Source: Coding guidelines
441-455: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win사전 점검 쿼리의 다섯
EXISTS결과가 사용되지 않습니다.다섯 개의
EXISTS식에는 별칭이 없고, 반환 결과도 읽지 않습니다. PostgreSQL은 이 열들에 모두exists라는 같은 이름을 부여합니다. 따라서 값이 서로 덮여 쓰이고 판단에 쓰이지 않습니다.ready는 영수증 테이블 존재 여부만 반영합니다.데이터 존재 여부를 blocker에 반영하지 않을 계획이면 이 서브쿼리를 제거하십시오. 반영할 계획이면 각 식에 별칭을 부여하고
blockers에 포함하십시오.♻️ 불필요한 서브쿼리 제거 예시
const result = await this.client.query<{ erasure_receipts_ready: unknown }>( `SELECT - EXISTS (SELECT 1 FROM planning.goals WHERE workspace_id = $1), - EXISTS (SELECT 1 FROM planning.projects WHERE workspace_id = $1), - EXISTS (SELECT 1 FROM planning.tasks WHERE workspace_id = $1), - EXISTS (SELECT 1 FROM planning.today_aggregates WHERE workspace_id = $1), - EXISTS ( - SELECT 1 FROM planning.today_idempotency_records WHERE workspace_id = $1 - ), - to_regclass('planning.data_rights_erasure_receipts') IS NOT NULL + to_regclass('planning.data_rights_erasure_receipts') IS NOT NULL AS erasure_receipts_ready`, [workspaceId], );참고:
$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 `@apps/planning-service/src/planning-data-rights.ts` around lines 441 - 455, Remove the five unused workspace data EXISTS subqueries from the preflight query in the erasure-receipts readiness flow, since only erasure_receipts_ready is consumed. Remove the now-unneeded workspaceId parameter from the query values array as well, while preserving the to_regclass check and erasureReceiptsReady assignment.
474-478: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win지원 PostgreSQL 버전을 문서화하십시오. PostgreSQL 17.10에는
hashtextextended(text, bigint)가 존재합니다. 현재 호출에서$1은text로 추론되고0은bigint로 변환되므로 타입 오류는 없습니다. 다만 이 함수는 공식 SQL 문서에 공개 함수로 문서화되지 않은 내부 함수입니다. 최소 지원 버전과 업그레이드 호환성 검사를 운영 계약에 명시하십시오.🤖 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 `@apps/planning-service/src/planning-data-rights.ts` around lines 474 - 478, planning-data-rights의 advisory-lock 쿼리와 관련된 운영 계약에 최소 지원 PostgreSQL 버전을 명시하고, hashtextextended 의존성이 있는 업그레이드 호환성 검사를 추가하십시오. PostgreSQL 17.10에서의 text, bigint 시그니처 지원을 기준으로 문서화하되, 해당 내부 함수가 업그레이드 후에도 사용 가능한지 확인하는 절차를 포함하십시오.apps/planning-service/migrations/0004_data_rights_erasure_receipts.sql (1)
1-19: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value영수증 테이블에
COMMENT ON과 보존 정책을 문서화하십시오.이 테이블은
requested_by_user_id를 무기한 보존합니다. 데이터 권리 삭제 이후에도 사용자 식별자가 남습니다. 보존 기간과 근거를 명시하십시오.COMMENT ON TABLE과 주요 열에COMMENT ON COLUMN을 추가하면 계약 의도가 스키마에 남습니다.🤖 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 `@apps/planning-service/migrations/0004_data_rights_erasure_receipts.sql` around lines 1 - 19, Document the retention contract for data_rights_erasure_receipts by adding COMMENT ON TABLE and COMMENT ON COLUMN statements, especially for requested_by_user_id and created_at. State the retention period and legal or operational basis, and clarify how long user identifiers remain after erasure; cover the key receipt, request, identity, timestamp, and digest columns without changing the table definition.apps/planning-service/tests/postgres-data-rights.integration.test.ts (1)
38-48: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win유휴 확인과
DROP DATABASE사이에 경합이 남습니다. 데이터베이스 이름도 중복 정의됩니다.두 가지를 지적합니다.
- 카운트 조회와
DROP DATABASE사이에 새 연결이 생기면 삭제가database is being accessed by other users로 실패합니다. PostgreSQL 13 이상에서는WITH (FORCE)가 이 경합을 없앱니다.DROP DATABASE IF EXISTS life_os_data_rights_test는 이름을 문자열로 직접 씁니다. 조회는TEMPORARY_DATABASE_NAME을 사용합니다. 상수를 바꾸면 두 문장이 어긋납니다.DROP DATABASE는 파라미터를 받지 않으므로, 상수를 검증한 뒤 보간하십시오.♻️ 제안
if (activeConnections.rows[0]?.count === 0) { - await adminPool.query('DROP DATABASE IF EXISTS life_os_data_rights_test'); + await adminPool.query( + `DROP DATABASE IF EXISTS "${TEMPORARY_DATABASE_NAME}" WITH (FORCE)`, + ); return; }
TEMPORARY_DATABASE_NAME은 이 파일의 리터럴 상수입니다. 외부 입력이 아니므로 주입 위험은 없습니다. 대상 PostgreSQL 버전이 13 미만이면WITH (FORCE)대신pg_terminate_backend를 사용하십시오.🤖 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 `@apps/planning-service/tests/postgres-data-rights.integration.test.ts` around lines 38 - 48, Update the database cleanup loop around TEMPORARY_DATABASE_NAME to use PostgreSQL 13+ DROP DATABASE ... WITH (FORCE), eliminating the race between the activity count and drop. Remove the hardcoded database name, validate TEMPORARY_DATABASE_NAME as the known literal before safely interpolating it into the DROP DATABASE statement, and preserve the existing return behavior after successful cleanup.
🤖 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 `@apps/planning-service/src/planning-data-rights.test.ts`:
- Around line 60-72: Update the test around createPlanningRuntime and
dataRightsContributor to invoke handle with a valid export or erase_preflight
request, then assert the contractually expected response rather than only
checking field presence and function type. Keep runtime.close cleanup and use
the inert pool setup so the test verifies contributor wiring and the SQL-client
execution path through the real runtime.
In `@apps/planning-service/src/planning-data-rights.ts`:
- Around line 379-423: Update the export flow that builds the normalized data
object so large workspaces are handled without passing more than
MAXIMUM_ARRAY_ITEMS to normalizeJson at once. Add pagination or chunked
retrieval for each relevant table query, aggregate all chunks into the export
while preserving recordCount and existing validation, or expose and enforce an
explicit per-table limit with a clear follow-up path instead of silently
producing a partial export.
- Around line 21-29: planning-data-rights.ts의 DataRightsJsonPrimitive,
DataRightsJsonArray, DataRightsJsonObject, DataRightsJsonValue,
DataRightsContributorRequest, DataRightsContributorResponse 및 공개 메서드 handle에 계약을
설명하는 docstring을 추가하십시오. 각 연산의 의미와 실패 조건, requestId가 요청에서 응답·오류로 전달되는 규칙을 새 기여자가
구현을 확인하지 않아도 이해할 수 있도록 문서화하고, 타입 정의와 handle의 동작은 변경하지 마십시오.
In `@apps/planning-service/src/planning-runtime.ts`:
- Line 203: PlanningRuntime의 공개 선언인 dataRightsContributor에 TSDoc을 추가하세요. 문서에는 이
contributor가 Planning 소유 데이터 권리를 담당하며 요청 처리 범위에서 사용되고 공유 SQL client를 함께 사용한다는
계약을 설명하고, 새 기여자가 구현을 추적하지 않고도 이해할 수 있도록 작성하세요.
In `@apps/planning-service/tests/postgres-data-rights.integration.test.ts`:
- Around line 264-279: Remove the cleanupError throw from the finally block in
the surrounding test cleanup flow. Preserve cleanup failures in a flag or
variable, then throw the AggregateError after the try/finally completes, while
retaining the existing primaryFailure cause handling and cleanup behavior.
---
Nitpick comments:
In `@apps/planning-service/migrations/0004_data_rights_erasure_receipts.sql`:
- Around line 1-19: Document the retention contract for
data_rights_erasure_receipts by adding COMMENT ON TABLE and COMMENT ON COLUMN
statements, especially for requested_by_user_id and created_at. State the
retention period and legal or operational basis, and clarify how long user
identifiers remain after erasure; cover the key receipt, request, identity,
timestamp, and digest columns without changing the table definition.
In `@apps/planning-service/src/planning-data-rights.ts`:
- Around line 288-306: Update countDeleted to eliminate dynamic table
interpolation by introducing a static SQL DELETE statement map keyed by the
validated ERASURE_TABLES values. Select the appropriate statement after
allowlist validation, keep workspaceId as the only bound parameter, and preserve
the existing deletion result counting and validation behavior.
- Around line 441-455: Remove the five unused workspace data EXISTS subqueries
from the preflight query in the erasure-receipts readiness flow, since only
erasure_receipts_ready is consumed. Remove the now-unneeded workspaceId
parameter from the query values array as well, while preserving the to_regclass
check and erasureReceiptsReady assignment.
- Around line 474-478: planning-data-rights의 advisory-lock 쿼리와 관련된 운영 계약에 최소 지원
PostgreSQL 버전을 명시하고, hashtextextended 의존성이 있는 업그레이드 호환성 검사를 추가하십시오. PostgreSQL
17.10에서의 text, bigint 시그니처 지원을 기준으로 문서화하되, 해당 내부 함수가 업그레이드 후에도 사용 가능한지 확인하는 절차를
포함하십시오.
In `@apps/planning-service/tests/postgres-data-rights.integration.test.ts`:
- Around line 38-48: Update the database cleanup loop around
TEMPORARY_DATABASE_NAME to use PostgreSQL 13+ DROP DATABASE ... WITH (FORCE),
eliminating the race between the activity count and drop. Remove the hardcoded
database name, validate TEMPORARY_DATABASE_NAME as the known literal before
safely interpolating it into the DROP DATABASE statement, and preserve the
existing return behavior after successful cleanup.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e7c43201-6394-40f2-9bde-cc8d3ac46741
📒 Files selected for processing (6)
apps/planning-service/migrations/0004_data_rights_erasure_receipts.sqlapps/planning-service/src/planning-controller-authority.test.tsapps/planning-service/src/planning-data-rights.test.tsapps/planning-service/src/planning-data-rights.tsapps/planning-service/src/planning-runtime.tsapps/planning-service/tests/postgres-data-rights.integration.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/planning-service/src/planning-data-rights.test.ts`:
- Around line 148-156: Update the export assertions in the planning data-rights
test to validate response.data.goals directly: assert its length equals
LARGE_EXPORT_GOAL_COUNT, and verify the first and last goal IDs match the
expected ordered export results. Keep the existing operation, recordCount, hash,
and query-count assertions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a50a45b-e83d-404f-a7b8-1b36001d74cc
📒 Files selected for processing (4)
apps/planning-service/src/planning-data-rights.test.tsapps/planning-service/src/planning-data-rights.tsapps/planning-service/src/planning-runtime.tsapps/planning-service/tests/postgres-data-rights.integration.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/planning-service/src/planning-runtime.ts
- apps/planning-service/src/planning-data-rights.ts
2 participants
Buyer/privacy outcome
Advance #55 with the first concrete owning-service contributor: Planning must expose the versioned data-rights lifecycle from its own runtime without cross-service database access.
Test-first state
This Draft intentionally starts with a RED runtime-composition contract. The test requires
PlanningRuntimeto expose one service-owned data-rights contributor; protected main currently has no such runtime boundary. After exact-head CI proves that failure, the branch will add the smallest production composition, then add separate RED behavioral tests for deterministic tenant-scoped export, fail-closed preflight, replay-safe erasure and post-erasure verification before implementing those behaviors.Scope
Planning only. The slice consumes the protected
life-os.data-rights-contributor.v1contract from #159 and will not let Identity read or mutate Planning tables directly. Other contributors, artifact delivery and whole-right orchestration remain separate #55 work.Refs #55 and #159.
Summary by CodeRabbit
새로운 기능
버그 수정