Make gqlgenc recognize Apollo Federation related directives #197
Conversation
WalkthroughThe update introduces a new Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sum
Files selected for processing (3)
- README.md (2 hunks)
- config/config.go (1 hunks)
- generator/generator.go (2 hunks)
Additional comments: 6
generator/generator.go (1)
- 11-11: The import of the
federationpackage fromgithub.com/99designs/gqlgen/plugin/federationis correctly added to support Apollo Federation directives.README.md (3)
- 45-46: The addition of the
federationfield with theversion: 2directive under themodelssection in the configuration file is correctly documented. This change effectively communicates how to enable Apollo Federation support.- 72-73: The documentation for adding Apollo Federation directives in a local schema configuration is clear and follows the same pattern as the remote configuration. This consistency helps users understand how to configure federation support regardless of schema source.
- 42-49: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-100]
The README.md updates provide clear instructions on how to configure Apollo Federation support in
gqlgenc. Ensure that all new configuration options are also documented in the official documentation or wiki for comprehensive coverage.config/config.go (2)
- 27-27: The addition of the
Federationfield of typeconfig.PackageConfigto theConfigstruct is correctly implemented. This change aligns with the PR objectives to support Apollo Federation by adding necessary configuration options.- 24-30: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-100]
The
Configstruct now includes theFederationfield, which is a significant addition for supporting Apollo Federation. Ensure that all functions and methods that manipulate or access theConfigstruct are updated to consider this new field appropriately.
| if cfg.Federation.Version != 0 { | ||
| if fed, ok := federation.New(cfg.Federation.Version).(plugin.EarlySourceInjector); ok { | ||
| if source := fed.InjectSourceEarly(); source != nil { | ||
| cfg.GQLConfig.Sources = append(cfg.GQLConfig.Sources, source) | ||
| } | ||
| } else { | ||
| return errors.New("failed to inject federation directives") | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The search for validation of the federation version in the codebase only returned references in generator/generator.go, specifically in the conditional block we are reviewing and the instantiation of a federation object. There are no explicit checks elsewhere in the code to ensure that cfg.Federation.Version is a positive integer beyond the check that it is not 0.
Given this context, it appears that there is no further validation of the federation version beyond ensuring it is not 0. This could potentially allow for invalid federation version values that are negative, assuming the type allows for such values, which is not explicitly verified but can be inferred based on common Go practices and the nature of versioning (typically non-negative).
Since the verification script did not find any additional validation logic for the federation version, and based on the information provided, it seems that the initial review comment stands correct in its advice but also reveals a potential oversight in validation logic within the codebase.
Analysis chain
The conditional logic to inject federation directives into the GraphQL schema only if the federation version is not 0 is correctly implemented. However, ensure that the federation version is validated elsewhere in the code to prevent invalid values.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that federation version is validated to be a positive integer elsewhere in the code.
rg "cfg.Federation.Version" --type go
Length of output: 207
| if cfg.Federation.Version != 0 { | ||
| if fed, ok := federation.New(cfg.Federation.Version).(plugin.EarlySourceInjector); ok { | ||
| if source := fed.InjectSourceEarly(); source != nil { | ||
| cfg.GQLConfig.Sources = append(cfg.GQLConfig.Sources, source) | ||
| } | ||
| } else { | ||
| return errors.New("failed to inject federation directives") | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation for injecting federation directives is clear and follows the intended logic. However, consider adding a log or debug statement before returning an error in line 62 for better debugging and operational visibility.
+ log.Printf("Federation version: %d is not supported for EarlySourceInjector", cfg.Federation.Version)
return errors.New("failed to inject federation directives")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if cfg.Federation.Version != 0 { | |
| if fed, ok := federation.New(cfg.Federation.Version).(plugin.EarlySourceInjector); ok { | |
| if source := fed.InjectSourceEarly(); source != nil { | |
| cfg.GQLConfig.Sources = append(cfg.GQLConfig.Sources, source) | |
| } | |
| } else { | |
| return errors.New("failed to inject federation directives") | |
| } | |
| } | |
| if cfg.Federation.Version != 0 { | |
| if fed, ok := federation.New(cfg.Federation.Version).(plugin.EarlySourceInjector); ok { | |
| if source := fed.InjectSourceEarly(); source != nil { | |
| cfg.GQLConfig.Sources = append(cfg.GQLConfig.Sources, source) | |
| } | |
| } else { | |
| log.Printf("Federation version: %d is not supported for EarlySourceInjector", cfg.Federation.Version) | |
| return errors.New("failed to inject federation directives") | |
| } | |
| } |
generator/generator.go
Outdated
| "github.com/99designs/gqlgen/plugin/modelgen" | ||
|
|
There was a problem hiding this comment.
Please run make fmt as it seems the code does not adhere to the gci format.
110y
force-pushed
the
federation-directives
branch
from
a3445b5
to
a88dcd6
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sum
Files selected for processing (3)
- README.md (2 hunks)
- config/config.go (1 hunks)
- generator/generator.go (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- README.md
- config/config.go
- generator/generator.go
|
Ah sorry, fixed it! I appreciate it if you could take another look, thanks 🙏 |
What
gqlgencrecognize Apollo Federation related GraphQL directives.Why
Current
gqlgencraises anUndefined directiveerror when a target GraphQL schema includes Apollo Federation related directives. This is just becausegqlgencdoes not recognize unknown directives not declared in a schema.Regarding the original
gqlgen, it has special treatments for Apollo Federation, so we don't have to explicitly declare Apollo Federation related directives in our schema.Given that, I think it is better to make
gqlgencrecognize Apollo Federation related directives like the originalgqlgen.Summary by CodeRabbit