fix: Handle order input field argument being nil #701

shahzadlone · 2022-08-01T13:37:21Z

Relevant issue(s)

Resolves #700

Description

This was a bug in (generate.go).genTypeOrderArgInput where if a [field.Type + "OrderArg"] key didn't exist in the typeMap it would return nil and pass that in InputObjectFieldConfig to Type.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

ci and local

Specify the platform(s) on which this was tested:

Arch Linux

codecov · 2022-08-01T13:53:10Z

Codecov Report

Merging #701 (9046555) into develop (d2b4623) will decrease coverage by 0.01%.
The diff coverage is 78.37%.

@@             Coverage Diff             @@
##           develop     #701      +/-   ##
===========================================
- Coverage    58.55%   58.53%   -0.02%     
===========================================
  Files          150      150              
  Lines        16884    16900      +16     
===========================================
+ Hits          9887     9893       +6     
- Misses        6074     6083       +9     
- Partials       923      924       +1

Impacted Files	Coverage Δ
query/graphql/schema/generate.go	`83.24% <78.37%> (-0.80%)`	⬇️

fredcarle

From your description I'm not sure if this is a draft PR or one that is ready to go. 🧐

query/graphql/schema/generate.go

tests/integration/query/one_to_one/with_order_test.go

fredcarle · 2022-08-01T15:07:31Z

From your description I'm not sure if this is a draft PR or one that is ready to go. 🧐

But I see that the commit message says WIP so that answers my question 😄

shahzadlone · 2022-08-01T23:18:17Z

From your description I'm not sure if this is a draft PR or one that is ready to go. 🧐

Sorry @fredcarle is my bad this should have been a draft just looking for feedback on how to support the failing test.

query/graphql/schema/generate.go

AndrewSisley

Looks good but have a couple more suggestions for you, and a test or two that fails before the fix goes in would be great :)

query/graphql/schema/generate.go

AndrewSisley · 2022-08-19T15:49:02Z

query/graphql/schema/generate.go

-						Type: g.manager.schema.TypeMap()[genTypeName(field.Type, "OrderArg")],
+					configType, isOrderable := typeMap[genTypeName(field.Type, "OrderArg")]
+					if !isOrderable {
+						fields[field.Name] = &gql.InputObjectFieldConfig{


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

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) }

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

AndrewSisley · 2022-08-25T03:47:04Z

tests/integration/schema/input_type_test.go

+				}
+			}
+		`,
+		ExpectedData: map[string]interface{}{


I would strongly recommend using ContainsData instead of ExpectedData here - currently the test will fail if stuff it is not concerned about changes (making the test more complex, and raising the cost of changing those elements)

Makes sense, I was planning to change it once I find out if anything in the ExpectedData changes with new code changes or not that were introduced in this PR (answer to that for this schema test is nothing changes. lmk if there are any other fields I should introspect according to you),

AndrewSisley · 2022-08-25T03:48:41Z

tests/integration/schema/input_type_test.go

+
+func TestInputTypeOfComplexSchema(t *testing.T) {
+	test := QueryTestCase{
+		Schema: []string{


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

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{} ).

query/graphql/schema/generate.go

…ad log error.

fredcarle

LGTM. I just have one low priority suggestion that I think should be addressed.

fredcarle · 2022-09-01T17:20:58Z

query/graphql/schema/generate.go

@@ -216,7 +215,9 @@ func (g *Generator) fromAST(ctx context.Context, document *ast.Document) ([]*gql
 	return defs, nil
 }

-func (g *Generator) expandInputArgument(obj *gql.Object) error {
+func (g *Generator) expandInputArgument(


suggestion: I think we should keep try to keep fonction definitions on one line when it doesn't reduce redability.

I would suggest this is dev/reviewer preference thing and does not matter (even if Shahzad accidentally did this in this instance) - I have known plenty of devs who much prefer this format, and unless we agree to add a linter to enforce this I don't think it should really come up in reviews

It's not just just a personal preference. It's what the go community prefers: https://github.com/golang/go/wiki/CodeReviewComments#line-length

It is a declared personal preference of the developers involved in writing that doc. Unless we add a linter it will only waste time in reviews.

Linters aren't the solution for every idiom. If the community finds that something is a best practice, we should make the effort to follow it. This is why Go is so easy to jump from one project to an other.

I'm going to disagree with you there that this type of line-formatting preference makes Go projects easier to pick up. It'll also be fairly inconsistently enforced without a linter (if the team want to follow this we should find one, as it is wasteful and error prone to rely on eyeballing this).

I would say it's part of it. The combination of all the idioms and community guidelines makes the language very approachable.

In any case, this was a suggestion. Shahzad does't have to follow it if he doesn't want to. I just think we should make an effort to follow guidelines.

I will make this change mostly because undoing the ctx change, this looks like a random diff. However, I agree with Andy that this is a very personal preference if I did let's say kept the ctx parameter I would like them on separate lines (unless enforced by linter). I don't think GoLang peepz cared much about this as it isn't built into gofmt. Also very subjective if this makes it easy to pick up go projects, or improves readability.

AndrewSisley

Test is looking better, but still needs more work before merging I think - sorry about the hassle

AndrewSisley · 2022-09-01T19:16:29Z

tests/integration/schema/input_type_test.go

+	"testing"
+)
+
+func TestInputTypeOfComplexSchema(t *testing.T) {


todo: It is better IMO, but it is still very very unclear what this test is trying to test, a few suggestions to hopefully bring the targets more into focus (I write these assuming you are testing the order elements):

The test name is still very unhelpful

I still see no reason for defining 3 types in the schema

Comments within the test might help explain why some stuff is (or isnt here)

Asserting on the _group field instead of the object/query-field is odd, what did it look like when you try assert on that directly? (If it is horrible, consider documenting this)

You could perhaps look at the contents of default_fields.go, doing something similar for input fields could help draw attention (and maintenance effort) away from the noise, hopefully leaving the order stuff defined explicitly within the test, and all else via consts/helper funcs

Will apply the todo and report back.

Cheers Shahzad - I'm also about to start writing a similar set of tests (probs tomorrow morning) for filter - give me a shout in the meantime if you end up making any changes to the shared code, or find a handy way of targeting order and I'll try keep mine consistent with yours

I did some changes according to your suggestions, in the latest commit.

AndrewSisley

Approved! Thanks for the testing effort here Shahzad, am surprised how nice you got it looking (what are your thoughts on it and the format?). Got a question or two, but looks good :)

tests/integration/schema/default_fields.go

AndrewSisley · 2022-09-02T14:12:21Z

tests/integration/schema/input_type_test.go

+				}
+			}
+		`,
+		ContainsData: map[string]any{


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)

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.

