Skip to content

fix: Return field names from updateWhere and updateById for explicit columns - #5913

Merged
marcelomendoncasoares merged 1 commit into
serverpod:mainfrom
vchrisb:fix/update-where-explicit-column-returning
Oct 27, 2026
Merged

fix: Return field names from updateWhere and updateById for explicit columns#5913
marcelomendoncasoares merged 1 commit into
serverpod:mainfrom
vchrisb:fix/update-where-explicit-column-returning

Conversation

@vchrisb

@vchrisb vchrisb commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

updateWhere and updateById fail to deserialize their returned rows on Postgres when a model field has a column= override.

Both methods used RETURNING *, so the rows came back with database column names (e.g. auth_user_id), but the generated fromJson expects field names (authUserId). The UPDATE itself succeeded, but the call then threw. Outside a transaction this left the row modified while the caller saw a failure. insert and update already avoid this by using buildReturningClause.

This PR uses buildReturningClause in all three places that returned rows:

  • updateById: the returned columns are renamed to field names.
  • updateWhere without limit, offset or ordering: same change.
  • updateWhere with limit, offset or ordering: the outer SELECT from the updated CTE now renames the columns. The ORDER BY now qualifies columns with the CTE name ("updated"."col"), so it still sorts by the database columns rather than the renamed output columns.

The returning clause uses the bare table name as its qualifier because these UPDATE statements don't alias the table. That keeps it valid for schema-qualified tables too.

Tables without column overrides still get RETURNING * / SELECT *. The only change to their SQL is the qualified ORDER BY. SQLite was not affected, because it already renames returned rows itself.

Integration tests in explicit_column_name/crud_test.dart cover updateById, updateWhere, updateWhere with orderBy and limit, and updateWhere with limit only. All of them fail without the fix and pass with it. The existing update_where, update_by_id and sql_injection integration suites also pass.

Fixes #5911

Pre-launch Checklist

  • I read the Contribute page and followed r submitting PRs.
  • This update contains only one single feature or bug fix and nothing else. (If you are submitting multiple fixes, please make multiple PRs.)
  • I read and followed the [Dart Style Guide](https://dart.dev/guides/language/effecd the code with dart format.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///), and made sure t the same style as other Serverpod documentation. I checked spelling and grammar.
  • I added new tests to check the change I am making.
  • All existing and new tests are passing.
  • Any breaking changes are documented below.

If you need help, consider asking for advice on the discussion board.

Breaking changes

None.

@vchrisb
fix: Return field names from updateWhere and updateById for explicit …
7aad16c 
…columns

The Postgres updateWhere and updateById returned rows with RETURNING *,
so columns with a column= override came back under their database name
and failed to deserialize. Build the returning clause with
buildReturningClause, as insert and update already do, and qualify the
CTE ORDER BY so it still resolves to the database columns.

Fixes serverpod#5911
marcelomendoncasoares
marcelomendoncasoares requested changes Oct 26, 2026
View reviewed changes

@marcelomendoncasoares marcelomendoncasoares left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix, @vchrisb! Only a small request to increase the coverage.

Comment thread ...rverpod_test_server/test_integration/database_operations/explicit_column_name/crud_test.dart Resolved
@vchrisb

vchrisb commented Oct 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

The new test model exposed a bug in rename detection

The TableWithSwappedColumnNames model added in 03a571c / 10a17de maps its fields to each other's column names (field first → column second, and the reverse). The migrations for the model are fine; each one is a plain CREATE TABLE. The bug appears when the live database schema is compared with the target definition.

Why it happens: generateTableMigration matches columns across versions by effectiveFieldName, which falls back to the column name when fieldName is null. A live database schema never has field names. So the live column second is treated as field second, which in the target maps to column first, and the reverse. The result is a rename cycle (first → second, second → first) for a table that is already in sync. As SQL, those renames can't even run, because each target name still exists.

Before this PR, no model had explicit column names that collided with another field's name, so the case never came up.

Potential Fix: if the source column has no stored fieldName and the target still has a column with the same name, treat it as the same column, not a rename (migration.dart).

Related:

  • A real swap between two stored definitions (both with fieldName) still generates the two renames back to back with no temporary name, so the migration fails on Postgres and SQLite. The dialects would need to rename through a temporary name.
  • definition.json files written before fieldName existed (feat: Generate column rename when overriding the column name on a model #4932) also have null field names, so the new guard could skip a real swap from such a file. An explicit "live schema" flag would be more robust than inferring that from fieldName == null.

@marcelomendoncasoares what do you suggest how to proceed?
I think this uncovered a different issue that should be handled separately.

@marcelomendoncasoares

marcelomendoncasoares commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

@marcelomendoncasoares what do you suggest how to proceed? I think this uncovered a different issue that should be handled separately.

@vchrisb Yeah, we should not try to fix the rename issue it here. Inclusively, the fix might not be as straightforward, because the current fallback exists to allow renames that transfer a column to another field to be done in two steps.

If there is no other way to create a test "where a database column name is also the returned alias of a different field", we can skip this for now and merge.

@vchrisb
vchrisb force-pushed the fix/update-where-explicit-column-returning branch from 10a17de to 7aad16c Compare October 26, 2026 21:51
@vchrisb

vchrisb commented Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

I removed the failing test and will create a separate issue for that.

@vchrisb vchrisb mentioned this pull request Oct 26, 2026
Open
1 task
marcelomendoncasoares
marcelomendoncasoares approved these changes Oct 27, 2026
View reviewed changes

@marcelomendoncasoares marcelomendoncasoares left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀

@marcelomendoncasoares
marcelomendoncasoares merged commit d749e5a into serverpod:main Oct 27, 2026
961 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 27, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@marcelomendoncasoares marcelomendoncasoares marcelomendoncasoares approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

updateWhere fails to deserialize returned rows when a field uses a column= override

2 participants

@vchrisb @marcelomendoncasoares