-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make gqlgenc recognize Apollo Federation related directives #197
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2,11 +2,13 @@ package generator | |||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
import ( | ||||||||||||||||||||||||||||||||||||||||
"context" | ||||||||||||||||||||||||||||||||||||||||
"errors" | ||||||||||||||||||||||||||||||||||||||||
"fmt" | ||||||||||||||||||||||||||||||||||||||||
"sort" | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
"github.com/99designs/gqlgen/api" | ||||||||||||||||||||||||||||||||||||||||
"github.com/99designs/gqlgen/plugin" | ||||||||||||||||||||||||||||||||||||||||
"github.com/99designs/gqlgen/plugin/federation" | ||||||||||||||||||||||||||||||||||||||||
"github.com/99designs/gqlgen/plugin/modelgen" | ||||||||||||||||||||||||||||||||||||||||
"github.com/Yamashou/gqlgenc/config" | ||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||
|
@@ -50,6 +52,16 @@ func Generate(ctx context.Context, cfg *config.Config, option ...api.Option) err | |||||||||||||||||||||||||||||||||||||||
o(cfg.GQLConfig, &plugins) | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
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") | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
Comment on lines
+55
to
+63
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
if err := cfg.LoadSchema(ctx); err != nil { | ||||||||||||||||||||||||||||||||||||||||
return fmt.Errorf("failed to load schema: %w", err) | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thatcfg.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:
Length of output: 207