Skip to content

fix(grails-data-hibernate7): stop conflating decimal and bit precision for numeric columns - #16026

Open
borinquenkid wants to merge 2 commits into
8.0.xfrom
fix/gorm-numeric-column-dialect-precision
Open

fix(grails-data-hibernate7): stop conflating decimal and bit precision for numeric columns#16026
borinquenkid wants to merge 2 commits into
8.0.xfrom
fix/gorm-numeric-column-dialect-precision

Conversation

@borinquenkid

Copy link
Copy Markdown
Member

Summary

NumericColumnConstraintsBinder applied one hand-rolled default precision (15, or 126 for Oracle) to every Number subtype when no explicit precision/constraints were configured. That conflates two different meanings of "precision":

  • NUMERIC/DECIMAL (BigDecimal/BigInteger) precision is a decimal digit count.
  • FLOAT/DOUBLE precision is a bit count (IEEE-754) — Hibernate converts whatever decimal-digit value it's given into bits when rendering float(n) DDL.

Feeding a decimal-oriented default (Hibernate's own Size.DEFAULT_PRECISION = 19, or naively a dialect's already-bit-valued getFloatPrecision()/getDoublePrecision()) overflows H2/PostgreSQL's 53-bit ceiling and produces DDL those dialects reject — silently, since hibernate.hbm2ddl.auto only logs the failure rather than throwing, so the table is never created.

Fix: Float/Double now leave precision unset, letting Hibernate fall back to the dialect's own correct float/double DDL type directly. BigDecimal/BigInteger default from Dialect.getDefaultDecimalPrecision(), which every dialect already computes correctly — removing the need for the Oracle-specific special case entirely.

Along the way this also caught a real gap in SimpleValueBinderSpec's test double (JdbcEnvironment.getDialect() was never stubbed, returning null — previously harmless, now exposed as an NPE since the fix calls dialect.getDefaultDecimalPrecision() directly). Fixed the mock rather than adding defensive null-handling in production code for a scenario a real Hibernate bootstrap can't produce.

Root cause, reproduced with real DDL

Reverting to Hibernate's own Size.DEFAULT_PRECISION (19) against H2 produces:

create table numeric_type_message (
    big_decimal_value numeric(19,2) not null,
    double_value float(64) not null,
    float_value float(64) not null,
    ...
)
org.h2.jdbc.JdbcSQLSyntaxErrorException: Precision ("64") must be between "1" and "53" inclusive

logged as a WARN via ExceptionHandlerLoggedImpl, not thrown — the table is silently dropped from the schema.

Test plan

  • New GormNumericTypeColumnPrecisionSpec (H2, no container): covers the default-precision behavior for Float/Double/BigDecimal, plus a live-DDL check that the table is actually created with valid columns.
  • New GormNumericTypeColumnIntegrationSpec (Postgres/MySQL/MariaDB via Testcontainers, Oracle excluded per the existing RLikeHibernate7Spec precedent): confirms valid, creatable DDL across dialects.
  • NumericColumnConstraintsBinderSpec: updated for the new propertyType parameter, added cases for the Float/Double/BigDecimal split and a parameterized dialect check (H2/Postgres/MySQL/Oracle) proving no per-dialect special case is needed.
  • Confirmed RED against unmodified code (H2: float(64) rejected; with my first, incorrect attempt at feeding dialect.getFloatPrecision() through setPrecision(): float(80), still rejected) and GREEN after the fix (float(24), float(53), numeric(38,2)).
  • Full grails-data-hibernate7-core suite: 3020 tests, 0 failures, 24 pre-existing skips.
  • codeStyle (checkstyle + CodeNarc): clean.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com

…n for numeric columns

NumericColumnConstraintsBinder applied one hand-rolled default precision (15,
126 for Oracle) to every Number subtype when no explicit precision/constraints
were configured. That conflates two different meanings of "precision": NUMERIC/
DECIMAL (BigDecimal/BigInteger) precision is a decimal digit count, but FLOAT/
DOUBLE precision is a bit count (IEEE-754) - and Hibernate converts whatever
decimal-digit value it's given into bits when rendering float(n) DDL. Feeding a
decimal-oriented default (Hibernate's own 19/38, or naively a dialect's already-
bit-valued getFloatPrecision()/getDoublePrecision()) overflows H2/PostgreSQL's
53-bit ceiling and produces DDL those dialects reject - silently, since
hibernate.hbm2ddl.auto only logs the failure rather than throwing, so the table
is never created.

