Repository navigation
Conversation
Check constraint SQL (with its base filter) and custom statement SQL were
interpolated as is into `"""` heredocs in the generated migration, so Elixir
reinterpreted backslash escapes and `#{` before the SQL reached Postgres:
`\d` became U+007F and `#{x}` was evaluated. An identity's unique index on a
resource with `base_filter_sql` wrote the filter between plain double quotes,
so a quoted identifier produced an invalid migration.
The heredoc bodies are now escaped (`\`, `#{` and `"""`), and the unique
index filter is written with `inspect/1`. The existing check constraint test
compared the migration's text, which matched even though Elixir read a
different string from it; it now compares the string Elixir reads.
Contributor
Author
|
@zachdaniel A heads-up on CI: none of the failures on this PR come from the change. Each one fails the same way on
Happy to open a separate PR for any of these if that helps. |
Contributor
Author
|
Update: with |
Contributor
|
See my coment on the issue for how this should be handled. 🚀 Thank you for your contribution! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #876.
The migration generator puts SQL from a resource into the generated migration's Elixir source without escaping it, so Elixir reads a different string from the migration than the one the resource declares:
check_constraintcheck:(alone, or combined with the base filter) goes into a"""heredoc inAddCheckConstraint.up/1andRemoveCheckConstraint.down/1.check: ~S[code ~ '^\d{4}$']reaches Postgres with U+007F in place of\d, so the constraint refuses the rows it was written to accept, and#{x}in the SQL is evaluated. The snapshot holds the declared SQL, somix ash.codegen --checkreports nothing pending.custom_statementswithoutcode?: truego into anexecute("""...""")heredoc inAddCustomStatement, with the same effect.base_filter_sqlis written aswhere: "(<base_filter_sql>)"inAddUniqueIndex.up/1. A quoted identifier such as"archived" = falseends the string early, andmix ash.codegenraises aSyntaxErrorwhile formatting the file.#578 moved check constraints into heredocs so that double quotes survive, which they now do, but a heredoc still processes backslash escapes and interpolation. The existing test for it compares the migration's text, which matches the declaration even though Elixir reads something else from it: the check in that test,
~S[title ~= '("\"\\"\\\"\\\\"\\\\\")'], is read back astitle ~= '(""\"\"\\"\\")'.This adds
Helper.escape_heredoc/1, which escapes\,#{and""", and applies it to the four check constraint heredocs and the two custom statement heredocs. The layout of generated migrations is unchanged, and a heredoc whose SQL contains none of those three sequences is generated exactly as before. The unique index filter is written withinspect(base_filter, printable_limit: :infinity); the limit is there becauseinspect/1otherwise truncates strings over 4096 bytes.custom_indexeswhere:already goes throughinspect/1(viaoption/2) and was not affected.One behaviour change to be aware of: anyone who worked around this by doubling backslashes in their resource (as suggested on #576) will get the doubled backslashes in the next migration generated for that constraint or statement. Migrations that were already generated are untouched, and since snapshots do not change, this does not generate any new migrations by itself.
Tests: a new
describe "raw SQL in generated migrations"block intest/migration_generator_test.exscovers a check constraint with\d,#{x}and"""(and itsdown), a check combined with a base filter, a custom statement'supanddown, and the identity under a quotedbase_filter_sql. The new helpers parse the generated migration withCode.string_to_quoted!/1and compare the strings Elixir reads, and the existing quote-heavy check test now does the same. Onmainall four new tests and the updated one fail; with the change,test/migration_generator_test.exspasses (119 tests).Full suite on
main(63cf7ca) and on this branch, PostgreSQL 18.6, Elixir 1.20.1 / OTP 29: 1039 passed onmain, 1043 on this branch (the four new tests), with the same 81 failures on both: 77 inAshPostgres.TemporalTest(unique_violationonsubscription_pkey) and the four sort tests inJoinSubquerySortTest,UniqAggregateSortTestandFromManyAggregateSortTest.mix format --check-formattedreports onlytest/support/temporal/domain.ex, which is unformatted onmainand untouched here;mix credo --strictandmix sobeloware clean. I did not get a localmix dialyzerresult: building the dependency PLT ran out of memory on this machine (past 28 GB) before it reached this project's code, so CI's run is the check for that.Found by an AI agent working with a human while generating migrations for their own application. The reproduction outside this repo is https://github.com/grempe/ash-fuzz-repros/blob/main/test/ash_postgres/migration_sql_not_escaped_test.exs, which generates and runs the migration against Postgres; its four failing cases pass against this branch.
Contributor checklist
Leave anything that you believe does not apply unchecked.