fix: Rename aggregate gql types #638

AndrewSisley · 2022-07-15T15:37:12Z

Relevant issue(s)

Description

Adds schema tests for aggregates. Orders schema input type and args (gql-go version update). Fixes a bunch of issues in generate.go. Renames aggregate gql types.

Suggest reviewing commit by commit, there are a few semi-related changes in this PR (each should have it's own commit+description).

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?

(replace) Describe the tests performed to verify the changes. Provide instructions to reproduce them.

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

Debian Linux

codecov · 2022-07-15T15:40:58Z

Codecov Report

Merging #638 (43eab8e) into develop (f1beb0a) will decrease coverage by 0.06%.
The diff coverage is 83.92%.

@@             Coverage Diff             @@
##           develop     #638      +/-   ##
===========================================
- Coverage    57.15%   57.09%   -0.07%     
===========================================
  Files          122      122              
  Lines        14642    14646       +4     
===========================================
- Hits          8369     8362       -7     
- Misses        5560     5567       +7     
- Partials       713      717       +4

Impacted Files	Coverage Δ
query/graphql/schema/generate.go	`84.41% <83.92%> (-0.82%)`	⬇️
query/graphql/schema/relations.go	`60.82% <0.00%> (-1.55%)`	⬇️

fredcarle

Just want to confirm, the ContainsData is to make sure the data contains what is defined but not necessarily matching perfectly?

fredcarle · 2022-07-15T20:12:50Z

query/graphql/schema/generate.go

-				"Failed to append type while generating query type defs from an AST",
-				err,
-			)
+			return nil, err


praise: I like this much better.

suggestion (non-blocking): you could keep the message if you think it's relevant.

return nil, fmt.Errorf("failed to append type while generating query type defs from an AST: %w", err)

Nice, thanks - was hiding a few errors that got fixed in this PR lol. I don't recall the extra text being particularly useful

Also don't forget the question I had in my main comment :)

shahzadlone · 2022-07-15T20:55:17Z

tests/integration/schema/utils.go

 		assert.Equal(t, testCase.ExpectedData, resultantData)
 	}

-	for k, result := range resultantData {
-		assert.Equal(t, testCase.ExpectedData[k], result)
+	if len(testCase.ExpectedData) == 0 && len(testCase.ContainsData) > 0 {


thought: perhaps else if?

It would make sense but these two can never be true at the same time so it won't change much other than visual flow.

Fair point, just feel like with an else if you don't even have to bother reading the checks to worry if it will go inside the other condition (by definition of else if).

Whilst the logic here fits an if-else, they are different concepts IMO and I prefer to keep the independent

shahzadlone · 2022-07-15T21:11:01Z

query/graphql/schema/generate.go

-						)
-						if err != nil {
-							log.ErrorE(ctx, "Error while registering single relation", err)
+						if !gql.IsLeafType(ltype) {


suggestion: limit scope of ltype.

Suggested change

if !gql.IsLeafType(ltype) {

if ltype := subobj.OfType; !gql.IsLeafType(ltype) {

nitpick: Also would like to rename ltype to lType (similar to fType).

I also think this makes sense.

This is out of scope, and whilst it would be quick to change, I do not wish to encourage further requests of this nature in PRs

EDIT: I missed the scope-limit request of your comment, mine reply was to the rename only. RE scope I dont think it matters much at all, and isn't worth tweaking the branch for this alone. (Plus if I edit this, it will be to remove the variable entirely, not to just shrink it's scope)

IMO is a fairly quick thing and better code. Non-blocking though so giving approval.

shahzadlone · 2022-07-15T21:16:19Z

query/graphql/schema/generate.go

+	return fmt.Sprintf("%s__%s__%s", hostName, fieldName, "CountSelector")
+}
+
+func genObjectCountName(hostName string) string {
+	return fmt.Sprintf("%s__%s", hostName, "CountSelector")


question: does "CountSelector" need to be commonly shared? if so perhaps a const.

It is only used in these two functions, and should never have need to be referenced elsewhere (e.g. in planner/etc). Leaving as is

AndrewSisley · 2022-07-18T13:13:08Z

Just want to confirm, the ContainsData is to make sure the data contains what is defined but not necessarily matching perfectly?

The stuff in ContainsData has to match exactly, but it does not need to include everything (so e.g. the tests dont have to specify every field on an object)

shahzadlone

LGTM, would still like the changes I requested (but non-blocking).

It errored, logged and continued

Will error

New version yields input fields and args in a deterministic order

Allows asserting that specific items exist, without forcing the tests to define all expected values (costing dev time and coupling the test to unrelated functionality).

Double underscore added as a seperator (reduces risk of name collisions). Removed naming suffix differences between inline-array and other fields. New suffix chosen. Casing corrected (left as user-defined, Title casing was lower casing inner values, and reduces risk of collisions (e.g. users vs Users)).

* Dont register inline arrays as relationships It errored, logged and continued * Dont try and generate filter blocks for inline arrays * Don't generate filters for objects with no fields Will error * Dont append filter base arg if already exists * Check filter is permitted before adding it * Return error instead of logging and continuing * Update gql-go version New version yields input fields and args in a deterministic order * Go mod tidy * Add contains capactiy to schema tests Allows asserting that specific items exist, without forcing the tests to define all expected values (costing dev time and coupling the test to unrelated functionality). * Add top-level aggregate schema tests * Add simple object aggregate schema tests * Add inline array aggregate schema tests * Rename aggregate selectors Double underscore added as a seperator (reduces risk of name collisions). Removed naming suffix differences between inline-array and other fields. New suffix chosen. Casing corrected (left as user-defined, Title casing was lower casing inner values, and reduces risk of collisions (e.g. users vs Users)).

AndrewSisley added bug Something isn't working documentation Improvements or additions to documentation area/schema Related to the schema system code quality Related to improving code quality action/no-benchmark Skips the action that runs the benchmark. labels Jul 15, 2022

AndrewSisley requested a review from a team July 15, 2022 15:37

AndrewSisley self-assigned this Jul 15, 2022

AndrewSisley linked an issue Jul 15, 2022 that may be closed by this pull request

"Cannot update a relation that is already finalized" errors when adding schema with relations on blank DB #574

Closed

AndrewSisley force-pushed the sisley/fix/I377-gql-type-names branch from 56c643c to 72e4059 Compare July 15, 2022 15:44

fredcarle reviewed Jul 15, 2022

View reviewed changes

shahzadlone requested changes Jul 15, 2022

View reviewed changes

shahzadlone reviewed Jul 15, 2022

View reviewed changes

AndrewSisley requested review from shahzadlone and fredcarle July 18, 2022 13:19

shahzadlone approved these changes Jul 18, 2022

View reviewed changes

AndrewSisley added 13 commits July 18, 2022 09:45

Dont register inline arrays as relationships

dfe8657

It errored, logged and continued

Dont try and generate filter blocks for inline arrays

d1128ab

Don't generate filters for objects with no fields

727421f

Will error

Dont append filter base arg if already exists

d10becd

Check filter is permitted before adding it

180e30f

Return error instead of logging and continuing

8266750

Update gql-go version

8a4190c

New version yields input fields and args in a deterministic order

Go mod tidy

90dd229

Add contains capactiy to schema tests

79b14f3

Allows asserting that specific items exist, without forcing the tests to define all expected values (costing dev time and coupling the test to unrelated functionality).

Add top-level aggregate schema tests

b52fd08

Add simple object aggregate schema tests

22d1dfe

Add inline array aggregate schema tests

6661898

AndrewSisley force-pushed the sisley/fix/I377-gql-type-names branch from 72e4059 to 43eab8e Compare July 18, 2022 13:45

AndrewSisley merged commit ddab2cc into develop Jul 18, 2022

AndrewSisley deleted the sisley/fix/I377-gql-type-names branch July 18, 2022 13:57

shahzadlone mentioned this pull request Jul 31, 2022

fix: Return without an error if relation is finalized #698

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Rename aggregate gql types #638

fix: Rename aggregate gql types #638

AndrewSisley commented Jul 15, 2022

codecov bot commented Jul 15, 2022 •

edited

Loading

fredcarle left a comment

fredcarle Jul 15, 2022

AndrewSisley Jul 15, 2022

fredcarle Jul 17, 2022

shahzadlone Jul 15, 2022

fredcarle Jul 17, 2022

shahzadlone Jul 17, 2022

AndrewSisley Jul 18, 2022

shahzadlone Jul 15, 2022

fredcarle Jul 17, 2022

AndrewSisley Jul 18, 2022 •

edited

Loading

shahzadlone Jul 18, 2022

shahzadlone Jul 15, 2022

AndrewSisley Jul 18, 2022

AndrewSisley commented Jul 18, 2022

shahzadlone left a comment

	if !gql.IsLeafType(ltype) {
	if ltype := subobj.OfType; !gql.IsLeafType(ltype) {

fix: Rename aggregate gql types #638

fix: Rename aggregate gql types #638

Conversation

AndrewSisley commented Jul 15, 2022

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Jul 15, 2022 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Jul 18, 2022 • edited Loading

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 commented Jul 18, 2022

shahzadlone left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 15, 2022 •

edited

Loading

AndrewSisley Jul 18, 2022 •

edited

Loading