fix(index): a Text field on a numeric column no longer makes the contentlet unindexable (#37272)

[fabrizzio-dotCMS]

Closes #37272. PR 2 of 2 in the Spec-Kit flow — the implementation. The spec is PR #37393 (approved by @dario-daza); until it merges this PR's diff also shows the spec commit, which is the shared ancestor rather than a duplicate.

The defect

ESMappingAPIImpl.loadFields picks its serialization branch from the storage column (field_contentlet), not the declared field type. dotCMS allows a TextField to be backed by an integerN/floatN column, and its own built-in types do it — htmlpageasset.sortOrder is an ImmutableTextField with DataTypes.INTEGER. So the value in hand may be a String, and handing it to DecimalFormat.format() threw:

java.lang.IllegalArgumentException: Cannot format given Object as a Number
    at ESMappingAPIImpl.loadFields(ESMappingAPIImpl.java:1118)
    at ESMappingAPIImpl.toMap(ESMappingAPIImpl.java:465)

Because the per-field catch rethrows, the whole contentlet was lost from the index — every other field included. And from both engines at once, in every migration phase: the document is rendered once before the provider fan-out (ContentletIndexAPIImpl.mapContentletForProcessor — "Compute mapping once; reuse across all providers").

The fix

loadNumericField now owns that branch:

Value in hand	Emits
Number	the identical pre-existing path — same object, same formatter call
String that converts ("54", "54.3")	same entries a natively-stored number produces, padding included
anything else	both keys omitted, one WARN naming field and content type

Conversion is NumberUtil.toLongOrEmpty / toFloatOrEmpty — new, and they report failure instead of substituting a default, because the caller must be able to omit rather than fabricate.

Routing a Number straight through is deliberate: it makes "no correctly-stored document moves" true by construction rather than dependent on the converter being equivalent.

Why both keys must go together

This is the part worth reviewing closely — it is not stylistic.

toMap derives <field>_dotraw from <field>_text, and falls back to the bare key when _text is absent, unpadded. So writing the numeric key alone produces a malformed _dotraw. And _dotraw is a keyword that every sort resolves through — addBuilderSort appends the suffix to whatever field name the caller passes. Fixed-width zero padding is therefore the only reason ordering by a numeric field is numeric rather than alphabetical:

• raw text: "N/A" sorts after every padded digit string ('N' = 0x4E > '9' = 0x39)
• unpadded number: 54 sorts before 9

Either way a listing silently reorders, with no error anywhere. The chain runs from loadFields through toMap's post-processing to addBuilderSort and is invisible from any single file, so it is now written down in docs/backend/INDEX_FIELD_EMISSION.md — it had already been broken once, in the spec's own first revision.

Omit, not zero

The numeric key is skipped rather than defaulted to 0: a 0 is indistinguishable from a genuine 0 in range queries, sorts and aggregations, with only a WARN to reveal the difference. A false in-range match is a correctness bug; an unset field is an honest absence. Recorded as decision C-3 in the spec, with the rejected alternative.

Tests

26/26 unit over the converters — signs, surrounding whitespace, already-a-Number, blanks, overflow, fractional-in-integer (rejected, not truncated), and the non-finite NaN/Infinity that Float.parseFloat accepts but JSON cannot carry.

15/15 integration over the emission contract via toMap:

• both columns, integer and float, with convertible and unconvertible values
• the _dotraw invariant — asserted as absence of the key, since assertNotEquals(rawText, …) would pass against the unpadded-number bug
• emitted runtime class matches the native path (Integer 54 is not equals to Long 54)
• one WARN naming field and content type, and not the value, which is customer content
• nothing logged at all on the happy path
• the neighbouring branches of the edited else if chain — boolean, date, general text
• unique field: the SHA-256 entry is skipped, not computed over nothing
• end to end through a real index: convertible values sort numerically asc and desc, and an unconvertible document does not answer [0 TO 10]

Registered in MainSuite1b and deliberately phase-agnostic — it exercises document construction only, never calls setPhase(...) and never assumes an index name resolves, so the weekly OpenSearch Phase Sweep runs it without reproducing #37432.

Run:

./mvnw test -pl :dotcms-core -Dtest=NumberUtilTest -Dmaven.build.cache.enabled=false
./mvnw install -pl :dotcms-core --am -DskipTests
./mvnw verify -pl :dotcms-integration -Dcoreit.test.skip=false -Dit.test=ESMappingAPINumericFieldTest

Deliberately out of scope

The surrounding per-field catch still rethrows — any other unserializable field still costs the whole document. Now tracked in #37462. Left out on purpose: it changes how indexing fails in the hottest path in indexing, converting a loud failure (document absent) into a quiet one (document present, one field short), and forcing an unexpected serialization failure has no test seam. That belongs in its own cycle with its own review.

Also out, per the spec's non-goals: the index mapping generator (rollback-unsafe, forces a reindex), repairing existing mis-typed stored values, content-type-save validation (it already permits this modelling, and forbidding it would break dotCMS's own built-in types at bootstrap), and ImportStarterUtil — the one ingestion path that can create this data, named as a known gap.