Float/Double now leave precision unset, letting Hibernate fall back to the
dialect's own correct float/double DDL type directly. BigDecimal/BigInteger
default from Dialect.getDefaultDecimalPrecision(), which every dialect already
computes correctly - removing the need for the Oracle-specific special case.

Adds GormNumericTypeColumnPrecisionSpec (H2, no container) covering the default-
precision behavior for all three type families, and GormNumericTypeColumnIntegrationSpec
(Postgres/MySQL/MariaDB via Testcontainers, Oracle excluded per the existing
RLikeHibernate7Spec precedent) confirming valid, creatable DDL across dialects.
Reproduced the original bug directly: reverting to Hibernate's own
Size.DEFAULT_PRECISION (19) produces `float(64)` DDL, which H2 rejects with
"Precision (64) must be between 1 and 53 inclusive".

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 20, 2026 17:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This pull request fixes default numeric column precision handling in grails-data-hibernate7 to avoid conflating decimal precision (NUMERIC/DECIMAL) with floating-point bit precision (FLOAT/DOUBLE), which could otherwise generate invalid float(n) DDL (notably on H2/PostgreSQL) and cause schema creation to silently skip table creation.

Changes:

  • Update NumericColumnConstraintsBinder to leave Float/Double precision unset (letting Hibernate/dialect choose the correct float/double DDL), while defaulting decimal precision via Dialect.getDefaultDecimalPrecision() for non-floating numeric types.
  • Extend ColumnBinder to pass the bound property’s Java type into NumericColumnConstraintsBinder.
  • Add new H2 and Testcontainers-based integration specs to verify the schema is actually created with valid numeric columns across dialects; adjust existing unit specs/mocks for the new binder signature and dialect access.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/binder/NumericColumnConstraintsBinder.java Splits float/double vs decimal precision behavior; removes dialect-specific special-casing and uses getDefaultDecimalPrecision() for decimal defaults.
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/binder/ColumnBinder.java Passes the property type into numeric constraint binding so float/double can be handled differently.
grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/GormNumericTypeColumnPrecisionSpec.groovy New H2-only regression coverage validating unset float/double precision and confirming the table is actually created.
grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/GormNumericTypeColumnIntegrationSpec.groovy New multi-dialect (Postgres/MySQL/MariaDB) Testcontainers check ensuring creatable DDL and non-dropped table behavior.
grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/NumericColumnConstraintsBinderSpec.groovy Updates unit tests for the new signature and adds coverage for float/double precision being left unset and dialect-default decimal precision.
grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/ColumnBinderSpec.groovy Updates mock expectations to match the new binder method signature.
grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/SimpleValueBinderSpec.groovy Fixes the JdbcEnvironment test double to return a non-null Dialect now required by the updated production code path.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.0000%. Comparing base (fa30af0) to head (b711dd5).
⚠️ Report is 49 commits behind head on 8.0.x.

Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                @@
##                8.0.x   #16026         +/-   ##
=================================================
- Coverage     51.4351%        0   -51.4351%     
=================================================
  Files            2039        0       -2039     
  Lines           95497        0      -95497     
  Branches        16564        0      -16564     
=================================================
- Hits            49119        0      -49119     
+ Misses          39078        0      -39078     
+ Partials         7300        0       -7300     

see 2039 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@borinquenkid
borinquenkid requested a review from jdaugherty July 21, 2026 00:54

@jdaugherty jdaugherty left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • No test for the Float/Double + min/max constraint case — the behavior change I flagged above goes undocumented by a test. Add one asserting precision stays null in that scenario to lock in the intent.
  • GormNumericTypeColumnIntegrationSpec starts three @shared containers. With @testcontainers, all shared containers are started for the whole spec regardless of the where: row, so the manual if (!container.isRunning()) container.start() guard is redundant, and you pay for Postgres+MySQL+MariaDB every run. Consider whether all three need to be @Shared/eagerly started, or drop the redundant guard.

- Lock in that min/max constraints don't leak into Float/Double
  precision, per review request on #16026 to cover that scenario
  explicitly rather than only the no-constraint case.
- Drop the redundant container.start() guard in
  GormNumericTypeColumnIntegrationSpec: @testcontainers already starts
  every @shared container for the whole spec regardless of the where:
  row, so the guard never does anything.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@testlens-app

testlens-app Bot commented Jul 21, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: b711dd5
▶️ Tests: 49188 executed
⚪️ Checks: 59/59 completed


Learn more about TestLens at testlens.app.

@jamesfredley jamesfredley moved this to Todo in Apache Grails Jul 24, 2026
@borinquenkid borinquenkid added this to the grails:8.0.0-RC1 milestone Jul 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

4 participants