Skip to content

Commit

Permalink
fix: requires+provides edge case (#928)
Browse files Browse the repository at this point in the history 
- do not add representation for root level request
- cleanup configuration visitor
  • Loading branch information
@devsergiy
devsergiy authored Oct 17, 2024
1 parent 9c8ca51 commit a9a2b9e
Show file tree
Hide file tree
Showing 6 changed files with 114 additions and 250 deletions.
12 changes: 8 additions & 4 deletions v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -342,11 +342,11 @@ func (p *Planner[T]) shouldSelectSingleEntity() bool {
}

func (p *Planner[T]) requiresEntityFetch() bool {
return p.dataSourcePlannerConfig.HasRequiredFields() && p.dataSourcePlannerConfig.PathType == plan.PlannerPathObject
return p.hasFederationRoot && p.dataSourcePlannerConfig.HasRequiredFields() && p.dataSourcePlannerConfig.PathType == plan.PlannerPathObject
}

func (p *Planner[T]) requiresEntityBatchFetch() bool {
return p.dataSourcePlannerConfig.HasRequiredFields() && p.dataSourcePlannerConfig.PathType != plan.PlannerPathObject
return p.hasFederationRoot && p.dataSourcePlannerConfig.HasRequiredFields() && p.dataSourcePlannerConfig.PathType != plan.PlannerPathObject
}

func (p *Planner[T]) ConfigureSubscription() plan.SubscriptionConfiguration {
Expand Down Expand Up @@ -710,6 +710,10 @@ func (p *Planner[T]) LeaveDocument(_, _ *ast.Document) {
}

func (p *Planner[T]) addRepresentationsVariable() {
if !p.hasFederationRoot {
return
}

if !p.dataSourcePlannerConfig.HasRequiredFields() {
return
}
Expand Down Expand Up @@ -1246,7 +1250,7 @@ func (p *Planner[T]) debugPrintQueryPlan(operation *ast.Document) {
"\n")
}

if p.dataSourcePlannerConfig.HasRequiredFields() { // IsRepresentationsQuery
if p.hasFederationRoot && p.dataSourcePlannerConfig.HasRequiredFields() { // IsRepresentationsQuery
args = append(args, "Representations:\n")
for _, cfg := range p.dataSourcePlannerConfig.RequiredFields {
key, report := plan.RequiredFieldsFragment(cfg.TypeName, cfg.SelectionSet, cfg.FieldName == "")
Expand Down Expand Up @@ -1281,7 +1285,7 @@ func (p *Planner[T]) generateQueryPlansForFetchConfiguration(operation *ast.Docu
var (
representations []resolve.Representation
)
if p.dataSourcePlannerConfig.HasRequiredFields() { // IsRepresentationsQuery
if p.hasFederationRoot && p.dataSourcePlannerConfig.HasRequiredFields() { // IsRepresentationsQuery
representations = make([]resolve.Representation, len(p.dataSourcePlannerConfig.RequiredFields))
for i, cfg := range p.dataSourcePlannerConfig.RequiredFields {
fragmentAst, report := plan.QueryPlanRequiredFieldsFragment(cfg.FieldName, cfg.TypeName, cfg.SelectionSet)
Expand Down
205 changes: 25 additions & 180 deletions v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -594,6 +594,7 @@ func TestGraphQLDataSourceFederation(t *testing.T) {

// TODO: add test for requires from 2 sibling subgraphs - should be Serial: Parallel -> Single
// TODO: add test for requires when query already has the required field with the different argument - it is using field from a query not with default arg
// TODO: add test for partially provided required fields

usersDatasourceConfiguration := mustDataSourceConfiguration(t,
"user.service",
Expand All @@ -608,8 +609,9 @@ func TestGraphQLDataSourceFederation(t *testing.T) {
FieldNames: []string{"id", "account", "oldAccount"},
},
{
TypeName: "Account",
FieldNames: []string{"id", "info", "address", "deliveryAddress"},
TypeName: "Account",
FieldNames: []string{"id", "info", "address", "deliveryAddress"},
ExternalFieldNames: []string{"name", "shippingInfo"},
},
{
TypeName: "Address",
Expand All @@ -618,8 +620,8 @@ func TestGraphQLDataSourceFederation(t *testing.T) {
},
ChildNodes: []plan.TypeField{
{
TypeName: "ShippingInfo",
FieldNames: []string{"zip"},
TypeName: "ShippingInfo",
ExternalFieldNames: []string{"zip"},
},
{
TypeName: "Info",
Expand Down Expand Up @@ -2639,8 +2641,6 @@ func TestGraphQLDataSourceFederation(t *testing.T) {
})

t.Run("nested selection set - but requirements are provided", func(t *testing.T) {
t.Skip("fixme")

operation := `
query Requires {
user {
Expand All @@ -2657,20 +2657,20 @@ func TestGraphQLDataSourceFederation(t *testing.T) {
expectedPlan := func() *plan.SynchronousResponsePlan {
return &plan.SynchronousResponsePlan{
Response: &resolve.GraphQLResponse{
Data: &resolve.Object{
Fetches: []resolve.Fetch{
&resolve.SingleFetch{
FetchDependencies: resolve.FetchDependencies{
FetchID: 0,
},
DataSourceIdentifier: []byte("graphql_datasource.Source"),
FetchConfiguration: resolve.FetchConfiguration{
Input: `{"method":"POST","url":"http://user.service","body":{"query":"{user {account {address {line1 line2 __typename id}}}}"}}`,
DataSource: &Source{},
PostProcessing: DefaultPostProcessingConfiguration,
},
Fetches: resolve.Sequence(
resolve.Single(&resolve.SingleFetch{
FetchDependencies: resolve.FetchDependencies{
FetchID: 0,
},
},
DataSourceIdentifier: []byte("graphql_datasource.Source"),
FetchConfiguration: resolve.FetchConfiguration{
Input: `{"method":"POST","url":"http://user.service","body":{"query":"{user {oldAccount {deliveryAddress {line1} shippingInfo {zip} __typename id info {a b}}}}"}}`,
DataSource: &Source{},
PostProcessing: DefaultPostProcessingConfiguration,
},
}),
),
Data: &resolve.Object{
Fields: []*resolve.Field{
{
Name: []byte("user"),
Expand All @@ -2679,176 +2679,21 @@ func TestGraphQLDataSourceFederation(t *testing.T) {
Nullable: true,
Fields: []*resolve.Field{
{
Name: []byte("account"),
Name: []byte("oldAccount"),
Value: &resolve.Object{
Path: []string{"account"},
Path: []string{"oldAccount"},
Nullable: true,
Fields: []*resolve.Field{
{
Name: []byte("address"),
Name: []byte("deliveryAddress"),
Value: &resolve.Object{
Path: []string{"address"},
Path: []string{"deliveryAddress"},
Nullable: true,
Fields: []*resolve.Field{
{
Name: []byte("fullAddress"),
Name: []byte("line1"),
Value: &resolve.String{
Path: []string{"fullAddress"},
},
},
},
Fetches: []resolve.Fetch{
&resolve.SingleFetch{
FetchDependencies: resolve.FetchDependencies{
FetchID: 3,
DependsOnFetchIDs: []int{0},
},
DataSourceIdentifier: []byte("graphql_datasource.Source"),
FetchConfiguration: resolve.FetchConfiguration{
Input: `{"method":"POST","url":"http://address-enricher.service","body":{"query":"query($representations: [_Any!]!){_entities(representations: $representations){... on Address {__typename country city}}}","variables":{"representations":[$$0$$]}}}`,
DataSource: &Source{},
PostProcessing: SingleEntityPostProcessingConfiguration,
RequiresEntityFetch: true,
Variables: []resolve.Variable{
&resolve.ResolvableObjectVariable{
Renderer: resolve.NewGraphQLVariableResolveRenderer(&resolve.Object{
Nullable: true,
Fields: []*resolve.Field{
{
Name: []byte("__typename"),
Value: &resolve.String{
Path: []string{"__typename"},
},
OnTypeNames: [][]byte{[]byte("Address")},
},
{
Name: []byte("id"),
Value: &resolve.String{
Path: []string{"id"},
},
OnTypeNames: [][]byte{[]byte("Address")},
},
},
}),
},
},
SetTemplateOutputToNullOnVariableNull: true,
},
},
&resolve.SingleFetch{
FetchDependencies: resolve.FetchDependencies{
FetchID: 2,
DependsOnFetchIDs: []int{0, 3},
},
DataSourceIdentifier: []byte("graphql_datasource.Source"),
FetchConfiguration: resolve.FetchConfiguration{
Input: `{"method":"POST","url":"http://address.service","body":{"query":"query($representations: [_Any!]!){_entities(representations: $representations){... on Address {__typename line3(test: "BOOM") zip}}}","variables":{"representations":[$$0$$]}}}`,
DataSource: &Source{},
PostProcessing: SingleEntityPostProcessingConfiguration,
RequiresEntityFetch: true,
Variables: []resolve.Variable{
&resolve.ResolvableObjectVariable{
Renderer: resolve.NewGraphQLVariableResolveRenderer(&resolve.Object{
Nullable: true,
Fields: []*resolve.Field{
{
Name: []byte("__typename"),
Value: &resolve.String{
Path: []string{"__typename"},
},
OnTypeNames: [][]byte{[]byte("Address")},
},
{
Name: []byte("id"),
Value: &resolve.String{
Path: []string{"id"},
},
OnTypeNames: [][]byte{[]byte("Address")},
},
{
Name: []byte("country"),
Value: &resolve.String{
Path: []string{"country"},
},
OnTypeNames: [][]byte{[]byte("Address")},
},
{
Name: []byte("city"),
Value: &resolve.String{
Path: []string{"city"},
},
OnTypeNames: [][]byte{[]byte("Address")},
},
},
}),
},
},
SetTemplateOutputToNullOnVariableNull: true,
},
},
&resolve.SingleFetch{
FetchDependencies: resolve.FetchDependencies{
FetchID: 1,
DependsOnFetchIDs: []int{0, 2},
},
DataSourceIdentifier: []byte("graphql_datasource.Source"),
FetchConfiguration: resolve.FetchConfiguration{
Input: `{"method":"POST","url":"http://account.service","body":{"query":"query($representations: [_Any!]!){_entities(representations: $representations){... on Address {__typename fullAddress}}}","variables":{"representations":[$$0$$]}}}`,
DataSource: &Source{},
PostProcessing: SingleEntityPostProcessingConfiguration,
RequiresEntityFetch: true,
Variables: []resolve.Variable{
&resolve.ResolvableObjectVariable{
Renderer: resolve.NewGraphQLVariableResolveRenderer(&resolve.Object{
Nullable: true,
Fields: []*resolve.Field{
{
Name: []byte("__typename"),
Value: &resolve.String{
Path: []string{"__typename"},
},
OnTypeNames: [][]byte{[]byte("Address")},
},
{
Name: []byte("id"),
Value: &resolve.String{
Path: []string{"id"},
},
OnTypeNames: [][]byte{[]byte("Address")},
},
{
Name: []byte("line1"),
Value: &resolve.String{
Path: []string{"line1"},
},
OnTypeNames: [][]byte{[]byte("Address")},
},
{
Name: []byte("line2"),
Value: &resolve.String{
Path: []string{"line2"},
},
OnTypeNames: [][]byte{[]byte("Address")},
},
{
Name: []byte("line3"),
Value: &resolve.String{
Path: []string{"line3"},
},
OnTypeNames: [][]byte{[]byte("Address")},
},
{
Name: []byte("zip"),
Value: &resolve.String{
Path: []string{"zip"},
},
OnTypeNames: [][]byte{[]byte("Address")},
},
},
}),
},
},
SetTemplateOutputToNullOnVariableNull: true,
Path: []string{"line1"},
},
},
},
Expand Down
Loading

0 comments on commit a9a2b9e

Please sign in to comment.