Fixed #35336 -- Addressed crash when adding a GeneratedField with % literals. #18027
Conversation
| if params: | ||
| expression_sql = expression_sql % tuple(self.quote_value(p) for p in params) | ||
| return f"GENERATED ALWAYS AS ({expression_sql}) {persistency_sql}" | ||
| return f"GENERATED ALWAYS AS ({expression_sql}) {persistency_sql}", params |
There was a problem hiding this comment.
The crux of the issue here is that
- This part was never
%escaped (%->%%) which mean that things were working properly during model creation unless the model included a database default which resulted intable_sqlreturningparamsand then interpolation kicked (see theparams or Noneincreate_model). As foradd_fieldit systematically crashed because a list was always passed toexecute(params)which triggered interpolation which is what is what was reported on ticket-35336. PostgresSome backends simply don't support parametrized DD so we should likely have a generic feature flag for it and do thequote_valuedance when it's not the case in a single location. That would require changing theConstraintinterfaces though as it currently return already interpolated SQL under some circumstances.
|
This one is going to be a fun one to solve without being too invasive. I'll revisit tomorrow. |
Thank you Simon for working on this. I'm currently planning on delaying the next release for a few days, even a week if necessary, for two reasons:
Let me know what you think! |
|
Sounds good, I'll try to find another solution that isn't as invasive for the backport with further improvement separated to target |
charettes
force-pushed
the
ticket-35336
branch
2 times, most recently
from
cf98156
to
b37f9db
Compare
|
buildbot test on oracle. |
|
It has already been tested on Oracle @nessita I just happened to delete the comment. You should see the successful status the checks. |
I see it now, thank you! |
|
Thank you Simon! |
|
@sarahboyce I just gave it a try, both the example in my ticket and our actual production code worked without issue! |
…iterals. A longer term solution is likely to have a better separation of parametrized DDL altogether to handle checks, constraints, defaults, and generated fields but such a change would require a significant refactor that isn't suitable for a backport. Thanks Adrian Garcia for the report.
There was a problem hiding this comment.
@charettes Thank you for putting together this PR!
Do you think we should Refs the related(ish) tickets in the commit message? I'm thinking about the three you mentioned (ticket-30408, ticket-34553, ticket-32369).
|
@nessita I think it's fine to don't ref them since while they are related to the same fundamental problem they are for different features. |
ticket-35336
A longer term solution is likely to have a better separation of parametrized DDL altogether to handle checks, constraints, defaults, and generated fields but such a change would require a significant refactor that isn't suitable for a backport.
Thanks Adrian Garcia for the report.