fix: Order by subtype without selecting the join child

[shahzadlone]

Relevant issue(s)

Resolves #588

Description

• Fix: Bug that caused panic when result was ordered by the subtype / relation / join field and there were no corresponding child fields requested (from that joined type).
• Test: Added test to ensure the query now returns valid results without panicing.
• Test: Added ton more tests according to code review suggestions.

Limitations

• Some tests were added to document panics which will be resolved outside this PR in Resolve panics caused by deeper nested orderby #833.
• Found bug Picks up previous iteration data on deeper one-one join #826 as part of this PR.
• Found bug One to one joins not linking properly #827 (fixed already).

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, Local

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

• Manjaro Wsl 2

[codecov]

Codecov Report

Merging #810 (a913ae1) into develop (cf046b7) will increase coverage by 0.03%.
The diff coverage is 83.33%.

❗ Current head a913ae1 differs from pull request most recent head ce2ab3a. Consider uploading reports for the commit ce2ab3a to get more accurate results

@@             Coverage Diff             @@
##           develop     #810      +/-   ##
===========================================
+ Coverage    59.64%   59.68%   +0.03%     
===========================================
  Files          155      155              
  Lines        17294    17300       +6     
===========================================
+ Hits         10315    10325      +10     
  Misses        6047     6047              
+ Partials       932      928       -4     

Impacted Files	Coverage Δ
query/graphql/mapper/mapper.go	85.09% <83.33%> (-0.13%)	⬇️
db/base/compare.go	54.23% <0.00%> (-6.78%)	⬇️
query/graphql/schema/manager.go	97.34% <0.00%> (-0.03%)	⬇️
query/graphql/schema/types/types.go	100.00% <0.00%> (ø)
query/graphql/planner/values.go	82.60% <0.00%> (+4.34%)	⬆️
connor/gt.go	76.19% <0.00%> (+4.76%)	⬆️
query/graphql/parser/commit.go	85.00% <0.00%> (+5.33%)	⬆️
query/graphql/mapper/targetable.go	75.00% <0.00%> (+8.33%)	⬆️

[AndrewSisley]

I think this needs more tests, sorry. Glad you are finding your way about the mapper file though :)

[fredcarle]

LGTM as long as Andy's concerns related to tests are addressed.

[jsimnz]

Was this only a problem with order (and some aggregate stuff andy mentioned in the original issue comment)? Was filter not effected by the same bug? IE:

query {
  user(filter: {author: {age: {_gt: 54}}}) {
    name
  }
}

[AndrewSisley]

Was this only a problem with order (and some aggregate stuff andy mentioned in the original issue comment)? Was filter not effected by the same bug? IE:

Filter was affected a while ago, maybe pre 0.3.0. Cheers for mentioning the aggregate sort too - it looks like this branch doesn't handle that.

new todo: Add tests for sort within aggregate e.g. _sum(childA: {field: childAField, order: {childB: {childBField: ASC}}})

[shahzadlone]

@AndrewSisley Have added some more tests according to your suggestion. Found a bug, so the implementation has changed slightly (mostly wasn't setting the child mapping's index properly).

I would pay attention to these three one-to-many cases you suggested and see if they are met (as these weren't correct queries I have tried my best to test as close as valid queries to these I could + their opposite direction order counterparts), if there is a way to properly add filter to these LMK:

These 6 can be found in tests/integration/query/one_to_many/with_sum_filter_order_test.go.

author(order: {author: {age: DESC}}) {
    name
    _sum(filter: {books: {rating: {_eq: 30}}})
}

author {
    name
    s1: _sum(filter: {books: {rating: {_eq: 30}}})
    s2: _sum(order: {books: {rating: DESC}}, limit: 2) // limit so the order affects the results
}

author {
    name
    _sum(filter: {books: {rating: {_eq: 30}}})
    books(order: {books: {rating: DESC}}, limit: 2) { // limit so the order affects the results
        name
    }
}

Moreover, #826 and #827 were found while testing for these.

And #833 is a follow-up after this when #826 is resolved.

[AndrewSisley]

Thanks Shahzad, testing is much better but I can still see no tests that cover what I am most concerned about (probably my fault for giving you broken test cases over discord). There seems to be no tests that the correct field will be used (prod code just takes the first). For example the below query with a nested one-many (publishers have many authors, authors have many books).

publishers {
    name
    topAuthor: authors(order: {books: rating DESC}, limit: 1) {
        name
    }
    // here we are checking that this 'books' is not the same books as used in the 'topAuthor' order
    worstAuthor: authors(order: {books: rating ASC}, limit: 1) {
        name
    }
}

and

publishers {
    name
    s1: _sum(authors: {field: age, order: {books: rating DESC}, limit: 2}) {
        name
    }
    s2: _sum(authors: {field: age, order: {books: rating ASC}, limit: 2}) {
        name
    }
}

and

publishers {
    name
    s1: _sum(authors: {field: age, order: {books: rating ASC}, limit: 2}) {
        name
    }
    topAuthor: authors(order: {books: rating DESC}, limit: 2) {
        name
    }
}

Aggregates are important to test separately as they have different codeblocks that deal with adding their dependencies. You could probably add more tests similar to these that include aggregate filters, to make sure that the filter isnt hiding any bugs in the order logic (by filtering out the expected results) - maybe something similar to the last example there, but one test where s1 is filtered (and no order), and one where topAuthor is filtered (and no order).

It is important that the order blocks contain child objects (or group, which would also be good to test), properties on the host object are unaffected by your prod code changes and so aren't worth worrying about.

Hope you can see what I'm driving at here, give me a shout if you have any questions.

[AndrewSisley]

.

[shahzadlone]

Thanks Shahzad, testing is much better but I can still see no tests that cover what I am most concerned about (probably my fault for giving you broken test cases over discord). There seems to be no tests that the correct field will be used (prod code just takes the first). For example the below query with a nested one-many (publishers have many authors, authors have many books).

...

Aggregates are important to test separately as they have different codeblocks that deal with adding their dependencies. You could probably add more tests similar to these that include aggregate filters, to make sure that the filter isnt hiding any bugs in the order logic (by filtering out the expected results) - maybe something similar to the last example there, but one test where s1 is filtered (and no order), and one where topAuthor is filtered (and no order).

It is important that the order blocks contain child objects (or group, which would also be good to test), properties on the host object are unaffected by your prod code changes and so aren't worth worrying about.

Hope you can see what I'm driving at here, give me a shout if you have any questions.

Thanks for these test cases, you are correct the prod code at the moment takes the first field if it's a related field (as I wasn't sure if it actually gets put into abc.def.zyx format or not (I didn't spot any tests, lmk if there are any). I wanted to make sure I could handle the splitting of the deep complex relation and so I started writing the deep 1-to-1-to-1 test which kept panicking and hence I found the source to be the #826 bug (although that was for 1-to-1).

Does the deeply nested one-many case work for normal query (with selections) before this PR? I can check this shortly after standup today if you aren't sure. If it does then happy to test these and make it work in this PR. If not I think we might be stepping outside of the scope of this PR, and combining multiple tasks that should be separate issues, which is why #833 was made.

#588 was only about this panicking (that aspect is resolved now, right? lmk if it isn't):

query {
  article  (order: {author: {age: ASC}}){
    name 
  }
}

[AndrewSisley]

#588 was only about this panicking (that aspect is resolved now, right? lmk if it isn't):

I am not a fan of replacing a panic with us returning incorrect values, I would probably prefer the panic

[shahzadlone]

#588 was only about this panicking (that aspect is resolved now, right? lmk if it isn't):

I am not a fan of replacing a panic with us returning incorrect values, I would probably prefer the panic

100% but where are we returning incorrect values for the test that panicked before? I feel like there are probably X different places that might be panicking or returning incorrect values in develop right now (hidden). We can tackle all those in separate issues rather than one ticket.

However as mentioned in the previous message I will see if the deeply nested one to many case works after #827 was merged and if I can produce a normal example of order being used deeply like this: publishers {authors(order: {books: rating DESC})...} in which case happy to add more tests. But not if I have to debug another bug to add more test which can be a separate issue.

[AndrewSisley]

100% but where are we returning incorrect values for the test that panicked before?

The cases I provided will likely be providing incorrect values, as they may be basing their results off the wrong underlying field (e.g. returning the top author, instead of the bottom). For the use-cases I have had in the past (pre-source), a panic would be much much prefered/cheaper than incorrect results.

[shahzadlone]

The cases I provided will likely be providing incorrect values, as they may be basing their results off the wrong underlying field (e.g. returning the top author, instead of the bottom).

If those incorrect values aren't other panics or non-related errors, then as I mentioned I am happy to incorporate them.

For the use-cases I have had in the past (pre-source), a panic would be much much prefered/cheaper than incorrect results.

No contradiction here. I am quite okay with this behavior especially when there might be a chance of incorrect write.

[shahzadlone]

As discussed on discord (@AndrewSisley and thanks for helping me navigate these order tests) I have modified your original requested tests from these:

publishers {
    name
    topAuthor: authors(order: {books: rating DESC}, limit: 1) {
        name
    }
    // here we are checking that this 'books' is not the same books as used in the 'topAuthor' order
    worstAuthor: authors(order: {books: rating ASC}, limit: 1) {
        name
    }
}

publishers {
    name
    s1: _sum(authors: {field: age, order: {books: rating DESC}, limit: 2}) {
        name
    }
    s2: _sum(authors: {field: age, order: {books: rating ASC}, limit: 2}) {
        name
    }
}

publishers {
    name
    s1: _sum(authors: {field: age, order: {books: rating ASC}, limit: 2}) {
        name
    }
    topAuthor: authors(order: {books: rating DESC}, limit: 2) {
        name
    }
}

To these 3 counterparts:

// Located in: tests/integration/query/one_to_many_to_one/with_order_limit_test.go.
query {
    Author {
		name
		NewestPublishersBook: book(order: {publisher: {yearOpened: DESC}}, limit: 1) {
			name
		}
		OldestPublishersBook: book(order: {publisher: {yearOpened: ASC}}, limit: 1) {
			name
		}
	}
}

// Located in: tests/integration/query/one_to_many_to_one/with_sum_order_limit_test.go
query {
    Author {
	name
	    s1: _sum(book: {field: rating, order: {publisher: {yearOpened: DESC}}, limit: 2})
	    s2: _sum(book: {field: rating, order: {publisher: {yearOpened: ASC}}, limit: 2})
	}
}

// Located in: tests/integration/query/one_to_many_to_one/with_sum_order_limit_test.go
query {
    Author {
	name
	    s1: _sum(book: {field: rating, order: {publisher: {yearOpened: DESC}}, limit: 2})
	    NewestPublishersBook: book(order: {publisher: {yearOpened: ASC}}, limit: 2) {
		    name
	    }
	}
}

The findings were that these all panic, hence No Incorrect Values are seen.

As these test cases panicked before this PR, and after changes introduced in this PR, this would fall under what I described previously as:

X different places that might be panicking or returning incorrect values in develop right now (hidden)

In other words, separate issues to be tackled after this PR.

If you have any other hunches or suggestions to create incorrect values as you mentioned perhaps using inner groupby fields, I am happy to explore those, let me know @AndrewSisley .

[AndrewSisley]

I have some more todos for you sorry, a couple on the tests, and then I reviewed the prod code which I had mostly skipped before.

[AndrewSisley]

Cheers Shahzad - bit of a tricky area to give someone new to mapper maybe at the end of a release. Thanks for all the effort RE testing, approving now assuming you sort out the prod code/tested-panics soon after the release :)

[shahzadlone]

Cheers Shahzad - bit of a tricky area to give someone new to mapper maybe at the end of a release. Thanks for all the effort RE testing, approving now assuming you sort out the prod code/tested-panics soon after the release :)

Cheers @AndrewSisley appreciate you bearing with me and helping me navigate the mapper, also for making me explore these aggregate and orderby test cases to get a better idea of what's happening under the hood. Indeed will try my best to resolve these panics in v0.4. There is an umbrella ticket (#833) that can expand into multiple tickets when these are investigated a bit more.