🐛 fix: surface UnexpectedNullableFound instead of remapping to ColumnNotFound (closes #560)#712
Conversation
…nNotFound Previously, NULL columns triggered an `UnexpectedNullableFound` error from `Column[T]`, but `SqlParser`'s internal `parseColumn` helper remapped it to `ColumnNotFound`, producing misleading "column not found" errors like "'X' not found, available columns: X, X" where the column was actually present and merely contained NULL. The fix drops the remap, and the now-trivial `parseColumn` helper is inlined at its three call sites. The `.?` combinator on `RowParser` previously depended on the remap to convert NULL to `None`; it now recovers from `UnexpectedNullableFound` directly, honouring its documented "missing or null column as None" contract. Closes playframework#560. Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
| Success(None) | ||
| case Success(a) => Success(Some(a)) | ||
| case Error(ColumnNotFound(_, _)) => Success(None) | ||
| case Error(UnexpectedNullableFound(_)) => Success(None) |
There was a problem hiding this comment.
If that's the desired behaviour, the pattern could be merge with the previous case
|
Thanks for the PR @MavenRain , this is pretty much the same as what Copilot already suggested in #665 but I haven't had time to dig a bit more at the consequences of the changes as I'm not yet familiar with the codebase and was a bit reluctant to merge without better understanding. I'll try to find some time soon to review this. Unless this is straightforward good for you @cchantep :) |
| * that will turn missing or null column as None. | ||
| */ | ||
| def ? : RowParser[Option[A]] = RowParser { | ||
| // Note: `UnexpectedNullableFound` is the error raised by `Column` when a |
There was a problem hiding this comment.
I don't think we need this entire comment. Sounds like classic AI over commenting ;)
| case in @ (_, m) => | ||
| parseColumn(row, m.column.qualified, c, in) | ||
| case in @ (_, _) => | ||
| // Previously a `parseColumn` helper mapped `UnexpectedNullableFound` to |
There was a problem hiding this comment.
Same comment, I don't think the comment is useful.
Not straightforward, needs cautious review. |
3 participants
Closes #560.
Problem
scalar[T].single(and other non-Option parsers) on a column containingNULL produced this confusing error:
'.completed_average' not found, available columns: completed_average,
completed_average
The column was actually present; it just contained NULL. The root cause
was an internal
parseColumnhelper inSqlParserthat remappedUnexpectedNullableFoundtoColumnNotFound. @gaeljw confirmed theimpact in
#560 (comment).
Fix
Two parts:
SqlParser.scala: drop theUnexpectedNullableFound → ColumnNotFoundremap in
parseColumn. Since the helper then only forwarded toc.tupled(input), it has been inlined at its three call sites andremoved. Errors from
Column[T].tuplednow surface unchanged, sousers see
UnexpectedNullableFound(column_name)and can distinguishNULL from missing column.
RowParser.scala(.?combinator): the optional-column combinatorpreviously only recovered from
ColumnNotFound. Its scaladoc hasalways promised "missing or null column as None", and the existing
singleOpt/?semantics depended on the buggy mapping to convertNULL to
None. Updated.?to also recover fromUnexpectedNullableFound, honouring its documented contractindependently of the
parseColumnremap.Regression test
AnormSpecgains one test verifying thatscalar[String].singleon aNULL result throws an error whose message contains
UnexpectedNullableFoundand does not containnot found, available columns.Behaviour change
User code that catches
ColumnNotFoundspecifically to recognise a NULLcolumn will now see
UnexpectedNullableFoundinstead. This was thedocumented behaviour all along; the buggy remap was the deviation. No
other observable change.
Local validation
++ 2.13; anorm-core/compile anorm-core/Test/compileclean++ 3; anorm-core/compile anorm-core/Test/compileclean++ 2.13; anorm-core/test294 / 294 pass (including the new test andthe existing "be parsed from mapping with optional column" test which
previously depended on the buggy mapping)
++ 3; anorm-core/testOnly tests.AnormSpec52 / 52 passscalafmtCheckAllcleananorm-core/mimaReportBinaryIssuesno binary issues