Rollback safety

Not rollback-unsafe. No mapping, schema, API-contract or serialized-state change. For existing good data the emitted document is unchanged, so old and new pods emit identical documents during a rolling deploy. For the currently-failing contentlets the change is additive — documents that were absent start appearing; a rollback returns them to absent, which is where they are today. No data repair needed: a normal reindex recovers them.

Review focus

The both-keys-together pairing and the omit-vs-0 call. Everything else follows from them.

🤖 Generated with Claude Code

[claude]

Claude finished @fabrizzio-dotCMS's task in 2m 33s —— View job

---

PR Review

• Read the diff
• Review NumberUtil.java converters
• Review ESMappingAPIImpl.loadNumericField
• Review tests + docs

I focused review where you asked — the both-keys-together pairing and the omit-vs-0 call — and traced the emission chain (loadNumericField → toMap _dotraw derivation → addBuilderSort) to confirm the invariant holds.

Findings

No blocking issues found. The fix is correct and the design decisions hold up under scrutiny:

• Both-keys-together is sound. In loadNumericField (ESMappingAPIImpl.java:1181-1209) keyName and keyNameText are always written as a pair or both omitted — there is no path that emits one without the other, so toMap's bare-key fallback for _dotraw can never fire on a numeric field. The integration test pins this correctly by asserting absence of _dotraw (test_unconvertibleValue_dotrawIsAbsent_notUnpadded), which is the only assertion that actually catches the unpadded-number regression — assertNotEquals would not.
• Number passthrough is genuinely equivalent. The Number branch (ESMappingAPIImpl.java:1185-1189) reuses the identical object and the same numFormatter.format(...) call as the pre-existing code, so correctly-stored documents are byte-identical. The converted path produces the same runtime class (Long for integer columns, Float for float columns) — verified against the native path in test_nativeNumber_* / test_convertible*, and the class-identity assertions (Long.class / Float.class) matter because Integer 54 != Long 54.
• Strict conversion is right. toLongOrEmpty (NumberUtil.java:94-118) rejects fractional-in-integer rather than truncating, and finiteOrEmpty (NumberUtil.java:151-153) correctly filters the NaN/Infinity that Float.parseFloat accepts but JSON cannot carry — pushing that failure downstream into the index write would have been a real trap.
• Null/empty guarded upstream. loadNumericField is only reachable when UtilMethods.isSet(valueObj) is true (ESMappingAPIImpl.java:1044-1047 short-circuits the else-if chain), so valueObj is never null here.
• WARN is safe. The log (ESMappingAPIImpl.java:1198-1203) names field, content type and column but never the value — correct for customer content, and asserted in test_*_warns_*.
• Unique-field SHA correctly skipped on omission — field.isUnique() && contentletMap.containsKey(keyName) (ESMappingAPIImpl.java:1138) is false when the key was omitted, so no SHA over a phantom value. Also covered by test.

Minor notes (non-blocking, no action required)

• 🟡 NumberUtil.java:100 — the fractional/finiteness guard number.longValue() != number.doubleValue() is only exercised by external callers of toLongOrEmpty, since loadNumericField returns early for any Number (line 1185) and only ever passes Strings here. It's correct as written (large in-range longs compare equal after both promote to double; out-of-range doubles clamp and are rejected), just dead relative to this PR's flow. Fine as a general-purpose util.
• DecimalFormat is not thread-safe, but numFormatter is a per-call local (ESMappingAPIImpl.java:997), so no sharing concern introduced.

The scope boundaries (rethrowing catch left to #37462, ImportStarterUtil named as a known gap, no mapping/schema change) are documented and reasonable, and the new docs/backend/INDEX_FIELD_EMISSION.md captures the cross-file invariant that made this class of bug invisible. Rollback-safety analysis checks out — the change is additive for failing contentlets and a no-op for good data.

Nice work — the tests are unusually good at targeting the exact regressions the design guards against.
• 37272-impl-textfield-numeric-column

[ihoffmann-dot]

Approved ✅

The fix correctly isolates the DecimalFormat-on-String failure into loadNumericField, converting best-effort via the new NumberUtil.toLongOrEmpty/toFloatOrEmpty and omitting both the numeric key and its _dotraw companion together when conversion fails.

Consistent with the existing branch structure in ESMappingAPIImpl and doesn't touch the index mapping generator, so no rollback risk.

Test coverage is thorough: unit + integration tests covered.
Ready to merge.