Conversation
Noroth
requested review from
JivusAyrus,
StarpTech,
endigma and
jensneuse
as code owners
WalkthroughThis change introduces a GraphQL SDL validation step into the gRPC service and router plugin code generation workflows. It adds a modular SDL validation framework with configurable rules, a function to render validation results in the console, and several new schema fixtures and tests to verify handling of warnings and errors. The validation logic is encapsulated in a new visitor class, with corresponding exports and comprehensive test coverage. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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). (2)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
protographic/src/index.ts (1)
1-112: Fix code formatting issues.The pipeline indicates Prettier formatting check failed. Please run
prettier --writeto fix the code style issues before merging.
🧹 Nitpick comments (3)
protographic/tests/sdl-validation/01-basic-validation.test.ts (1)
1-3: Remove unused importThe
buildSchemaimport from graphql is not used anywhere in this test file.-import { buildSchema } from 'graphql'; import { describe, expect, test } from 'vitest'; import { SDLValidationVisitor } from '../../src/sdl-validation-visitor';cli/test/grpc-service.test.ts (1)
193-195: Remove empty line for consistencyRemove the empty line to match the formatting in other test cases.
const originalConsoleLog = console.log; const consoleOutput: string[] = []; - console.log = (...args: any[]) => {protographic/src/sdl-validation-visitor.ts (1)
120-137: Remove unnecessary null checkThe GraphQL
parse()function throws an error on invalid input rather than returning null, making the null check unnecessary.public visit(): ValidationResult { try { const astNode = parse(this.schema); - if (!astNode) { - throw new Error('Schema parsing resulted in null AST'); - } const visitor = this.createASTVisitor(); visit(astNode, visitor);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cli/src/commands/grpc-service/commands/generate.ts(2 hunks)cli/src/commands/router/commands/plugin/helper.ts(2 hunks)cli/src/commands/router/commands/plugin/toolchain.ts(3 hunks)cli/test/fixtures/schema-with-nullable-list-items.graphql(1 hunks)cli/test/fixtures/schema-with-validation-errors.graphql(1 hunks)cli/test/fixtures/schema-with-warnings-and-errors.graphql(1 hunks)cli/test/grpc-service.test.ts(4 hunks)protographic/src/index.ts(2 hunks)protographic/src/sdl-validation-visitor.ts(1 hunks)protographic/tests/sdl-validation/01-basic-validation.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Learnt from: SkArchon
PR: wundergraph/cosmo#1929
File: router/internal/circuit/manager.go:16-25
Timestamp: 2025-06-30T20:39:02.387Z
Learning: In the Cosmo router project, parameter validation for circuit breaker configuration is handled at the JSON schema level rather than through runtime validation methods on structs. The config.schema.json file contains comprehensive validation constraints for circuit breaker parameters.
📚 Learning: in the cosmo router project, required field validation for jwks configuration (secret, algorithm, ke...
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Applied to files:
cli/src/commands/grpc-service/commands/generate.tscli/src/commands/router/commands/plugin/toolchain.ts
📚 Learning: in the cosmo router project, when extending json schema validation for security-sensitive fields lik...
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.
Applied to files:
cli/src/commands/router/commands/plugin/helper.tscli/src/commands/router/commands/plugin/toolchain.tscli/test/fixtures/schema-with-warnings-and-errors.graphql
🧬 Code Graph Analysis (2)
protographic/src/index.ts (1)
protographic/src/sdl-validation-visitor.ts (2)
ValidationResult(18-23)SDLValidationVisitor(59-414)
cli/src/commands/router/commands/plugin/toolchain.ts (2)
protographic/src/index.ts (1)
validateGraphQLSDL(88-91)cli/src/commands/router/commands/plugin/helper.ts (1)
renderValidationResults(75-132)
🪛 GitHub Actions: Protographic CI
protographic/src/index.ts
[warning] 1-1: Prettier formatting check failed. Code style issues found. Run 'prettier --write' to fix code style issues.
protographic/src/sdl-validation-visitor.ts
[warning] 1-1: Prettier formatting check failed. Code style issues found. Run 'prettier --write' to fix code style issues.
🪛 GitHub Actions: wgc CI
cli/src/commands/router/commands/plugin/helper.ts
[warning] 1-1: Prettier formatting check failed. Code style issues found. Run Prettier with --write to fix.
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (28)
cli/src/commands/grpc-service/commands/generate.ts (3)
2-7: LGTM! Clean import addition.The
validateGraphQLSDLfunction is properly imported from the protographic module alongside existing functions.
13-13: LGTM! Proper helper function import.The
renderValidationResultsimport is correctly added from the helper module.
130-136: Excellent validation integration!The validation step is well-positioned in the workflow and properly integrated:
- Validates after schema reading but before generation
- Provides clear user feedback via spinner
- Uses the validation results rendering for consistent output
- Follows the established pattern where errors halt generation while warnings allow continuation
protographic/src/index.ts (3)
7-7: LGTM! Proper import of validation components.The import correctly brings in the
SDLValidationVisitorclass andValidationResulttype needed for the validation functionality.
81-91: Well-implemented validation function!The
validateGraphQLSDLfunction has a clean implementation:
- Clear and focused responsibility (validate GraphQL SDL)
- Proper error handling documentation
- Consistent with the module's patterns
- Matches the usage pattern in the CLI integration
96-96: LGTM! Necessary exports for validation functionality.The exports properly expose the
SDLValidationVisitorclass andValidationResulttype for external use, following the established patterns in the module.Also applies to: 100-100
cli/test/fixtures/schema-with-nullable-list-items.graphql (1)
1-18: Well-designed test fixture for validation testing.This schema fixture effectively tests the nullable list items validation:
- Contains realistic GraphQL schema structure
- The
tags: [String]!field on line 12 properly demonstrates nullable items in a non-nullable list- Will appropriately trigger validation warnings without causing errors
- Good balance of different field types for comprehensive testing
cli/src/commands/router/commands/plugin/toolchain.ts (3)
7-12: LGTM! Consistent import additions.The imports properly add the validation functionality with the same pattern used in the gRPC service generation command, ensuring consistency across workflows.
Also applies to: 17-17
368-369: Good refactoring for better code organization.Storing the schema file path in a variable improves readability and enables its reuse for validation result rendering.
384-387: Excellent consistency with gRPC service validation!The validation implementation mirrors the gRPC service generation command perfectly:
- Same workflow positioning (after schema reading, before generation)
- Same user feedback via spinner updates
- Same validation and rendering function calls
- Ensures consistent behavior across both generation workflows
cli/test/fixtures/schema-with-warnings-and-errors.graphql (1)
1-16: Excellent comprehensive validation test fixture!This schema effectively tests the complete validation workflow:
- Warnings: Lines 3-4 with nullable list items (
[String],[User]) will trigger nullable-items-in-list-types warnings- Errors: Line 13 with nested key directive (
@key(fields: "id nested { name }")) will trigger nested-key-directives error- Documentation: Comments clearly explain expected validation behavior
- Realism: Represents a practical schema structure that validates real-world scenarios
Perfect for testing both individual validation rules and their interaction.
protographic/tests/sdl-validation/01-basic-validation.test.ts (5)
6-18: LGTM!Basic validation test is well-structured and covers the happy path scenario correctly.
59-72: LGTM!Test correctly validates that nullable list items generate appropriate warnings.
74-87: LGTM!Test correctly handles nested list validation scenarios.
89-111: LGTM!Test correctly validates that nested key directives generate errors as expected.
113-133: LGTM!Test correctly validates that @requires directive generates appropriate warnings.
cli/test/fixtures/schema-with-validation-errors.graphql (1)
1-13: LGTM!The fixture correctly implements a schema with nested key directive that will trigger validation errors as intended for testing purposes.
cli/src/commands/router/commands/plugin/helper.ts (2)
1-1: LGTM!Import is correctly added for the new validation functionality.
75-132: Well-structured validation result rendererThe function provides excellent user experience with:
- Clear visual hierarchy using tree-like formatting
- Appropriate use of colors for warnings (yellow) and errors (red)
- Limiting output to 10 items to avoid overwhelming users
- Proper error propagation to stop generation on errors
cli/test/grpc-service.test.ts (4)
2-14: LGTM!Imports are correctly updated to support ES module path resolution for test fixtures.
174-230: LGTM!Test correctly validates that generation continues with warnings and properly captures/restores console output.
232-290: LGTM!Test correctly validates that generation fails on errors and no output files are created.
292-357: LGTM!Test comprehensively validates the behavior when both warnings and errors are present, ensuring proper message ordering and generation failure.
protographic/src/sdl-validation-visitor.ts (5)
1-53: LGTM!Well-structured imports and interface definitions with comprehensive TypeScript types.
59-76: LGTM!Clean class setup with proper initialization and documentation.
194-222: LGTM!The nullable list validation logic correctly handles nested list types and identifies nullable items.
256-277: LGTM!The key directive validation correctly identifies nested field selections and validates the fields argument type.
303-413: LGTM!Helper methods are well-implemented with proper null checks and consistent formatting.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
protographic/src/sdl-validation-visitor.ts (1)
1-388: Fix Prettier formatting issuesThe pipeline indicates formatting issues in this file. Please run Prettier to fix code style.
Run the following command to fix formatting:
npx prettier --write protographic/src/sdl-validation-visitor.ts
🧹 Nitpick comments (1)
protographic/src/sdl-validation-visitor.ts (1)
123-125: Remove unnecessary null check after parse()The
parse()function from GraphQL.js either returns a validDocumentNodeor throws an error. It never returnsnullorundefined, making this check redundant.try { const astNode = parse(this.schema); - if (!astNode) { - throw new Error('Schema parsing resulted in null AST'); - } const visitor = this.createASTVisitor();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cli/src/commands/router/commands/plugin/helper.ts(2 hunks)protographic/src/index.ts(2 hunks)protographic/src/sdl-validation-visitor.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- protographic/src/index.ts
- cli/src/commands/router/commands/plugin/helper.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Learnt from: SkArchon
PR: wundergraph/cosmo#1929
File: router/internal/circuit/manager.go:16-25
Timestamp: 2025-06-30T20:39:02.387Z
Learning: In the Cosmo router project, parameter validation for circuit breaker configuration is handled at the JSON schema level rather than through runtime validation methods on structs. The config.schema.json file contains comprehensive validation constraints for circuit breaker parameters.
📚 Learning: in the protographic sdl validation system, the validation logic for nullable list items only refers ...
Learnt from: Noroth
PR: wundergraph/cosmo#2108
File: protographic/tests/sdl-validation/01-basic-validation.test.ts:20-57
Timestamp: 2025-08-05T13:00:30.255Z
Learning: In the protographic SDL validation system, the validation logic for nullable list items only refers to the actual list type and not the nesting. For example, `[[String!]]` (nullable list of non-nullable lists) would not trigger a nullable list warning because the validation focuses on the immediate list structure rather than nested list structures.
Applied to files:
protographic/src/sdl-validation-visitor.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). (2)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
Noroth
deleted the
ludwig/eng-7712-pre-check-the-graphql-schema-in-protographic
branch
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Checklist