Skip to content

Conversation

@enobayram
Copy link
Contributor

This PR makes the sqlalchemy.escape_params function more specific about which : characters it escapes. I believe this is a safe change, because we're using the same regex that the sqlalchemy library uses to find the parameters.

I believe this change makes the output more pleasant to look at in more cases, but my main motivation for making this change is how the old behavior of escape_params interacted with postgresql.Views: When sqlalchemy-declarative-extensions encounters a changed Postgres view, it creates a temporary VIEW in Postgres with the new View expression and reads the body of the VIEW it back from Postgres to construct the Python literal that's used to assemble the op.execute(...) statement for the alembic migration. The trouble is that Postgres augments every literal expression in the View definition with explicit :: TYPE casts. The old behavior of escape_params then replaced all the :: occurrences with \:\:s unnecessarily.

The proliferation of these \:\: occurrences makes the resulting migration harder to read, but aside from that aesthetic concern, it also interacts poorly with how the op.execute() statements are rendered:

[f'op.execute("""{command}""")' for command in commands]

Which causes the escaped \: fragments to appear directly in the generated migration file and then Pyright complains that \: is not a valid escape sequence. Ideally we'd emit the op.execute statements via an expression like f'op.execute({repr(command)})' and let the repr machinery do all the escaping, but I'm not suggesting that change since repr(<string>) produces very ugly one-line strings that escape newlines as well.

TLDR; \:\: fragments we're avoiding with this PR are unnecessary and bad for readability, but they also interact badly with the way we're emitting alembic migration statements.

@codecov
Copy link

codecov bot commented Aug 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.81%. Comparing base (4ea6f90) to head (8d990b6).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #122   +/-   ##
=======================================
  Coverage   89.81%   89.81%           
=======================================
  Files          86       86           
  Lines        4054     4056    +2     
  Branches      841      841           
=======================================
+ Hits         3641     3643    +2     
  Misses        336      336           
  Partials       77       77           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@DanCardin
Copy link
Owner

Do you mind adding a simple View test with a cast, asserting that it outputs your expected syntax? Mostly just so this doesn't get inadvertently reverted

@enobayram
Copy link
Contributor Author

Makes perfect sense, I'll do that.

@enobayram
Copy link
Contributor Author

@DanCardin I just extended the test_escape_bindparam to demonstrate that view round-tripping injects ::s to literals and that bindparams escaping now leaves those ::'s intact.

@enobayram
Copy link
Contributor Author

Umm, I really don't understand what happened with the last git pull --rebase 🤷 These cross-repo pull requests behave very strangely. I'll try to fix it.

@enobayram enobayram force-pushed the narrow-down-sqlalchemy-parameter-escapes branch from fedea97 to 0d8bf83 Compare August 18, 2025 16:46
@DanCardin DanCardin force-pushed the narrow-down-sqlalchemy-parameter-escapes branch from 0d8bf83 to 8d990b6 Compare August 18, 2025 20:01
@DanCardin DanCardin merged commit 66ff703 into DanCardin:main Aug 19, 2025
19 checks passed
@enobayram
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants