#129 - throw an error when selectExactlyOne retrieves 0 rows when used in lateral#189
#129 - throw an error when selectExactlyOne retrieves 0 rows when used in lateral#189lukeramsden wants to merge 6 commits intojawj:masterfrom
Conversation
| const copy = new SQLFragment<RunResult, Constraint>(literals, expressions); | ||
|
|
||
| // copy across relevant flags that aren’t part of the constructor | ||
| if (this.expectExactlyOne !== undefined) (copy as any).expectExactlyOne = this.expectExactlyOne; |
There was a problem hiding this comment.
Can we not instead add expectExactlyOne to the set of properties copied across via Object.assign, just below?
|
|
||
| query.runResultTransform = | ||
|
|
||
| // ---- runtime transform ------------------------------------------------- |
There was a problem hiding this comment.
Sorry, only a stylistic preference, but can we cut all the hyphens? :)
|
|
||
| // If there are any lateral subqueries that themselves expected exactly one | ||
| // row, we need to verify those expectations here (their own transforms will | ||
| // not run, because only the outermost query's transform executes). |
There was a problem hiding this comment.
Just wondering if it would be more elegant to have the nested transforms run, rather than doing a separate recurrsive transform from the top down? Would that make sense here?
|
Thanks so much for working on this PR — and the beginning of a test suite! And sorry it's taken me a while to look at it. I haven't had time for any deep thinking on this so far, but I have added a few comments, as you'll have seen. My main worry is that it just seems like quite a lot of code and complication to support this. Is there any possible way this could be simplified? Is there even some simple change we could make to the SQL to have Postgres do the checking?? |
|
Busy today but will have a think about what you said + your comments and get back to you! Agree on all of what you're saying though. |
…it does when used standalone See PR in upstream for more context: jawj/zapatos#189
2 participants
Has the commits from #188 too but see the final commit for the actual change