fix(types): spread and embedding combination #569
Conversation
src/select-query-parser/result.ts
Outdated
| > = NodeType['type'] extends Ast.StarNode['type'] // If the selection is * | ||
| ? Row | ||
| : NodeType extends Ast.SpreadNode // If the selection is a ...spread | ||
| ? ProcessSpreadNode<Schema, Row, RelationName, Relationships, NodeType> | ||
| : NodeType extends Ast.FieldNode | ||
| ? ProcessFieldNode<Schema, Row, RelationName, Relationships, NodeType> | ||
| : NodeType['type'] extends Ast.SpreadNode['type'] // If the selection is a ...spread | ||
| ? ProcessSpreadNode<Schema, Row, RelationName, Relationships, Extract<NodeType, Ast.SpreadNode>> | ||
| : NodeType['type'] extends Ast.FieldNode['type'] | ||
| ? ProcessFieldNode<Schema, Row, RelationName, Relationships, Extract<NodeType, Ast.FieldNode>> |
There was a problem hiding this comment.
remark
User signaled from discord an issue with types inference in some cases when using spread operator: https://discord.com/channels/839993398554656828/1300910943634460682
This was due to the fact that NodeType extends Ast.SpreadNode will also returns true if the NodeType is actually a Ast.FieldNode even if their type discriminator are different.
I couldn't really understand exactly why forcing the check over the type property itself work while checking the whole type didn't (especially since it was working to discriminate between Star and Field types nodes) but it fixes this issue.
Note that I couldn't reproduce the issue with our own test database, but the test based on the user report I used for this can be found in this commit: 34c9c82
There was a problem hiding this comment.
Not sure if these changes are needed - tests pass without them, only the change in ProcessSpreadNodeResult is needed.
There was a problem hiding this comment.
Yeah this one is a nasty one, and as I mentioned I'm not completely sure of what's happening but I finally managed to get this reproduced in a test case here:
071185a#diff-db137d8db955b42a3a2e437ed10e24dcdd2e2754c5116a3b0259aebb681f167aR104-R120
Here is the debugging steps that I took to get to the fix. Basically the root cause of the error, is that, at some point, ProcessNode fail to infer the right node type I still don't understand why. But at some point a FieldNode is sent to the SpreadNode processing function. And here we can see that this is because this "Node extends Ast.SpreadNode" returns true.
Note that, inverting the conditional order in which we check the conditions (checking the FieldNode before SpreadNode) in ProcessNode would also fix this case, because the node actually result in true for BOTH types.
Where it becomes really really weird, is that reproducing the exact same node in a literal and testing the assertion does not give the same result:
Here the extends Ast.SpreadNode properly result in false as expected.
My solution is to make sure in ProcessNode that we discriminate over the type property of the node. Doing so the SpreadNode processing is no longer called over a FieldNode object:
But as a mentioned, on this one I'm not entirely sure about WHY the extends check doesn't behave as intended in this specific case. So I'm open to suggestions.
src/select-query-parser/result.ts
Outdated
| type ProcessSpreadNodeResult<Result> = Result extends Record<string, SelectQueryError<string>> | ||
| ? Result | ||
| : ExtractFirstProperty<Result> extends infer SpreadedObject |
There was a problem hiding this comment.
remark
Trying to reproduce on our database, I encountered a case with the spread operator where an error within the spread wasn't properly handled here was the previous result for such case:
| message_details: SelectQueryError<`Could not embed because more than one relationship was found for 'messages' and '${string}' you need to hint the column with messages!<columnName> ?`> | ||
| }[] | ||
| } | ||
| expectType<TypeEqual<typeof result, typeof expected>>(true) |
There was a problem hiding this comment.
Been meaning to ask in the parser rewrite PR, but why use ts-expect? This makes it hard to see what exactly went wrong in the CI logs, since the only information it gives is whether the types match or not.
There was a problem hiding this comment.
I had issue where expectType somehow swallowed some of what might be considered types errors (such as additional non expected fields, or union with "null" missing or being added without being caught).
|
🎉 This PR is included in version 1.17.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What kind of change does this PR introduce?
Fix some issues with result types when mixing embedding, spread operators and error returns.
What is the current behavior?
Please link any relevant issues here.
What is the new behavior?
Feel free to include screenshots if it includes visual changes.
Additional context
Add any other context or screenshots.