fix: compare workbook names as strings for postgres - #1253
Open
chdecultot wants to merge 1 commit into
Open
Conversation
`Insights Workbook` is autoincrement, so its `name` is an int, while every column that references it is varchar(140): `View Log.reference_name` and `DocShare.share_name` (Dynamic Link) and the `workbook` Link fields. The pypika-based list query binds filter values as-is (the legacy db_query path used to cstr them), so postgres gets `"reference_name" IN (3,2,1)` and fails with `operator does not exist: character varying = integer`. Stringify the name everywhere it reaches a filter or an insert. Link columns need the controllers to do it: `_validate_links` re-sets the link to the fetched `name` (an int) before `validate`/`before_save` run, so the client sending "3" isn't enough. Also cast the bigint side of the four workbook joins in the permission queries. Admins short-circuit those, which is why only the View Log error surfaced; without the cast a non-admin can't list workbooks at all. MariaDB coerces on its own, so the cast is postgres-only and plans there are unchanged. Fixes frappe#1193 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Tick the box to add this pull request to the merge queue (same as
|
Confidence Score: 5/5This looks safe to merge.
Reviews (1): Last reviewed commit: "fix: compare workbook names as strings f..." | Re-trigger Greptile |
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.
Insights Workbookis autoincrement, so itsnameis an int, while every column that references it is varchar(140):View Log.reference_nameandDocShare.share_name(Dynamic Link) and theworkbookLink fields. The pypika-based list query binds filter values as-is (the legacy db_query path used to cstr them), so postgres gets"reference_name" IN (3,2,1)and fails withoperator does not exist: character varying = integer.Stringify the name everywhere it reaches a filter or an insert. Link columns need the controllers to do it:
_validate_linksre-sets the link to the fetchedname(an int) beforevalidate/before_saverun, so the client sending "3" isn't enough.Also cast the bigint side of the four workbook joins in the permission queries. Admins short-circuit those, which is why only the View Log error surfaced; without the cast a non-admin can't list workbooks at all. MariaDB coerces on its own, so the cast is postgres-only and plans there are unchanged.
Fixes #1193