nice sounds good :)

AndrewSisley · 2022-09-02T14:12:43Z

tests/integration/schema/input_type_test.go

+												"ofType": any(nil),
+											},
+										},
+										// Without the relation type we won't have the following ordering type(s).


tests/integration/schema/input_type_test.go

AndrewSisley · 2022-09-02T14:14:03Z

tests/integration/schema/input_type_test.go

+					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.


shahzadlone · 2022-09-02T16:18:24Z

Approved! Thanks for the testing effort here Shahzad, am surprised how nice you got it looking (what are your thoughts on it and the format?). Got a question or two, but looks good :)

Thanks! I do like the format much better here as order is explicit and clear to see now. Addressed the questions.

- Resolves sourcenetwork#700 - Description: This was a bug in (`generate.go`).`genTypeOrderArgInput` where if a [`field.Type` + "OrderArg"] key didn't exist in the `typeMap` it would return `nil` and pass that in `InputObjectFieldConfig` to `Type`.

shahzadlone added bug Something isn't working area/schema Related to the schema system area/errors Related to the internal management or design of our error handling system area/cli Related to the CLI binary labels Aug 1, 2022

shahzadlone self-assigned this Aug 1, 2022

shahzadlone force-pushed the lone/fix/input-type-but-got-nil branch 2 times, most recently from a6f57b1 to 64de62d Compare August 1, 2022 13:43

fredcarle reviewed Aug 1, 2022

View reviewed changes

query/graphql/schema/generate.go Outdated Show resolved Hide resolved

tests/integration/query/one_to_one/with_order_test.go Outdated Show resolved Hide resolved

sourcenetwork deleted a comment from source-devs Aug 1, 2022

shahzadlone marked this pull request as draft August 1, 2022 23:17

shahzadlone force-pushed the lone/fix/input-type-but-got-nil branch 2 times, most recently from 4f55a47 to e6e7701 Compare August 8, 2022 13:36

AndrewSisley reviewed Aug 8, 2022

View reviewed changes

query/graphql/schema/generate.go Outdated Show resolved Hide resolved

AndrewSisley reviewed Aug 8, 2022

View reviewed changes

query/graphql/schema/generate.go Show resolved Hide resolved

shahzadlone force-pushed the lone/fix/input-type-but-got-nil branch from e6e7701 to fa834d7 Compare August 19, 2022 14:07

shahzadlone changed the title ~~fix: Handle order nil field type properly~~ fix: Handle order input field argument being nil Aug 19, 2022

sourcenetwork deleted a comment from source-devs Aug 19, 2022

shahzadlone marked this pull request as ready for review August 19, 2022 14:25

shahzadlone added the action/no-benchmark Skips the action that runs the benchmark. label Aug 19, 2022

shahzadlone requested a review from a team August 19, 2022 14:26

shahzadlone added this to the DefraDB v0.3.1 milestone Aug 19, 2022

sourcenetwork deleted a comment from source-devs Aug 19, 2022

AndrewSisley requested changes Aug 19, 2022

View reviewed changes

shahzadlone requested a review from AndrewSisley August 19, 2022 16:47

shahzadlone requested a review from AndrewSisley August 24, 2022 23:29

AndrewSisley reviewed Aug 25, 2022

View reviewed changes

AndrewSisley self-requested a review August 25, 2022 03:49

AndrewSisley reviewed Aug 26, 2022

View reviewed changes

query/graphql/schema/generate.go Outdated Show resolved Hide resolved

shahzadlone added 6 commits September 1, 2022 12:42

wip: Check for nil and also don't panic if target is not found, inste…

95c5b27

…ad log error.

PR: Stop execution according to Andy's suggestion.

a7cea05

PR: Fix the order's nil type error.

6f73d38

PR: Remove the logging and format error according to suggestion.

876315e

PR: Add schema test but it still doesn't have the order nil input type.

0a32f85

PR: Use ContainsData to trim the test down.

4fb13d7

shahzadlone force-pushed the lone/fix/input-type-but-got-nil branch from 855d618 to 4fb13d7 Compare September 1, 2022 16:44

shahzadlone requested a review from AndrewSisley September 1, 2022 16:45

PR: Remove ctx

f20d7fd

fredcarle approved these changes Sep 1, 2022

View reviewed changes

AndrewSisley requested changes Sep 1, 2022

View reviewed changes

shahzadlone force-pushed the lone/fix/input-type-but-got-nil branch 2 times, most recently from 3254315 to 82e788d Compare September 2, 2022 08:47

shahzadlone requested a review from AndrewSisley September 2, 2022 08:49

shahzadlone force-pushed the lone/fix/input-type-but-got-nil branch 3 times, most recently from 0c94263 to 517742c Compare September 2, 2022 09:21

AndrewSisley approved these changes Sep 2, 2022

View reviewed changes

PR: Adhere to code review.

9046555

shahzadlone force-pushed the lone/fix/input-type-but-got-nil branch from 517742c to 9046555 Compare September 2, 2022 16:00

shahzadlone merged commit 16c30b9 into develop Sep 2, 2022

shahzadlone deleted the lone/fix/input-type-but-got-nil branch September 2, 2022 16:19

shahzadlone mentioned this pull request Sep 12, 2022

fix: Handle filter input field argument being nil #787

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Handle order input field argument being nil #701

fix: Handle order input field argument being nil #701

shahzadlone commented Aug 1, 2022 •

edited

Loading

codecov bot commented Aug 1, 2022 •

edited

Loading

fredcarle left a comment

fredcarle commented Aug 1, 2022

shahzadlone commented Aug 1, 2022

AndrewSisley left a comment

AndrewSisley Aug 19, 2022

shahzadlone Aug 19, 2022

AndrewSisley Aug 19, 2022

AndrewSisley Aug 25, 2022

shahzadlone Aug 25, 2022

AndrewSisley Aug 25, 2022 •

edited

Loading

shahzadlone Aug 25, 2022

fredcarle left a comment

fredcarle Sep 1, 2022

AndrewSisley Sep 1, 2022

fredcarle Sep 1, 2022

AndrewSisley Sep 1, 2022

fredcarle Sep 1, 2022

AndrewSisley Sep 1, 2022

fredcarle Sep 1, 2022

shahzadlone Sep 1, 2022 •

edited

Loading

AndrewSisley left a comment

AndrewSisley Sep 1, 2022

shahzadlone Sep 1, 2022

AndrewSisley Sep 1, 2022

shahzadlone Sep 2, 2022

AndrewSisley left a comment

AndrewSisley Sep 2, 2022

shahzadlone Sep 2, 2022

AndrewSisley Sep 2, 2022

AndrewSisley Sep 2, 2022

AndrewSisley Sep 2, 2022

shahzadlone commented Sep 2, 2022

fix: Handle order input field argument being nil #701

fix: Handle order input field argument being nil #701

Conversation

shahzadlone commented Aug 1, 2022 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Aug 1, 2022 • edited Loading

Codecov Report

fredcarle left a comment

Choose a reason for hiding this comment

fredcarle commented Aug 1, 2022

shahzadlone commented Aug 1, 2022

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone commented Sep 2, 2022

shahzadlone commented Aug 1, 2022 •

edited

Loading

codecov bot commented Aug 1, 2022 •

edited

Loading

AndrewSisley Aug 25, 2022 •

edited

Loading

shahzadlone Sep 1, 2022 •

edited

Loading