Support overloaded computed column functions - #2997
Conversation
馃 Changeset detectedLatest commit: 740d7f9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
benjie
left a comment
There was a problem hiding this comment.
Thanks for the contribution; here's some guidance on how to help move this forward. I think we can end up with a much smaller PR that still achieves your goals 馃
|
I have pushed changes to address a number of comments. It also moves towards the consistent prefix behavior, but that does break existing functions (see the thread). In general I'm strictly against breaking changes, however since we are still pre 5.0 stable and since it makes behavior more consistent across the board, I think it may be a good direction. The breaking change will probably have a very limited impact since it's a rare feature? |
|
As a result of these changes, the I've made the test succeed now. But if we think this is the good way forward then it may make sense to rename |
c731613 to
090c885
Compare
benjie
left a comment
There was a problem hiding this comment.
Thanks for the patience whilst we were working through the V5 release!
88d18cc to
e8197a2
Compare
benjie
left a comment
There was a problem hiding this comment.
Not the simplest of problems to solve 馃槄
Let me know what you think of the ideas below.
02b3e40 to
56bb459
Compare
By default, PostGraphile skips overloaded functions because their derived resource names would clash. This is now detected by comparing inflected resource names, and a warning is logged when the skipped overloads look like computed columns targeting different tables. The new preset factors the input argument types into function resource names (code(pets) becomes code__pets) so that overloads receive distinct names. This enables overloaded computed column functions targeting different tables, including across schemas.
56bb459 to
740d7f9
Compare
|
Hi @benjie, I re-did the feature based on your feedback and based on the latest main (this branch was 474 commit behind 馃槄 ). AI Disclaimer: I used Claude Fable 5 while making these changes. |
Description
Support overloaded computed column functions, where the first argument is a table.
Fixes #2972
Adds a test for this specific scenario.
I deliberately did not modify the PgV4BehaviorPlugin as I suspect users will not appreciate a sudden change in behavior. So this still requires smart tags, just like computed column functions that are NOT overloaded.
AI disclaimer: I used Claude to investigate the codebase and implement parts of this PR, but am actively trying to avoid slop. I personally read all the changes line-by-line, except for the snapshots, and validate the changes to the best of my ability.
Regardless of the use of AI; this is my first PR on this project and first experience with the inner workings of graphile. Although I've been a user for a couple of years now and wrote some custom plugins, diving into a large project like this one always a little overwhelming at first. So please consider carefully and do not hesitate to just close my PR if it's bad, I'll understand.
About the snapshots, I'm not sure if all those changes are correct, this is what recreating the snapshots resulted in but I wonder if it's supposed to do that? It feels like the diff is way too large... I am not familiar enough (yet) with this project to asses this; help needed.
Since procedures need a unique name it probably makes sense to add the table the overloaded procedure is targeting.
For
FUNCTION code(finance.invoices)we could go with either:code_finance_invoicesorfinance_invoices_codeI decided to go with the first since it has the same order of identifiers as the function definition (
a(b.c)>a_b_c), and less likely to be confused/collide with thefinance.invoices_code(..)approach of v4.Performance impact
Should be relatively low. I used manual loops instead of functional to keep allocations low.
Security impact
unknown
Checklist
yarn lint:fixpasses.yarn testpasses.RELEASE_NOTES.mdfile (if one exists).