chore: add more authorization directive tests#2454
Conversation
Aenimus
requested review from
JivusAyrus,
StarpTech and
thisisnithin
as code owners
WalkthroughThe authorization-directives test file was restructured and greatly expanded; two new public API constants, Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-09-08T20:57:07.946ZApplied to files:
📚 Learning: 2025-08-29T10:28:04.846ZApplied to files:
📚 Learning: 2025-08-28T09:17:49.477ZApplied to files:
🧬 Code graph analysis (1)composition/tests/v1/directives/authorization-directives.test.ts (7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2454 +/- ##
===========================================
- Coverage 60.84% 37.66% -23.19%
===========================================
Files 229 769 +540
Lines 23839 114353 +90514
Branches 0 7869 +7869
===========================================
+ Hits 14504 43066 +28562
- Misses 8089 70928 +62839
+ Partials 1246 359 -887 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@composition/tests/v1/directives/authorization-directives.test.ts`:
- Around line 841-873: Remove the duplicate test whose description is "that
`@authenticated` on an Object field generates the correct router configuration"
(the second occurrence), or change it to cover a different scenario; locate the
test using the same call to federateSubgraphsSuccess([fnaa, fnab],
ROUTER_COMPATIBILITY_VERSION_ONE) and the same assertions against
fieldConfigurations and schemaToSortedNormalizedString/normalizeString that
reference OBJECT and AUTHENTICATED_DIRECTIVE, then either delete that block or
replace it with a distinct case (e.g., test a different type kind or directive
combination) so only one test asserts this exact behavior.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
composition/tests/v1/directives/authorization-directives.test.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-08T20:57:07.946Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Applied to files:
composition/tests/v1/directives/authorization-directives.test.ts
📚 Learning: 2025-08-28T09:17:49.477Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
Applied to files:
composition/tests/v1/directives/authorization-directives.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
composition/tests/v1/directives/authorization-directives.test.ts (3)
1-13: LGTM!The new imports of
INTERFACEandOBJECTconstants are correctly added and properly used in the test assertions for the router configuration checks.
876-1000: LGTM!The
@requiresScopestests are well-structured with comprehensive coverage:
- Persistence in federated schema
- Interface field behavior with implementations
- Error handling for scope limits with both subgraph orderings
2276-2485: LGTM!The new subgraph definitions are well-organized and provide comprehensive test coverage for authorization directives across different GraphQL type kinds (Enum, Interface, Object, Scalar) and their fields. The naming convention (
fiaa/fiab,fjaa/fjab, etc.) is consistent and allows for easy pairing in federation tests.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@composition/tests/v1/directives/authorization-directives.test.ts`:
- Around line 2539-2561: Remove the unused Subgraph definitions fpaa and fpab
(the constants fpaa and fpab that call parse(...) to set definitions and names)
from the test file since they are not referenced by any test; alternatively, if
they are meant to be exercised, add explicit test cases that import/instantiate
these symbols and assert their expected behavior instead of leaving the unused
constants in the file.
♻️ Duplicate comments (1)
composition/tests/v1/directives/authorization-directives.test.ts (1)
841-873: Duplicate test case detected.This test is identical to the test at lines 807-839—same description, same subgraphs
[fnaa, fnab], and same assertions. Remove this duplicate or update it to test a different scenario.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
composition/tests/v1/directives/authorization-directives.test.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
📚 Learning: 2025-09-08T20:57:07.946Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Applied to files:
composition/tests/v1/directives/authorization-directives.test.ts
📚 Learning: 2025-08-28T09:17:49.477Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
Applied to files:
composition/tests/v1/directives/authorization-directives.test.ts
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
composition/tests/v1/directives/authorization-directives.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
composition/tests/v1/directives/authorization-directives.test.ts (4)
5-7: LGTM!The newly exported
INTERFACEandOBJECTconstants are properly imported and used in the test assertions fortypeNamefields in the fieldConfigurations.
876-1051: LGTM!The new
@requiresScopes testsdescribe block is well-organized with clear test coverage for:
- Schema persistence
- Interface field scope isolation from implementations
- Error handling for scope limits
- Inter-subgraph scope reduction
2328-2537: LGTM!The new subgraph definitions are well-structured and provide comprehensive test coverage for
@authenticateddirective across different GraphQL type kinds (Enum, Interface, Object, Scalar) and their fields.
2563-2591: LGTM!The inter-subgraph scope test fixtures (
fqaa,fqab,fqac) are correctly designed to verify scope reduction logic:
fqaaandfqabwith identical scopes test merge behaviorfqacwith an additional scope tests intersection (requiredScopes) vs. union (requiredScopesByOR) semantics
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
2 participants
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist