fix: Handle returned error in select.go

[AndrewSisley]

Relevant issue(s)

Resolves #1264

Description

Handles a previously unhandled returned error path in select.go.

An extra if-else was needed to be added, as commit queries in develop-branch actually depend on the silent ignoring of returned errors.

[codecov]

Codecov Report

Merging #1329 (b6f14f1) into develop (8ea58ce) will increase coverage by 0.00%.
The diff coverage is 72.72%.

@@           Coverage Diff            @@
##           develop    #1329   +/-   ##
========================================
  Coverage    70.55%   70.56%           
========================================
  Files          184      184           
  Lines        17784    17792    +8     
========================================
+ Hits         12548    12555    +7     
  Misses        4280     4280           
- Partials       956      957    +1     

Impacted Files	Coverage Δ
planner/select.go	79.22% <72.72%> (-1.58%)	⬇️

... and 5 files with indirect coverage changes

[jsimnz]

I'm confused by this additional if case, it has a bunch of conditions but is a noop?

[AndrewSisley]

I'm confused by this additional if case, it's has a bunch of conditions but is a noop?

Sorry - I failed to write why that was needed in the description. Basically currently in develop at the moment (and presumably the last few releases), under those conditions we are erroring here (in the else) but ignoring the errors raised (along with any actually useful errors that should be reported).

We cant just return the error (the change to the else), as commit queries with links would stop working, as instead of ignoring the errors we'd report them.

[jsimnz]

This might be problematic, as youve written the if case to be super specific to how the commit.links field is represented (name is links, within a commits select, and not within a collection name. tangentially, its not even clear why you need to check that f.Collection is empty, if youre already checking that its within a commit query).

In the future, as we add other kinds of fields with selection sets (objects) that are system reserved (like links), we'll need more overly specific if branches.

This seems like its caused by the "catchall" else branch, which says if we get here, then the only possible type it could be is a user defined sub type, and we need a typeJoin on it.

So instead of using the complicated noop if branch (which will need to get expanded more and more), why not replace the else with another else if that checks if its a user defined sub-type?

[AndrewSisley]

So instead of using the complicated noop if branch (which will need to get expanded more and more), why not replace the else with another else if that checks if its a user defined sub-type?

How would you write that code? You cant use f.CollectionName, as that inherits from further up the select-chain (required for stuff like aggregates). You also cant use f.Name, as otherwise it will clash with any user-defined links types.

The if-else clause shouldnt get more complex. If other cases get added prior to a rework of this whole function/flow, it should really be another if-else block specific to that case IMO.

I'm also quite adverse to making this too complex right now, atm we are hopefully releasing today, and I want to avoid doing anything too complicated (i.e. risky) in the hours remaining.

[jsimnz]

KK, we can tackle a better else if condition in 0.5.1

[jsimnz]

LGTM

[AndrewSisley]

KK, we can tackle a better else if condition in 0.5.1

I do worry that doing it properly will not be straightforward - I really dont like the select initSource/Fields funcs, and see it almost as a symptom of deficiencies in other parts of the codebase (initFields in particular looks like it should at the very least be handled in planner.go).

At a lower-level I see the need for such a complex if-else as a sign that the mapper model needs improvement.