Skip to content
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.

Sign up for GitHub

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

fix: Handle order input field argument being nil #701

Merged
merged 8 commits into from
Sep 2, 2022
Merged

fix: Handle order input field argument being nil #701

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
95c5b27
wip: Check for nil and also don't panic if target is not found, inste…
shahzadlone Aug 1, 2022
a7cea05
PR: Stop execution according to Andy's suggestion.
shahzadlone Aug 12, 2022
6f73d38
PR: Fix the order's `nil` type error.
shahzadlone Aug 19, 2022
876315e
PR: Remove the logging and format error according to suggestion.
shahzadlone Aug 24, 2022
0a32f85
PR: Add schema test but it still doesn't have the order nil input type.
shahzadlone Aug 24, 2022
4fb13d7
PR: Use `ContainsData` to trim the test down.
shahzadlone Sep 1, 2022
f20d7fd
PR: Remove ctx
shahzadlone Sep 1, 2022
9046555
PR: Adhere to code review.
shahzadlone Sep 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 39 additions & 23 deletions query/graphql/schema/generate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,7 @@ func (g *Generator) fromAST(ctx context.Context, document *ast.Document) ([]*gql
return nil, err
}

// for each built type
// generate query inputs
// for each built type generate query inputs
queryType := g.manager.schema.QueryType()
generatedQueryFields := make([]*gql.Field, 0)
for _, t := range g.typeDefs {
Expand Down Expand Up @@ -246,8 +245,6 @@ func (g *Generator) expandInputArgument(obj *gql.Object) error {
return err
}

// obj.AddFieldConfig(f, expandedField)
// obj := g.manager.schema.Type(obj.Name()).(*gql.Object)
obj.AddFieldConfig(f, expandedField)

case *gql.List: // new field object with arguments (list)
Expand All @@ -270,7 +267,9 @@ func (g *Generator) expandInputArgument(obj *gql.Object) error {
}
case *gql.Scalar:
if _, isAggregate := parserTypes.Aggregates[f]; isAggregate {
g.createExpandedFieldAggregate(obj, def)
if err := g.createExpandedFieldAggregate(obj, def); err != nil {
return err
}
}
// @todo: check if NonNull is possible here
//case *gql.NonNull:
Expand All @@ -284,26 +283,33 @@ func (g *Generator) expandInputArgument(obj *gql.Object) error {
func (g *Generator) createExpandedFieldAggregate(
obj *gql.Object,
f *gql.FieldDefinition,
) {
) error {
for _, aggregateTarget := range f.Args {
target := aggregateTarget.Name()
var filterTypeName string
if target == parserTypes.GroupFieldName {
filterTypeName = obj.Name() + "FilterArg"
} else {
filterType := obj.Fields()[target].Type
if list, isList := filterType.(*gql.List); isList && gql.IsLeafType(list.OfType) {
// If it is a list of leaf types - the filter is just the set of OperatorBlocks
// that are supported by this type - there can be no field selections.
if notNull, isNotNull := list.OfType.(*gql.NonNull); isNotNull {
// GQL does not support '!' in type names, and so we have to manipulate the
// underlying name like this if it is a nullable type.
filterTypeName = fmt.Sprintf("NotNull%sOperatorBlock", notNull.OfType.Name())
if targeted := obj.Fields()[target]; targeted != nil {
if list, isList := targeted.Type.(*gql.List); isList && gql.IsLeafType(list.OfType) {
// If it is a list of leaf types - the filter is just the set of OperatorBlocks
// that are supported by this type - there can be no field selections.
if notNull, isNotNull := list.OfType.(*gql.NonNull); isNotNull {
// GQL does not support '!' in type names, and so we have to manipulate the
// underlying name like this if it is a nullable type.
filterTypeName = fmt.Sprintf("NotNull%sOperatorBlock", notNull.OfType.Name())
} else {
filterTypeName = genTypeName(list.OfType, "OperatorBlock")
}
} else {
filterTypeName = genTypeName(list.OfType, "OperatorBlock")
filterTypeName = targeted.Type.Name() + "FilterArg"
shahzadlone marked this conversation as resolved.
Show resolved Hide resolved
}
} else {
filterTypeName = filterType.Name() + "FilterArg"
return fmt.Errorf(
"Aggregate target not found. HostObject: {%s}, Target: {%s}",
obj.Name(),
target,
)
}
}

Expand All @@ -315,6 +321,8 @@ func (g *Generator) createExpandedFieldAggregate(
aggregateTarget.Type.(*gql.InputObject).AddFieldConfig("filter", expandedField)
}
}

return nil
}

func (g *Generator) createExpandedFieldSingle(
Expand Down Expand Up @@ -1192,13 +1200,21 @@ func (g *Generator) genTypeOrderArgInput(obj *gql.Object) *gql.InputObject {
if _, ok := parserTypes.ReservedFields[f]; ok && f != parserTypes.DocKeyFieldName {
continue
}
typeMap := g.manager.schema.TypeMap()
if gql.IsLeafType(field.Type) { // only Scalars, and enums
fields[field.Name] = &gql.InputObjectFieldConfig{
Type: g.manager.schema.TypeMap()["Ordering"],
Type: typeMap["Ordering"],
}
} else { // sub objects
fields[field.Name] = &gql.InputObjectFieldConfig{
Type: g.manager.schema.TypeMap()[genTypeName(field.Type, "OrderArg")],
configType, isOrderable := typeMap[genTypeName(field.Type, "OrderArg")]
if !isOrderable {
fields[field.Name] = &gql.InputObjectFieldConfig{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: This does look incorrect, are you sure this is desired? It looks like you are adding unorderable fields to the ordering gql type. Some tests would be quite good to have here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this PR we were adding everything and having Type: nil which was causing the error. We already have this test under: tests/integration/query/one_to_one/with_order_test.go which fails if you remove this.

func TestQueryOneToOneWithChildBooleanOrderDescending(t *testing.T) {
	test := testUtils.QueryTestCase{
		Description: "One-to-one relation query with simple order by sub type",
		Query: `query {
			book(order: {author: {verified: DESC}}) {
				name
				rating
				author {
					name
					age
				}
			}
		}`,
		Docs: map[int][]string{
			//books
			0: {
				// bae-fd541c25-229e-5280-b44b-e5c2af3e374d
				`{
					"name": "Painted House",
					"rating": 4.9
				}`,
				// bae-d432bdfb-787d-5a1c-ac29-dc025ab80095
				`{
					"name": "Theif Lord",
					"rating": 4.8
				}`,
			},
			//authors
			1: {
				// bae-41598f0c-19bc-5da6-813b-e80f14a10df3
				`{
					"name": "John Grisham",
					"age": 65,
					"verified": true,
					"published_id": "bae-fd541c25-229e-5280-b44b-e5c2af3e374d"
				}`,
				// bae-b769708d-f552-5c3d-a402-ccfd7ac7fb04
				`{
					"name": "Cornelia Funke",
					"age": 62,
					"verified": false,
					"published_id": "bae-d432bdfb-787d-5a1c-ac29-dc025ab80095"
				}`,
			},
		},
		Results: []map[string]interface{}{
			{
				"name":   "Painted House",
				"rating": 4.9,
				"author": map[string]interface{}{
					"name": "John Grisham",
					"age":  uint64(65),
				},
			},
			{
				"name":   "Theif Lord",
				"rating": 4.8,
				"author": map[string]interface{}{
					"name": "Cornelia Funke",
					"age":  uint64(62),
				},
			},
		},
	}

	executeTestCase(t, test)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but we need to test that the gql types/schema is correct, for both a valid order(able) field, and a field that is not orderable - something under test/integration/schema

Type: &gql.InputObjectField{},
}
} else {
fields[field.Name] = &gql.InputObjectFieldConfig{
Type: configType,
}
}
}
}
Expand Down Expand Up @@ -1229,7 +1245,7 @@ func (g *Generator) genTypeQueryableFieldList(
if err := g.manager.schema.AppendType(config.filter); err != nil {
log.ErrorE(
ctx,
"Failed to append runtime schema",
"Failed to append runtime schema with filter",
err,
logging.NewKV("SchemaItem", config.filter),
)
Expand All @@ -1238,7 +1254,7 @@ func (g *Generator) genTypeQueryableFieldList(
if err := g.manager.schema.AppendType(config.groupBy); err != nil {
log.ErrorE(
ctx,
"Failed to append runtime schema",
"Failed to append runtime schema with groupBy",
err,
logging.NewKV("SchemaItem", config.groupBy),
)
Expand All @@ -1247,7 +1263,7 @@ func (g *Generator) genTypeQueryableFieldList(
if err := g.manager.schema.AppendType(config.having); err != nil {
log.ErrorE(
ctx,
"Failed to append runtime schema",
"Failed to append runtime schema with having",
err,
logging.NewKV("SchemaItem", config.having),
)
Expand All @@ -1256,7 +1272,7 @@ func (g *Generator) genTypeQueryableFieldList(
if err := g.manager.schema.AppendType(config.order); err != nil {
log.ErrorE(
ctx,
"Failed to append runtime schema",
"Failed to append runtime schema with order",
err,
logging.NewKV("SchemaItem", config.order),
)
Expand Down
16 changes: 16 additions & 0 deletions tests/integration/schema/default_fields.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,3 +112,19 @@ var aggregateFields = fields{
},
},
}

// makeInputObject retrned a properly made input field type
// using name (outer), name of type (inner), and types ofType.
func makeInputObject(
name string,
typeName any,
ofType any,
) map[string]any {
return map[string]any{
"name": name,
"type": map[string]any{
"name": typeName,
"ofType": ofType,
},
}
}
219 changes: 219 additions & 0 deletions tests/integration/schema/input_type_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,219 @@
// Copyright 2022 Democratized Data Foundation
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt.
//
// As of the Change Date specified in that file, in accordance with
// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

package schema

import (
"testing"
)

func TestInputTypeOfOrderFieldWhereSchemaHasRelationType(t *testing.T) {
test := QueryTestCase{
Schema: []string{
Copy link
Contributor

@AndrewSisley AndrewSisley Aug 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo: Why is this schema so complex? I cannot tell what this test is trying to test - is looks like it is trying to test everything! Please reduce the scope and use smaller, more specific test cases

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that once I find the actual input order element that I am looking for (which was previously Type: nil and now Type: &gql.InputObjectField{} ).

`
type book {
name: String
rating: Float
author: author
}

type author {
name: String
age: Int
verified: Boolean
wrote: book @primary
}
`,
},
IntrospectionQuery: `
query IntrospectionQuery {
__type (name: "author") {
name
fields {
name
args {
name
type {
name
ofType {
name
kind
}
inputFields {
name
type {
name
ofType {
name
kind
}
}
}
}
}
}
}
}
`,
ContainsData: map[string]any{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: This looks much more focused than I was expecting! Thank you very much.

defaultGroupArgsWithoutOrder still exists, and I am curious as to whether we'll end up tripping over it and will need to keep tweaking it with unrelated changes long term, but now it is very clear what the test cares about.

Am curious if we can do something similar for explain, and am looking forward to having a play with this (as mentioned am planning on writing similar tests this morning)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha thanks for the nudge in the right direction.

Good point, was wondering the same, if we do end up tripping over it, should be able to further break it down into their own individual defaults (not sure how much that would help though).

Funny you mention that, I actually did something similar for explain test refactor when you left for vacation last month, I been wanted to open a draft for feedback however I keep prioritizing 0.3.1 tasks as explain test refactor needs slight more work and is marked for 0.4. Will ping you for feedback on it soon.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice sounds good :)

"__type": map[string]any{
"name": "author",
"fields": []any{
map[string]any{
// Asserting only on group, because it is the field that contains `order` info we are
// looking for, additionally wanted to reduce the noise of other elements that were getting
// dumped out which made the entire output horrible.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks :)

"name": "_group",
"args": append(
defaultGroupArgsWithoutOrder,
map[string]any{
"name": "order",
"type": map[string]any{
"name": "authorOrderArg",
"ofType": nil,
"inputFields": []any{
map[string]any{
"name": "_key",
"type": map[string]any{
"name": "Ordering",
"ofType": nil,
},
},
map[string]any{
"name": "age",
"type": map[string]any{
"name": "Ordering",
"ofType": nil,
},
},
map[string]any{
"name": "name",
"type": map[string]any{
"name": "Ordering",
"ofType": nil,
},
},
map[string]any{
"name": "verified",
"type": map[string]any{
"name": "Ordering",
"ofType": nil,
},
},
// Without the relation type we won't have the following ordering type(s).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cheers :)

map[string]any{
"name": "wrote",
"type": map[string]any{
"name": "bookOrderArg",
"ofType": nil,
},
},
map[string]any{
"name": "wrote_id",
"type": map[string]any{
"name": "Ordering",
"ofType": nil,
},
},
},
},
},
),
},
},
},
},
}

ExecuteQueryTestCase(t, test)
}

var defaultGroupArgsWithoutOrder = []any{

map[string]any{
"name": "filter",
"type": filterArg,
},

map[string]any{
"name": "groupBy",
"type": groupByArg,
},

map[string]any{
"name": "having",
"type": havingArg,
},

map[string]any{
"name": "limit",
"type": intArgType,
},

map[string]any{
"name": "offset",
"type": intArgType,
},
}

var groupByArg = map[string]any{
"inputFields": nil,
"name": nil,
"ofType": map[string]any{
"kind": "NON_NULL",
"name": nil,
},
}

var intArgType = map[string]any{
"inputFields": nil,
"name": "Int",
"ofType": nil,
}

var filterArg = map[string]any{
"name": "authorFilterArg",
"ofType": nil,
"inputFields": []any{
makeInputObject("_and", nil, inputObjAuthorFilterArg),
makeInputObject("_key", "IDOperatorBlock", nil),
makeInputObject("_not", "authorFilterArg", nil),
makeInputObject("_or", nil, inputObjAuthorFilterArg),
makeInputObject("age", "IntOperatorBlock", nil),
makeInputObject("name", "StringOperatorBlock", nil),
makeInputObject("verified", "BooleanOperatorBlock", nil),
makeInputObject("wrote", "bookFilterBaseArg", nil),
makeInputObject("wrote_id", "IDOperatorBlock", nil),
},
}

var havingArg = map[string]any{
"name": "authorHavingArg",
"ofType": nil,
"inputFields": []any{
makeAuthorHavingBlockForName("_avg"),
makeAuthorHavingBlockForName("_count"),
makeAuthorHavingBlockForName("_key"),
makeAuthorHavingBlockForName("_sum"),
makeAuthorHavingBlockForName("age"),
makeAuthorHavingBlockForName("name"),
makeAuthorHavingBlockForName("verified"),
makeAuthorHavingBlockForName("wrote_id"),
},
}

var inputObjAuthorFilterArg = map[string]any{
"kind": "INPUT_OBJECT",
"name": "authorFilterArg",
}

func makeAuthorHavingBlockForName(name string) map[string]any {
return makeInputObject(name, "authorHavingBlock", nil)
}
2 changes: 2 additions & 0 deletions tests/integration/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,6 +617,8 @@ func assertQueryResults(
if assertErrors(t, description, result.Errors, expectedError) {
return true
}

// Note: if result.Data == nil this panics (the panic seems useful while testing).
resultantData := result.Data.([]map[string]interface{})

log.Info(ctx, "", logging.NewKV("QueryResults", result.Data))
Expand Down