fix: print client schema with select directives #2232
Conversation
Aenimus
requested review from
JivusAyrus,
StarpTech,
thisisnithin and
wilsonrivera
as code owners
WalkthroughComment-only update to include Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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: 0
🧹 Nitpick comments (1)
composition/tests/v1/directives/authorization-directives.test.ts (1)
529-529: Minor: Comment capitalization inconsistency.The header was changed from "Federation Tests" to "Federation tests" - consider keeping consistent capitalization with other test group headers in the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
composition/tests/v1/directives/authorization-directives.test.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
composition/tests/v1/directives/authorization-directives.test.ts (4)
composition/tests/utils/utils.ts (3)
federateSubgraphsSuccess(59-72)schemaToSortedNormalizedString(27-29)normalizeString(19-21)composition/src/router-compatibility-version/router-compatibility-version.ts (1)
ROUTER_COMPATIBILITY_VERSION_ONE(3-3)composition/src/utils/string-constants.ts (1)
QUERY(115-115)composition/tests/v1/utils/utils.ts (1)
schemaQueryDefinition(94-97)
⏰ 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). (4)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: build_push_image
- GitHub Check: build_test
🔇 Additional comments (6)
composition/tests/v1/directives/authorization-directives.test.ts (6)
8-8: LGTM: Consistent usage of exported constant.The import of the
QUERYconstant aligns with the goal of using centralized string constants instead of hardcoded values.
531-534: LGTM: Enhanced test structure with client schema validation.The destructuring now includes
federatedGraphClientSchema, which aligns with the PR objective of testing client schema generation with directives. This provides comprehensive validation of both server and client schema outputs.
535-544: LGTM: Usage of QUERY constant in field configurations.Replacing the hardcoded
'Query'string with theQUERYconstant improves maintainability and consistency across the codebase.
563-578: LGTM: Client schema assertion validates directive exclusion.The new assertion for
federatedGraphClientSchemacorrectly validates that the client schema excludes authorization directives (@authenticated) while preserving the core type structure. This aligns with the expected behavior where client schemas should not include server-side authorization directives.
582-585: LGTM: Consistent pattern for client schema validation.The second test follows the same pattern as the first, properly destructuring
federatedGraphClientSchemaand validating that client schemas exclude@requiresScopesdirectives while maintaining type definitions. This comprehensive testing approach ensures both server and client schemas are correctly generated.Also applies to: 614-629
531-578: Client schema should include only persisted directives — current tests are correct.Confirmed: composition/src/schema-building/utils.ts filters persisted client directives (only @deprecated, @OneOf, @semanticNonNull); one-of tests expect @OneOf in the client schema and the authorization test correctly omits @authenticated. No change required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
controlplane/test/contracts.test.ts (3)
25-25: LGTM on adding a schema header; consider centralizing for reuseThe header constant stabilizes expectations. Consider moving this to a shared test helper (e.g., test utils) so other tests can reuse the same header and reduce drift.
25-25: Make SDL comparisons resilient and DRYDirect string equality on multi-line SDLs is brittle. Normalize both sides once via a small helper and reuse it across assertions.
Apply this pattern:
@@ -const schemaDefinition = `schema {\n query: Query\n}\n\n` +const schemaDefinition = `schema {\n query: Query\n}\n\n` +const expectClientSchema = (actual: string | undefined, body: string) => { + expect(normalizeString(actual ?? '')).toBe(normalizeString(schemaDefinition + body)); +};Example usages (apply similarly to the other assertions listed in this comment’s line ranges):
@@ - expect(sdlResponse.clientSchema).toEqual(schemaDefinition + `type Query { - hello: String! -}`); + expectClientSchema(sdlResponse.clientSchema, `type Query { + hello: String! +}`); @@ - expect(sdlResponse2.clientSchema).toEqual(schemaDefinition + `type Query {\n hello: String!\n hi: String!\n}`); + expectClientSchema(sdlResponse2.clientSchema, `type Query {\n hello: String!\n hi: String!\n}`);Note: also prefer multi-line template literals over embedded “\n” for readability where possible.
Also applies to: 1041-1044, 1056-1056, 1096-1096, 1109-1109, 1159-1163, 1174-1177, 1292-1296, 1308-1311, 1352-1356, 1369-1372, 1432-1435, 1449-1450
1736-1740: Client schema header in router config: good; add directive coverage to match PR goalThe client SDL now includes the schema header—nice. Given the PR intent (“print client schema with select directives”), add 1–2 targeted assertions that those directives are present in the printed client schema (where applicable) to lock in behavior.
For example, after the existing equality check:
+ expect(normalizeString(executionConfig.engineConfig!.graphqlClientSchema!)) + .toContain('directive @tag(')And in the “include tags” variant (where auth/scope directives are present upstream):
+ expect(normalizeString(executionConfig.engineConfig!.graphqlClientSchema!)) + .toContain('directive @authenticated') + expect(normalizeString(executionConfig.engineConfig!.graphqlClientSchema!)) + .toContain('directive @requiresScopes')If the design is to omit unused directive definitions from the client SDL, please confirm and we’ll skip these.
Also applies to: 1841-1845, 1998-2002, 2104-2108
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/test/contracts.test.ts(21 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T20:57:07.946Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#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:
controlplane/test/contracts.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_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
Aenimus
deleted the
david/eng-8198-print-client-schema-with-directives
branch
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests
Checklist