diff --git a/CLAUDE.md b/CLAUDE.md index 76a2579afcdb..d76c175cd607 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -127,6 +127,7 @@ How it's built + upgrade re-apply notes: [.specify/CUSTOMIZATIONS.md](.specify/C - [Security Patterns](docs/backend/SECURITY_BACKEND.md) — Input validation, auth, SQL/XSS prevention, secure logging - [Search API Migration](docs/backend/SEARCH_API_MIGRATION.md) — ES → OpenSearch: deprecated `ContentletAPI` search methods, plugin migration guide - [Telemetry Implementation](docs/backend/TELEMETRY_IMPLEMENTATION.md) — CDI-based metrics system, creating new metrics, `/v1/usage` endpoints +- [Index Field Emission](docs/backend/INDEX_FIELD_EMISSION.md) — how `loadFields`/`toMap` build the index document; the `_dotraw` zero-padding sort invariant - [Jandex Metadata Scanning](docs/backend/JANDEX_METADATA_SCANNING.md) — Fast class/annotation metadata lookup, prefer over reflection - **ES → OpenSearch Migration** — infra migration from ElasticSearch to OpenSearch, phased dual-write/read rollout - [Migration Design](docs/backend/OPENSEARCH_MIGRATION.md) — Architecture, phased rollout, configuration diff --git a/docs/backend/INDEX_FIELD_EMISSION.md b/docs/backend/INDEX_FIELD_EMISSION.md new file mode 100644 index 000000000000..828873cf84c8 --- /dev/null +++ b/docs/backend/INDEX_FIELD_EMISSION.md @@ -0,0 +1,105 @@ +# Index Field Emission — how a contentlet becomes an index document + +How `ESMappingAPIImpl` turns a contentlet's fields into the document written to Elasticsearch and +OpenSearch, and the one invariant in that path that is easy to break and impossible to see from +any single file. + +Applies to both engines: the document is rendered **once** and shared. + +## The two keys every field emits + +`loadFields` writes each field under two names (`ESMappingAPIImpl.java`, in the per-field loop): + +| Local variable | Key written | What happens to it | +|---|---|---| +| `keyName` | `.` | survives into the final document | +| `keyNameText` | `._text` | **renamed** to `_dotraw`, then dropped | + +The rename is not in `loadFields` — it is in `toMap`, in the post-processing loop that runs +*after* `loadFields` returns. That loop: + +1. lowercases every key, +2. renames `_text` → `_dotraw`, +3. drops the `_text` key unless `CREATE_TEXT_INDEX_FIELD_FOR_NON_TEXT_FIELDS=true` (default false). + +**So `_dotraw` is the only survivor of the pair**, and a test that calls `loadFields` in isolation +cannot observe it at all. + +## The invariant: `_dotraw` on a numeric field is zero-padded, or absent + +`*_dotraw` is mapped `keyword` (`es-content-mapping.json`, `template_1`), so it sorts +**lexicographically**. And every sort in dotCMS resolves through it — `addBuilderSort` appends +`_dotraw` to whatever field name the caller passes (`ContentFactoryIndexOperationsES`, and the +OpenSearch counterpart). + +That is why numeric fields are padded to a fixed width by the `DecimalFormat` in `loadFields`: + +``` +"0000000000000000000.000000000000000000" 19 integer digits, 18 decimals +54 → "0000000000000000054.000000000000000000" +``` + +Fixed-width zero-padding is the **only** reason ordering by a numeric field is numeric rather than +alphabetical. Break it and results silently reorder — no error, no exception, nothing in the log: + +- `"N/A"` sorts after every padded digit string, because `'N'` (0x4E) > `'9'` (0x39). +- an unpadded `54` sorts before `9`. + +**Rule: for a field on a numeric storage column, `_dotraw` is either the padded form or the +key is not written at all. Never raw text, never an unpadded number.** + +### The trap when omitting a field + +`toMap`'s derivation loop has a fallback: when `_text` is missing it synthesizes +`_dotraw` **from the bare key's value, unpadded**. So omitting only `_text` while still +writing the numeric key produces exactly the malformed `_dotraw` this invariant forbids. + +**Omitting a numeric field means omitting both keys together.** They are one atomic change. + +## Storage column decides the branch, not the field type + +`loadFields` selects its serialization branch from `field.getFieldContentlet()` — the storage +column — not from the declared field type. That matters because dotCMS allows a `TextField` to be +backed by a numeric column, and its own built-in types do it: +`htmlpageasset.sortOrder` is an `ImmutableTextField` with `DataTypes.INTEGER` +(`PageContentType`), and `FieldFactoryImpl` accepts it because `TextField.acceptedDataTypes()` +includes `INTEGER` and `FLOAT`. + +Consequence: **the value in a numeric-column field may be a `Number` or a `String`.** Handing a +`String` straight to `DecimalFormat.format` throws `IllegalArgumentException`, and since the +per-field catch rethrows, the whole contentlet is lost from the index — from *both* engines at +once, in every migration phase, because the mapping is computed before the provider fan-out +(`ContentletIndexAPIImpl.mapContentletForProcessor`: *"Compute mapping once; reuse across all +providers"*). This was issue #37272. + +`loadNumericField` handles it: a `Number` takes the identical pre-existing path, a `String` is +converted best-effort against the column's own type (`NumberUtil.toLongOrEmpty` / +`toFloatOrEmpty`), and a value that cannot be represented numerically omits both keys and logs a +WARN naming the field and content type. + +### Generated mapping by column + +| `field_contentlet` | Mapping | Emitted class | +|---|---|---| +| `integer%` | `long` | `Long` | +| `float%` | `double` | **`Float`** | +| `bool%` | `boolean` | `Boolean` | +| `*_dotraw` | `keyword`, `ignore_above: 8191` | `String` | + +Mapping type and emitted class differ by design (`double` vs `Float`). It is pre-existing and +harmless — do not "normalize" it, since changing an emitted class changes the document for every +correctly-stored value in every installation. + +## Testing this path + +- **Assert against `toMap`, not `loadFields`.** `_dotraw` does not exist yet when `loadFields` + returns, so the assertions that matter are unreachable from there. +- **Assert the key is absent**, not that it differs from the raw text. `assertNotEquals("n/a", …)` + passes against the unpadded-number bug described above. +- **Field defaults bite.** `FieldDataGen` defaults `defaultValue` to `"testDefaultValue"`, + a non-numeric String; combined with the generator's `IndexPolicy.FORCE`, the save indexes + synchronously and a numeric-column field will fail during test setup. Give it a numeric default. +- **A `unique` field is implicitly required** and must carry a value at save time; a field-level + default is not enough. +- **`CheckboxField` does not accept `DataTypes.BOOL`** — only Hidden, Radio and Select do. +- Working example: `dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ESMappingAPINumericFieldTest.java` diff --git a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESMappingAPIImpl.java b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESMappingAPIImpl.java index 62ae02b89b66..a1f5c2dc7b74 100644 --- a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESMappingAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESMappingAPIImpl.java @@ -65,6 +65,7 @@ import com.dotmarketing.util.Config; import com.dotmarketing.util.InodeUtils; import com.dotmarketing.util.Logger; +import com.dotmarketing.util.NumberUtil; import com.dotmarketing.util.PaginatedArrayList; import com.dotmarketing.util.ThreadSafeSimpleDateFormat; import com.dotmarketing.util.UtilMethods; @@ -1114,8 +1115,8 @@ else if (field.getFieldType().equals(ESMappingConstants.FIELD_ELASTIC_TYPE_DATE) .startsWith(ESMappingConstants.FIELD_ELASTIC_TYPE_FLOAT) || field .getFieldContentlet() .startsWith(ESMappingConstants.FIELD_ELASTIC_TYPE_INTEGER)) { - contentletMap.put(keyName, valueObj); - contentletMap.put(keyNameText, numFormatter.format(valueObj)); + loadNumericField(contentletMap, field, structure, valueObj, keyName, + keyNameText, numFormatter); } else { if (valueObj instanceof Date){ try { @@ -1147,6 +1148,66 @@ else if (field.getFieldType().equals(ESMappingConstants.FIELD_ELASTIC_TYPE_DATE) } } + /** + * Emits a field whose storage column is numeric ({@code integerN} / {@code floatN}). + * + *

The column, not the field type, decides this branch — and dotCMS allows a + * {@link com.dotcms.contenttype.model.field.TextField} to be backed by a numeric column, as + * its own built-in types do ({@code htmlpageasset.sortOrder}). So the value in hand may be a + * {@link Number} or a numeric {@link String}, and handing a String straight to + * {@code DecimalFormat.format} threw, aborting the whole document (issue #37272).

+ * + *

A value that is already a {@link Number} takes exactly the path it always took — same + * object, same formatter call — so documents for correctly-stored content do not move. A + * String is converted best-effort against the column's own type, so a stored {@code "54"} + * produces a document identical to a natively-stored {@code 54}.

+ * + *

When the value cannot be represented numerically, both keys are + * omitted. That pairing is not a preference:

+ *
    + *
  • The numeric key is skipped rather than defaulted to {@code 0}, because a {@code 0} is + * indistinguishable from a genuine {@code 0} in range queries, sorts and aggregations.
    • + *
  • {@code keyNameText} must go with it. {@code toMap}'s post-processing derives + * {@code _dotraw} from {@code _text}, and falls back to the bare key when + * that is absent — writing the numeric key alone would therefore produce an + * unpadded {@code _dotraw}. Since {@code _dotraw} is a {@code keyword} and every + * sort resolves through it, the fixed-width zero padding is the only reason ordering by + * a numeric field is numeric rather than lexicographic; an unpadded entry silently + * reorders results.
    • + *
+ * + * @param valueObj the raw value, a {@link Number} or {@link String} + */ + private void loadNumericField(final Map contentletMap, final Field field, + final Structure structure, final Object valueObj, final String keyName, + final String keyNameText, final DecimalFormat numFormatter) { + + if (valueObj instanceof Number) { + contentletMap.put(keyName, valueObj); + contentletMap.put(keyNameText, numFormatter.format(valueObj)); + return; + } + + final boolean floatColumn = field.getFieldContentlet() + .startsWith(ESMappingConstants.FIELD_ELASTIC_TYPE_FLOAT); + final Optional converted = floatColumn + ? NumberUtil.toFloatOrEmpty(valueObj) + : NumberUtil.toLongOrEmpty(valueObj); + + if (converted.isEmpty()) { + Logger.warn(ESMappingAPIImpl.class, String.format( + "Field '%s' of Content Type '%s' is stored in the numeric column '%s' but its " + + "value is not numeric; the field is omitted from the index document " + + "for this contentlet. The rest of the contentlet is indexed.", + field.getVelocityVarName(), structure.getVelocityVarName(), + field.getFieldContentlet())); + return; + } + + contentletMap.put(keyName, converted.get()); + contentletMap.put(keyNameText, numFormatter.format(converted.get())); + } + private boolean loadTagsField(final Contentlet contentlet, final Map contentletMap, final Structure structure, diff --git a/dotCMS/src/main/java/com/dotmarketing/util/NumberUtil.java b/dotCMS/src/main/java/com/dotmarketing/util/NumberUtil.java index 8115dfebca82..094328540d4b 100644 --- a/dotCMS/src/main/java/com/dotmarketing/util/NumberUtil.java +++ b/dotCMS/src/main/java/com/dotmarketing/util/NumberUtil.java @@ -1,6 +1,7 @@ package com.dotmarketing.util; import java.text.DecimalFormat; +import java.util.Optional; import java.util.function.Supplier; public class NumberUtil { @@ -78,4 +79,77 @@ public static int asInt(final Object value) { return value != null ? Integer.parseInt(value.toString()) : 0; } // asInt. + /** + * Best-effort coercion to a {@link Long} of a value that may already be a {@link Number} or a + * numeric {@link String}. + * + *

Unlike {@link #toLong(String, Supplier)} this reports failure instead of substituting a + * default: an empty result means "this value is not a long", which lets the caller omit the + * value entirely rather than fabricate one. Parsing is strict — a value carrying a fractional + * part is not silently truncated.

+ * + * @param value the value to coerce; may be null + * @return the coerced value, or empty when it cannot be represented as a long + */ + public static Optional toLongOrEmpty(final Object value) { + + if (value instanceof Number) { + final Number number = (Number) value; + // Reject a value carrying a fractional part rather than truncating it silently. + // Also rejects NaN and the infinities, whose longValue() is a meaningless clamp. + if (number.longValue() != number.doubleValue()) { + return Optional.empty(); + } + return Optional.of(number.longValue()); + } + if (!(value instanceof String)) { + return Optional.empty(); + } + final String candidate = ((String) value).trim(); + if (!UtilMethods.isSet(candidate)) { + return Optional.empty(); + } + try { + return Optional.of(Long.parseLong(candidate)); + } catch (final NumberFormatException e) { + // Not a long: non-numeric text, a fractional value, or beyond Long's range. + return Optional.empty(); + } + } // toLongOrEmpty. + + /** + * Best-effort coercion to a {@link Float} of a value that may already be a {@link Number} or a + * numeric {@link String}. Same reporting semantics as {@link #toLongOrEmpty(Object)}. + * + * @param value the value to coerce; may be null + * @return the coerced value, or empty when it cannot be represented as a finite float + */ + public static Optional toFloatOrEmpty(final Object value) { + + if (value instanceof Number) { + return finiteOrEmpty(((Number) value).floatValue()); + } + if (!(value instanceof String)) { + return Optional.empty(); + } + final String candidate = ((String) value).trim(); + if (!UtilMethods.isSet(candidate)) { + return Optional.empty(); + } + try { + return finiteOrEmpty(Float.parseFloat(candidate)); + } catch (final NumberFormatException e) { + return Optional.empty(); + } + } // toFloatOrEmpty. + + /** + * Guards against the non-finite values {@link Float#parseFloat} accepts. "Infinity" and "NaN" + * parse successfully but cannot be serialized to JSON, so letting them through would push the + * failure downstream into the index write instead of reporting it here. + */ + private static Optional finiteOrEmpty(final float candidate) { + return Float.isFinite(candidate) ? Optional.of(candidate) : Optional.empty(); + } // finiteOrEmpty. + } // E:O:F:NumberUtil diff --git a/dotCMS/src/test/java/com/dotmarketing/util/NumberUtilTest.java b/dotCMS/src/test/java/com/dotmarketing/util/NumberUtilTest.java index 044932c00400..aff37ce06652 100644 --- a/dotCMS/src/test/java/com/dotmarketing/util/NumberUtilTest.java +++ b/dotCMS/src/test/java/com/dotmarketing/util/NumberUtilTest.java @@ -4,8 +4,11 @@ import org.junit.Test; import java.text.ParseException; +import java.util.Optional; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; /** * Unit test for {@link DateUtil} @@ -84,4 +87,147 @@ public void testLongInt () throws ParseException { assertEquals(123l, i); } + // ------------------------------------------------------------------------------------------ + // toLongOrEmpty / toFloatOrEmpty — issue #37272 + // + // These report failure instead of substituting a default, because the caller + // (ESMappingAPIImpl.loadFields) must be able to OMIT the value rather than fabricate one. + // See specs/37272-textfield-numeric-column/spec.md, Resolved Decisions C-3. + // ------------------------------------------------------------------------------------------ + + @Test + public void test_toLongOrEmpty_numericString_converts () { + + final Optional result = NumberUtil.toLongOrEmpty("54"); + assertTrue("a numeric String must convert", result.isPresent()); + assertEquals(Long.valueOf(54L), result.get()); + } + + /** + * The emitted value has to be the SAME runtime class the natively-stored path produces, or + * the index document for a converted value differs from one that was stored correctly. + * {@code Integer 54} is not {@code equals} to {@code Long 54} (AC-009). + */ + @Test + public void test_toLongOrEmpty_returnsLong_notInteger () { + + final Object result = NumberUtil.toLongOrEmpty("54").orElseThrow(AssertionError::new); + assertEquals("emitted class must match the native path", Long.class, result.getClass()); + } + + @Test + public void test_toLongOrEmpty_negativeAndSigned_convert () { + + assertEquals(Long.valueOf(-54L), NumberUtil.toLongOrEmpty("-54").orElseThrow(AssertionError::new)); + assertEquals(Long.valueOf(54L), NumberUtil.toLongOrEmpty("+54").orElseThrow(AssertionError::new)); + } + + @Test + public void test_toLongOrEmpty_surroundingWhitespace_converts () { + + assertEquals(Long.valueOf(54L), NumberUtil.toLongOrEmpty(" 54 ").orElseThrow(AssertionError::new)); + } + + @Test + public void test_toLongOrEmpty_alreadyANumber_convertsToLong () { + + assertEquals(Long.valueOf(54L), NumberUtil.toLongOrEmpty(Integer.valueOf(54)).orElseThrow(AssertionError::new)); + assertEquals(Long.valueOf(54L), NumberUtil.toLongOrEmpty(Long.valueOf(54L)).orElseThrow(AssertionError::new)); + } + + @Test + public void test_toLongOrEmpty_nonNumericText_isEmpty () { + + assertFalse("\"N/A\" is the real-world case from #37272", NumberUtil.toLongOrEmpty("N/A").isPresent()); + assertFalse(NumberUtil.toLongOrEmpty("123abc").isPresent()); + } + + @Test + public void test_toLongOrEmpty_nullAndBlank_areEmpty () { + + assertFalse(NumberUtil.toLongOrEmpty(null).isPresent()); + assertFalse(NumberUtil.toLongOrEmpty("").isPresent()); + assertFalse(NumberUtil.toLongOrEmpty(" ").isPresent()); + } + + /** + * Deliberately strict: truncating "54.3" to 54 for an integer-backed column would change the + * value silently. Empty means the caller omits the field, which is an honest absence rather + * than a quiet edit. Flagged for the developer at the approval gate — reversible. + */ + @Test + public void test_toLongOrEmpty_fractionalValue_isEmpty_notTruncated () { + + assertFalse("a fractional value must not be silently truncated", + NumberUtil.toLongOrEmpty("54.3").isPresent()); + } + + @Test + public void test_toLongOrEmpty_overflow_isEmpty () { + + assertFalse("beyond Long.MAX_VALUE cannot be represented", + NumberUtil.toLongOrEmpty("99999999999999999999").isPresent()); + } + + ///// + + @Test + public void test_toFloatOrEmpty_numericString_converts () { + + final Optional result = NumberUtil.toFloatOrEmpty("54.3"); + assertTrue("a decimal String must convert", result.isPresent()); + assertEquals(Float.valueOf(54.3f), result.get()); + } + + /** Float, not Double — matching what a float-backed column natively produces (AC-009). */ + @Test + public void test_toFloatOrEmpty_returnsFloat_notDouble () { + + final Object result = NumberUtil.toFloatOrEmpty("54.3").orElseThrow(AssertionError::new); + assertEquals("emitted class must match the native path", Float.class, result.getClass()); + } + + @Test + public void test_toFloatOrEmpty_wholeNumberString_converts () { + + assertEquals(Float.valueOf(54f), NumberUtil.toFloatOrEmpty("54").orElseThrow(AssertionError::new)); + } + + @Test + public void test_toFloatOrEmpty_alreadyANumber_convertsToFloat () { + + assertEquals(Float.valueOf(54f), NumberUtil.toFloatOrEmpty(Integer.valueOf(54)).orElseThrow(AssertionError::new)); + assertEquals(Float.valueOf(54.3f), NumberUtil.toFloatOrEmpty(Float.valueOf(54.3f)).orElseThrow(AssertionError::new)); + } + + @Test + public void test_toFloatOrEmpty_nonNumericText_isEmpty () { + + assertFalse(NumberUtil.toFloatOrEmpty("N/A").isPresent()); + assertFalse(NumberUtil.toFloatOrEmpty("123abc").isPresent()); + } + + @Test + public void test_toFloatOrEmpty_nullAndBlank_areEmpty () { + + assertFalse(NumberUtil.toFloatOrEmpty(null).isPresent()); + assertFalse(NumberUtil.toFloatOrEmpty("").isPresent()); + assertFalse(NumberUtil.toFloatOrEmpty(" ").isPresent()); + } + + /** + * Float.parseFloat accepts "Infinity" and "NaN". Neither can be indexed — a non-finite value + * breaks JSON serialization (the class of bug already seen in #36478/#36480), so they must be + * reported as unconvertible, not passed through. + */ + @Test + public void test_toFloatOrEmpty_nonFiniteValues_areEmpty () { + + assertFalse("Infinity must not reach the index", NumberUtil.toFloatOrEmpty("Infinity").isPresent()); + assertFalse(NumberUtil.toFloatOrEmpty("-Infinity").isPresent()); + assertFalse("NaN must not reach the index", NumberUtil.toFloatOrEmpty("NaN").isPresent()); + assertFalse(NumberUtil.toFloatOrEmpty(Float.NaN).isPresent()); + assertFalse(NumberUtil.toFloatOrEmpty(Float.POSITIVE_INFINITY).isPresent()); + } + } diff --git a/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java b/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java index f9332c46ce24..8018e7fe84c9 100644 --- a/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java +++ b/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java @@ -25,6 +25,7 @@ com.dotcms.contenttype.test.DeleteFieldJobTest.class, com.dotcms.content.elasticsearch.business.ESSiteSearchAPITest.class, com.dotcms.content.elasticsearch.business.ESMappingAPITest.class, + com.dotcms.content.elasticsearch.business.ESMappingAPINumericFieldTest.class, com.dotcms.content.elasticsearch.business.ContentletIndexAPIImplTest.class, com.dotcms.contenttype.test.ContentTypeAPIImplTest.class, com.dotcms.contenttype.test.ContentTypeBuilderTest.class, diff --git a/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ESMappingAPINumericFieldTest.java b/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ESMappingAPINumericFieldTest.java new file mode 100644 index 000000000000..89761afd7f12 --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ESMappingAPINumericFieldTest.java @@ -0,0 +1,634 @@ +package com.dotcms.content.elasticsearch.business; + +import static com.dotcms.content.elasticsearch.business.ESMappingAPIImpl.DOTRAW; +import static com.dotcms.content.elasticsearch.business.ESMappingAPIImpl.TEXT; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +import com.dotcms.contenttype.model.field.RadioField; +import com.dotcms.contenttype.model.field.DataTypes; +import com.dotcms.contenttype.model.field.DateTimeField; +import com.dotcms.contenttype.model.field.Field; +import com.dotcms.contenttype.model.field.TextField; +import com.dotcms.contenttype.model.type.ContentType; +import com.dotcms.datagen.ContentTypeDataGen; +import com.dotcms.datagen.ContentletDataGen; +import com.dotcms.datagen.FieldDataGen; +import com.dotcms.util.IntegrationTestInitService; +import com.dotmarketing.business.APILocator; +import com.dotmarketing.portlets.contentlet.model.Contentlet; +import com.dotmarketing.portlets.contentlet.model.IndexPolicy; +import com.dotmarketing.util.Config; +import java.util.ArrayList; +import java.util.Date; +import java.util.List; +import java.util.Map; +import java.util.regex.Pattern; +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.core.LogEvent; +import org.apache.logging.log4j.core.Logger; +import org.apache.logging.log4j.core.appender.AbstractAppender; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Emission contract for a Text field backed by a numeric storage column — issue #37272. + * + *

{@code loadFields} picks its serialization branch from the storage column + * ({@code field_contentlet}) rather than the field type, so a {@code TextField} on an + * {@code integerN}/{@code floatN} column hands a {@code String} to {@code DecimalFormat.format()}, + * which throws and — because the per-field catch rethrows — aborts {@code toMap} and loses the + * whole contentlet from the index.

+ * + *

Why these assertions live at the integration layer

+ *

The keys that matter do not exist inside {@code loadFields}. It writes + * {@code _text}; the rename {@code _text} → {@code _dotraw} happens later, in + * {@code toMap}'s post-processing loop, after {@code loadFields} returns. A {@code loadFields}-only + * unit test can therefore never observe {@code _dotraw}, which is the key every sort actually + * targets. See {@code specs/37272-textfield-numeric-column/research.md} R-1.

+ * + *

Phase-awareness

+ *

This class is registered in {@code MainSuite1b}, a shard of the weekly Scheduled OpenSearch + * Phase Sweep, so it also runs under OS Phase 3 where the {@code indicies} table carries no + * Elasticsearch pointers. It is deliberately phase-agnostic: it exercises + * {@code toMap} only — document construction, no index I/O and no phase routing — and must never + * call {@code setPhase(...)} nor assume an index name resolves. Getting that wrong reproduces + * issue #37432.

+ * + * @see ESMappingAPIImpl#toMap(Contentlet) + */ +public class ESMappingAPINumericFieldTest { + + /** The zero-padded form {@code _dotraw} must carry for a numeric field: 19 digits, 18 decimals. */ + private static final Pattern PADDED_NUMERIC = Pattern.compile("^\\d{19}\\.\\d{18}$"); + + private static final String NUMERIC_FIELD_VAR = "numericTextField"; + private static final String COMPANION_FIELD_VAR = "companionTextField"; + private static final String COMPANION_VALUE = "untouched"; + + private static ESMappingAPIImpl esMappingAPI; + + @BeforeClass + public static void prepare() throws Exception { + IntegrationTestInitService.getInstance().init(); + esMappingAPI = new ESMappingAPIImpl(); + // Keep the default: _text is dropped and only _dotraw survives, which is the shape + // production indexes actually have. + Config.setProperty("CREATE_TEXT_INDEX_FIELD_FOR_NON_TEXT_FIELDS", false); + } + + /** + * A content type carrying a Text field on the given numeric storage column — the modelling + * dotCMS allows and its own built-in types use ({@code htmlpageasset.sortOrder} is an + * {@code ImmutableTextField} with {@code DataTypes.INTEGER}). + */ + private static ContentType typeWithNumericTextField(final DataTypes dataType) { + final Field numericField = new FieldDataGen() + .type(TextField.class) + .dataType(dataType) + // FieldDataGen defaults defaultValue to "testDefaultValue" — a + // non-numeric String. Left alone it lands in the field on save, the save path + // indexes, and the defect under test fires during setup. "0" mirrors what + // dotCMS's own PageContentType does for its INTEGER-backed sortOrder field. + .defaultValue("0") + .velocityVarName(NUMERIC_FIELD_VAR) + .indexed(true) + .next(); + final Field companionField = new FieldDataGen() + .type(TextField.class) + .dataType(DataTypes.TEXT) + .velocityVarName(COMPANION_FIELD_VAR) + .indexed(true) + .next(); + return new ContentTypeDataGen() + .field(numericField) + .field(companionField) + .nextPersisted(); + } + + /** + * A persisted contentlet that the current code can map. + * + *

This matters more than it looks. {@code nextPersisted()} saves the contentlet, the save + * path indexes it (the generator forces {@code IndexPolicy.FORCE}), and indexing calls + * {@code toMap} — so a non-numeric value in the numeric field triggers the very defect under + * test during setup, and the test errors before reaching a single assertion. The + * field's {@code "0"} default keeps the save mappable; the value under test is substituted in + * memory afterwards, so only the {@code toMap} call in the test body ever sees it.

+ */ + private static Contentlet persistedContentlet(final ContentType type) { + return new ContentletDataGen(type.id()) + .setProperty(COMPANION_FIELD_VAR, COMPANION_VALUE) + .nextPersisted(); + } + + /** + * Reproduces the defect's state without persisting bad data: + * {@code Contentlet.setStringProperty} is a plain {@code map.put} with no coercion, and + * {@code loadFields} reads the value back through {@code contentlet.get(velocityVarName)}. + * This is the same shape {@code ImportStarterUtil} leaves in the database — a value whose + * type contradicts its column, reached without going through the coercing save path. + */ + private static Contentlet contentletWithStringInNumericField(final ContentType type, + final String storedValue) { + final Contentlet contentlet = persistedContentlet(type); + contentlet.setStringProperty(NUMERIC_FIELD_VAR, storedValue); + return contentlet; + } + + private static Contentlet contentletWithNumberInNumericField(final ContentType type, + final Number storedValue) { + final Contentlet contentlet = persistedContentlet(type); + contentlet.setProperty(NUMERIC_FIELD_VAR, storedValue); + return contentlet; + } + + private static String numericKey(final ContentType type) { + return (type.variable() + "." + NUMERIC_FIELD_VAR).toLowerCase(); + } + + // ---------------------------------------------------------------------------------------- + // AC-001 / AC-009 — a numeric value stored as a String indexes, exactly like a native number + // ---------------------------------------------------------------------------------------- + + /** + * The reported defect. Today {@code toMap} throws + * {@code IllegalArgumentException: Cannot format given Object as a Number}. + */ + @Test + public void test_integerColumn_numericString_isIndexed() { + final ContentType type = typeWithNumericTextField(DataTypes.INTEGER); + final Map map = + esMappingAPI.toMap(contentletWithStringInNumericField(type, "54")); + + assertNotNull("the document must be built, not aborted", map); + final String key = numericKey(type); + assertTrue("the numeric key must be present", map.containsKey(key)); + assertEquals("emitted class must match the natively-stored path", + Long.class, map.get(key).getClass()); + assertEquals(Long.valueOf(54L), map.get(key)); + } + + @Test + public void test_floatColumn_numericString_isIndexed() { + final ContentType type = typeWithNumericTextField(DataTypes.FLOAT); + final Map map = + esMappingAPI.toMap(contentletWithStringInNumericField(type, "54.3")); + + assertNotNull(map); + final String key = numericKey(type); + assertTrue("the float path is in scope too — AC-001 says numeric, not integer", + map.containsKey(key)); + assertEquals("emitted class must match the natively-stored path", + Float.class, map.get(key).getClass()); + assertEquals(Float.valueOf(54.3f), map.get(key)); + } + + /** + * AC-009 stated as a direct comparison rather than a golden value: the same content type, the + * same value, stored once as a String and once as a number, must emit the same entries. + */ + @Test + public void test_convertedString_emitsSameEntriesAsNativeNumber() { + final ContentType type = typeWithNumericTextField(DataTypes.INTEGER); + final String key = numericKey(type); + + final Map fromString = + esMappingAPI.toMap(contentletWithStringInNumericField(type, "54")); + final Map fromNumber = + esMappingAPI.toMap(contentletWithNumberInNumericField(type, 54L)); + + assertEquals("numeric key must be identical, value and class", + fromNumber.get(key), fromString.get(key)); + assertEquals("_dotraw must be identical, padding included", + fromNumber.get(key + DOTRAW), fromString.get(key + DOTRAW)); + } + + // ---------------------------------------------------------------------------------------- + // AC-007 — the _dotraw invariant: padded or absent, never raw text, never unpadded + // ---------------------------------------------------------------------------------------- + + /** + * Pins the invariant on a value that converts. {@code _dotraw} is a {@code keyword} and every + * sort in dotCMS targets it, so fixed-width zero-padding is the only reason ordering by a + * numeric field is numeric rather than lexicographic. + */ + @Test + public void test_convertedValue_dotrawIsZeroPadded() { + final ContentType type = typeWithNumericTextField(DataTypes.INTEGER); + final Map map = + esMappingAPI.toMap(contentletWithStringInNumericField(type, "54")); + + final Object dotraw = map.get(numericKey(type) + DOTRAW); + assertNotNull("_dotraw must be emitted for a convertible value", dotraw); + assertTrue("_dotraw must be zero-padded 19.18, got: " + dotraw, + PADDED_NUMERIC.matcher(String.valueOf(dotraw)).matches()); + assertEquals("0000000000000000054.000000000000000000", dotraw); + } + + /** + * The research R-1 trap, asserted as absence rather than inequality. + * + *

{@code toMap}'s derivation loop synthesizes {@code _dotraw} from the numeric key + * whenever {@code _text} is missing — unpadded. So an implementation that omits + * only {@code _text} while still writing the numeric key produces a malformed {@code _dotraw} + * and silently reorders every listing sorted by that field. Written as + * {@code assertNotEquals("n/a", dotraw)} this test would pass against that bug, which is why + * it asserts the key is not there at all.

+ */ + @Test + public void test_unconvertibleValue_dotrawIsAbsent_notUnpadded() { + final ContentType type = typeWithNumericTextField(DataTypes.INTEGER); + final Map map = + esMappingAPI.toMap(contentletWithStringInNumericField(type, "N/A")); + + final String dotrawKey = numericKey(type) + DOTRAW; + assertFalse("_dotraw must be absent, not raw text and not an unpadded number — got: " + + map.get(dotrawKey), map.containsKey(dotrawKey)); + } + + // ---------------------------------------------------------------------------------------- + // AC-008 — an unconvertible value omits both keys; no fabricated 0 + // ---------------------------------------------------------------------------------------- + + /** + * Both keys, in one test, because they are one atomic change: writing the numeric key alone + * re-creates the unpadded {@code _dotraw} above. + */ + @Test + public void test_unconvertibleValue_omitsBothKeys() { + final ContentType type = typeWithNumericTextField(DataTypes.INTEGER); + final Map map = + esMappingAPI.toMap(contentletWithStringInNumericField(type, "N/A")); + + final String key = numericKey(type); + assertNotNull("the document must still be built", map); + assertFalse("no fabricated 0 under the numeric key — it would match a range query" + + " indistinguishably from a genuine 0", map.containsKey(key)); + assertFalse(map.containsKey(key + DOTRAW)); + assertFalse("_text must not survive either", map.containsKey(key + TEXT)); + } + + /** AC-002: the field degrades, the document does not. */ + @Test + public void test_unconvertibleValue_otherFieldsStillIndexed() { + final ContentType type = typeWithNumericTextField(DataTypes.INTEGER); + final Map map = + esMappingAPI.toMap(contentletWithStringInNumericField(type, "N/A")); + + final String companionKey = (type.variable() + "." + COMPANION_FIELD_VAR).toLowerCase(); + assertTrue("a sibling field must survive one field's failure", + map.containsKey(companionKey)); + assertEquals(COMPANION_VALUE, map.get(companionKey)); + } + + // ---------------------------------------------------------------------------------------- + // AC-005 — regression: a correctly-stored number must not move at all. + // These two must be GREEN before the fix; a red result here means the test is wrong. + // ---------------------------------------------------------------------------------------- + + @Test + public void test_nativeNumber_onIntegerColumn_isUnchanged() { + final ContentType type = typeWithNumericTextField(DataTypes.INTEGER); + final Map map = + esMappingAPI.toMap(contentletWithNumberInNumericField(type, 54L)); + + final String key = numericKey(type); + assertEquals(Long.valueOf(54L), map.get(key)); + assertEquals("0000000000000000054.000000000000000000", map.get(key + DOTRAW)); + } + + @Test + public void test_nativeNumber_dotrawMatchesThePaddedFormat() { + final ContentType type = typeWithNumericTextField(DataTypes.FLOAT); + final Map map = + esMappingAPI.toMap(contentletWithNumberInNumericField(type, 54.3f)); + + final Object dotraw = map.get(numericKey(type) + DOTRAW); + assertTrue("the padded format is the pre-existing contract: " + dotraw, + PADDED_NUMERIC.matcher(String.valueOf(dotraw)).matches()); + } + + // ---------------------------------------------------------------------------------------- + // AC-003 / AC-010 — the report is actionable, and the happy path stays silent + // ---------------------------------------------------------------------------------------- + + /** + * AC-003. One WARN, naming the field and the content type so an operator can act on it — + * and deliberately not naming the value, which is customer content + * (Constitution Principle III: never log sensitive data). + */ + @Test + public void test_unconvertibleValue_logsOneWarnNamingFieldAndType() { + final ContentType type = typeWithNumericTextField(DataTypes.INTEGER); + final Contentlet contentlet = contentletWithStringInNumericField(type, "N/A"); + + final List events = capturingMappingLogs(() -> esMappingAPI.toMap(contentlet)); + + final List warns = eventsAtLevel(events, Level.WARN); + assertEquals("exactly one WARN for one unconvertible field, got: " + messagesOf(warns), + 1, warns.size()); + final String message = warns.get(0).getMessage().getFormattedMessage(); + assertTrue("the WARN must name the field: " + message, + message.contains(NUMERIC_FIELD_VAR)); + assertTrue("the WARN must name the content type: " + message, + message.contains(type.variable())); + assertFalse("the offending value is customer content and must not be logged: " + message, + message.contains("N/A")); + } + + /** + * AC-010. {@code loadFields} runs for every field of every contentlet on every index write, + * so a fix that logs on the happy path would flood the log on every page's {@code sortOrder}. + */ + @Test + public void test_happyPath_logsNothing() { + final ContentType type = typeWithNumericTextField(DataTypes.INTEGER); + final Contentlet nativeNumber = contentletWithNumberInNumericField(type, 54L); + final Contentlet convertibleString = contentletWithStringInNumericField(type, "54"); + + final List events = capturingMappingLogs(() -> { + esMappingAPI.toMap(nativeNumber); + esMappingAPI.toMap(convertibleString); + }); + + final List noisy = new ArrayList<>(eventsAtLevel(events, Level.WARN)); + noisy.addAll(eventsAtLevel(events, Level.ERROR)); + assertTrue("a native number and a convertible String must both map silently, got: " + + messagesOf(noisy), noisy.isEmpty()); + } + + // ---------------------------------------------------------------------------------------- + // AC-006 — neighbouring branches of the edited else-if chain are untouched + // ---------------------------------------------------------------------------------------- + + /** + * The numeric branch sits in an {@code else if} chain with the boolean, date and general + * text branches. This pins its immediate neighbours: they must keep emitting what they + * emitted before, both key and runtime class. + * + *

Category and Relationship fields are covered by {@code ESMappingAPITest} and are + * resolved in {@code loadCategories} / {@code loadRelationshipFields}, i.e. outside this + * chain entirely.

+ */ + @Test + public void test_neighbouringFieldTypes_areUnchanged() { + final Field numericField = new FieldDataGen().type(TextField.class) + .dataType(DataTypes.INTEGER).defaultValue("0") + .velocityVarName(NUMERIC_FIELD_VAR).indexed(true).next(); + // RadioField, not CheckboxField: only Hidden/Radio/Select accept DataTypes.BOOL, which + // is what puts the field on a bool% column and therefore in the branch above the + // numeric one. + final Field boolField = new FieldDataGen().type(RadioField.class) + .dataType(DataTypes.BOOL).defaultValue("false") + .velocityVarName("boolTestField").indexed(true).next(); + final Field dateField = new FieldDataGen().type(DateTimeField.class) + .dataType(DataTypes.DATE).defaultValue(null) + .velocityVarName("dateTestField").indexed(true).next(); + final Field textField = new FieldDataGen().type(TextField.class) + .dataType(DataTypes.TEXT).defaultValue(null) + .velocityVarName(COMPANION_FIELD_VAR).indexed(true).next(); + + final ContentType type = new ContentTypeDataGen() + .field(numericField).field(boolField).field(dateField).field(textField) + .nextPersisted(); + + final Contentlet contentlet = new ContentletDataGen(type.id()) + .setProperty(NUMERIC_FIELD_VAR, 54L) + .setProperty("boolTestField", true) + .setProperty("dateTestField", new Date()) + .setProperty(COMPANION_FIELD_VAR, COMPANION_VALUE) + .nextPersisted(); + + final Map map = esMappingAPI.toMap(contentlet); + // The final document lowercases every key, so build them the same way numericKey does. + final String prefix = (type.variable() + ".").toLowerCase(); + + assertEquals("numeric branch", Long.valueOf(54L), map.get(numericKey(type))); + assertTrue("boolean branch must still emit its key", + map.containsKey(prefix + "booltestfield")); + assertTrue("date branch must still emit its key", + map.containsKey(prefix + "datetestfield")); + assertEquals("general text branch", COMPANION_VALUE, + map.get(prefix + COMPANION_FIELD_VAR.toLowerCase())); + } + + /** + * AC-006, unique-field edge. The SHA-256 entry is written only when the numeric key exists + * ({@code if (field.isUnique() && contentletMap.containsKey(keyName))}), so omitting both + * keys skips it rather than throwing. Asserted so a future refactor of that guard cannot + * turn this into an NPE. + */ + @Test + public void test_uniqueField_unconvertibleValue_skipsShaAndDoesNotThrow() { + final Field uniqueNumericField = new FieldDataGen().type(TextField.class) + .dataType(DataTypes.INTEGER).defaultValue("0").unique(true) + .velocityVarName(NUMERIC_FIELD_VAR).indexed(true).next(); + final Field companionField = new FieldDataGen().type(TextField.class) + .dataType(DataTypes.TEXT).defaultValue(null) + .velocityVarName(COMPANION_FIELD_VAR).indexed(true).next(); + final ContentType type = new ContentTypeDataGen() + .field(uniqueNumericField).field(companionField).nextPersisted(); + + // A unique field is implicitly required, so it must carry a value at save time — the + // field-level default is not enough. The unconvertible value is substituted in memory + // afterwards, as everywhere else in this class. + final Contentlet contentlet = new ContentletDataGen(type.id()) + .setProperty(COMPANION_FIELD_VAR, COMPANION_VALUE) + .setProperty(NUMERIC_FIELD_VAR, 1L) + .nextPersisted(); + contentlet.setStringProperty(NUMERIC_FIELD_VAR, "N/A"); + + final Map map = esMappingAPI.toMap(contentlet); + + final String key = numericKey(type); + assertFalse("the numeric key is omitted", map.containsKey(key)); + assertFalse("so the unique SHA-256 entry is skipped, not computed over nothing", + map.containsKey(key + "_sha256")); + assertTrue("and the rest of the contentlet still indexes", + map.containsKey((type.variable() + "." + COMPANION_FIELD_VAR).toLowerCase())); + } + + // ---------------------------------------------------------------------------------------- + // Log capture + // ---------------------------------------------------------------------------------------- + + private static List capturingMappingLogs(final Runnable action) { + final CapturingAppender appender = new CapturingAppender(); + appender.start(); + final Logger logger = (Logger) LogManager.getLogger(ESMappingAPIImpl.class); + logger.addAppender(appender); + try { + action.run(); + } finally { + logger.removeAppender(appender); + appender.stop(); + } + return appender.events; + } + + private static List eventsAtLevel(final List events, final Level level) { + final List matching = new ArrayList<>(); + for (final LogEvent event : events) { + if (level.equals(event.getLevel())) { + matching.add(event); + } + } + return matching; + } + + private static String messagesOf(final List events) { + final List messages = new ArrayList<>(); + for (final LogEvent event : events) { + messages.add(event.getLevel() + ": " + event.getMessage().getFormattedMessage()); + } + return messages.toString(); + } + + private static class CapturingAppender extends AbstractAppender { + + private final List events = new ArrayList<>(); + + CapturingAppender() { + super("NumericFieldCapturingAppender", null, null, true, null); + } + + @Override + public void append(final LogEvent event) { + events.add(event.toImmutable()); + } + } + + // ---------------------------------------------------------------------------------------- + // End-to-end through a real index: the sort invariant and the absent-not-zero contract. + // + // These two are the reason the _dotraw finding matters. Everything above proves the emitted + // MAP is right; only a real query proves the resulting ORDER is right. + // ---------------------------------------------------------------------------------------- + + /** + * Indexes three convertible documents plus one whose value cannot be converted, then sorts by + * that field ascending and descending. + * + *

Asserted on inode order, not on values: {@code search} returns + * contentlets hydrated from the database, so their field values are the persisted ones and say + * nothing about what the index holds.

+ * + *

The unconvertible document is deliberately seeded with {@code 50} — between {@code 40} + * and {@code 300}. If the index still carried a value for it, it would sort into that gap. + * With both keys omitted it has no sort key and the engines place it last, leaving the three + * convertible documents in strict numeric order. That is the invariant: raw text in + * {@code _dotraw} would sort it after everything ascending ({@code 'N'} = 0x4E > + * {@code '9'} = 0x39) but an unpadded number would sort it before {@code 9}, silently + * reordering the listing.

+ */ + @Test + public void test_unconvertibleDocument_doesNotDisturbSortOrder() throws Exception { + final ContentType type = typeWithNumericTextField(DataTypes.INTEGER); + + final String five = indexedWithNumber(type, 5L).getInode(); + final String forty = indexedWithNumber(type, 40L).getInode(); + final String threeHundred = indexedWithNumber(type, 300L).getInode(); + // Seeded at 50 so that a leaked index value would land it between 40 and 300. + final String unconvertible = indexedWithUnconvertibleString(type, 50L, "N/A"); + + final String sortField = + type.variable().toLowerCase() + "." + NUMERIC_FIELD_VAR.toLowerCase(); + final String query = "+contentType:" + type.variable(); + + final List ascending = inodesInSortOrder(query, sortField + " asc"); + assertEquals("the three convertible documents must be in numeric order, with the " + + "unconvertible one carrying no sort key at all: " + ascending, + List.of(five, forty, threeHundred), withoutInode(ascending, unconvertible)); + // Deliberately NOT asserting where the unconvertible document itself lands. Both + // `nextPersisted()` and `addContentToIndex` enqueue their index write as a commit + // listener while a transaction is open, so which of the two wins is not deterministic: + // if the save's listener runs last it overwrites the document with the clean database + // version (the 50 seed) and the position means nothing. What IS deterministic — and is + // the invariant that matters — is that the convertible documents keep strict numeric + // order, asserted above and below. The absence of a fabricated value in the index is + // proven separately by `test_unconvertibleDocument_doesNotMatchARangeQuery`. + + final List descending = inodesInSortOrder(query, sortField + " desc"); + assertEquals("and descending must be the exact reverse: " + descending, + List.of(threeHundred, forty, five), withoutInode(descending, unconvertible)); + } + + /** + * The unconvertible document must not answer a range query that happens to include zero — + * which is what emitting a {@code 0} sentinel under the numeric key would have caused, with + * only a WARN to reveal it. Seeded at {@code 50}, outside the queried range, so a leaked + * index value cannot be mistaken for the fabricated zero. + */ + @Test + public void test_unconvertibleDocument_doesNotMatchARangeQuery() throws Exception { + final ContentType type = typeWithNumericTextField(DataTypes.INTEGER); + + final Contentlet inRange = indexedWithNumber(type, 7L); + indexedWithUnconvertibleString(type, 50L, "N/A"); + + final String field = type.variable().toLowerCase() + "." + NUMERIC_FIELD_VAR.toLowerCase(); + final List hits = APILocator.getContentletAPI().search( + "+contentType:" + type.variable() + " +" + field + ":[0 TO 10]", + 100, 0, null, APILocator.systemUser(), false); + + assertEquals("only the genuine in-range document may match: " + inodesOf(hits), + 1, hits.size()); + assertEquals(inRange.getInode(), hits.get(0).getInode()); + } + + private static Contentlet indexedWithNumber(final ContentType type, final Number value) + throws Exception { + return new ContentletDataGen(type.id()) + .setProperty(COMPANION_FIELD_VAR, COMPANION_VALUE) + .setProperty(NUMERIC_FIELD_VAR, value) + .setPolicy(IndexPolicy.WAIT_FOR) + .nextPersisted(); + } + + /** + * The bad value cannot be saved through the API — {@code validateContentlet} rejects a value + * whose type contradicts its column with {@code [BADTYPE]}, which is exactly why no + * user-facing path can create this data. So the contentlet is persisted with a valid + * {@code seed} and then re-indexed with the bad value substituted in memory. + * {@code addContentToIndex} indexes the instance it is handed without reloading it, so the + * substituted value is what reaches the index — while the database keeps {@code seed}. + * + * @return the inode of the re-indexed contentlet + */ + private static String indexedWithUnconvertibleString(final ContentType type, + final Number seed, final String badValue) throws Exception { + final Contentlet contentlet = indexedWithNumber(type, seed); + contentlet.setStringProperty(NUMERIC_FIELD_VAR, badValue); + contentlet.setIndexPolicy(IndexPolicy.WAIT_FOR); + APILocator.getContentletIndexAPI().addContentToIndex(contentlet, false); + return contentlet.getInode(); + } + + private static List inodesInSortOrder(final String query, final String sortBy) + throws Exception { + return inodeListOf(APILocator.getContentletAPI() + .search(query, 100, 0, sortBy, APILocator.systemUser(), false)); + } + + private static List withoutInode(final List inodes, final String excluded) { + final List remaining = new ArrayList<>(inodes); + remaining.remove(excluded); + return remaining; + } + + private static List inodeListOf(final List contentlets) { + final List inodes = new ArrayList<>(); + for (final Contentlet contentlet : contentlets) { + inodes.add(contentlet.getInode()); + } + return inodes; + } + + private static String inodesOf(final List contentlets) { + return inodeListOf(contentlets).toString(); + } +} diff --git a/specs/37272-textfield-numeric-column/spec.md b/specs/37272-textfield-numeric-column/spec.md new file mode 100644 index 000000000000..71e494787d85 --- /dev/null +++ b/specs/37272-textfield-numeric-column/spec.md @@ -0,0 +1,384 @@ +# Issue Resolution Specification: A TextField stored in a numeric column makes the whole contentlet unindexable + +**Feature Branch**: `37272-textfield-numeric-column` + +**Created**: 2026-09-03 + +**Status**: Draft + +**Type**: Issue / Bug Resolution + +**Related GitHub Issue**: #37272 + +**Input**: User description: "37272" + +## Problem Statement *(mandatory)* + +A contentlet silently disappears from the search index. The save succeeds, the UI reports +nothing, and the reindex shows only an aggregate failure count with no field or content-type +attribution. The only trace is a WARN in the log. + +The cause is a field declared as **Text** whose stored value is a Java `String`, but whose backing +storage column is numeric (`integer1`, `float3`, …). At index time the serialization branch is +chosen by the **storage column name** rather than by the value, so the String is handed to a number +formatter, which throws. The exception is then rethrown, and **the entire contentlet is discarded** +— not just the offending field. + +**Severity / Impact**: Medium. It affects only contentlets whose value for such a field is a +String — but for those, the document is absent from the index after any reindex, permanently and +invisibly. Observed during ES→OpenSearch migration testing: of 370 contentlets refreshed, 2 were +lost this way (content types `RptTopHits` and `Trending`, field `viewWeekTotal`). + +**Not a regression.** Both defects are present verbatim in commit `e8ef584ec9` — *"initial trunk +import"*, 2012-03-22, the oldest commit in the repository. The ES→OpenSearch work neither +introduced nor worsened it; the bulk refresh simply pushed enough documents through the same path +to make it visible. + +## Reproduction *(mandatory)* + +**Environment**: `dotcms/dotcms:trunk`, PostgreSQL 16, ~1.55 M contentlets, OpenSearch 1.3.20 + +3.4.0 in `PHASE_1_DUAL_WRITE_ES_READS`. Not phase- or engine-specific — the defect is in the +contentlet→document mapper shared by both engines, so it reproduces on Elasticsearch-only installs. + +**The field modelling is legitimate and supported.** `TextField.acceptedDataTypes()` includes +`INTEGER` and `FLOAT`, so "Text field, Value type: Whole Number" is a valid UI choice. dotCMS ships +content types modelled this way: `htmlpageasset.sortOrder` and `Vanity URL.order` are both +`ImmutableTextField` + `DataTypes.INTEGER` + `indexed(true)`. The modelling is not the defect. + +**What actually decides the failure is the Java class of the value at index time**: + +| stored `contentlet_as_json` | value in the contentlet map | outcome | +|---|---|---| +| `{"type":"Long","value":54}` | `Long` | indexed correctly (padded `_dotraw`) | +| `{"type":"Text","value":"54"}` | `String` | 💥 whole contentlet lost | + +`DecimalFormat.format(Object)` accepts only `Number`; with a String it throws **always**, even when +the content is numeric. This is why `htmlpageasset.sortOrder` never fails (the core sets it via +`setLongProperty`) and `viewWeekTotal` fails for 100% of its contentlets. + +**Steps to Reproduce (A — the defect itself, deterministic; this is the Red test)**: + +1. Create a content type with a **Text** field, Value type **Whole Number**, indexed. +2. Build the `Contentlet` and set the value raw, bypassing `setContentletProperty`: + `contentlet.setStringProperty("", "54");` +3. Call `APILocator.getContentletMappingAPI().toMap(contentlet)`. + +**Steps to Reproduce (B — the production data state, for end-to-end QA)**: + +1. Write the String value into the stored JSON directly: + ```sql + UPDATE contentlet + SET contentlet_as_json = jsonb_set(contentlet_as_json, + '{fields,}', '{"type": "Text", "value": "54"}'::jsonb) + WHERE inode = ''; + ``` +2. Flush caches, then reindex that contentlet. + +**Expected Behavior**: The contentlet is indexed. The field is serialized in a way that suits its +value, and every other field of the document is indexed regardless. + +**Actual Behavior**: The contentlet is absent from the index. The log shows, at WARN level: + +``` +WARN business.ESMappingAPIImpl - Error indexing field: viewWeekTotal of contentlet: +java.lang.IllegalArgumentException: Cannot format given Object as a Number + at java.base/java.text.DecimalFormat.format(DecimalFormat.java:584) + at com.dotcms.content.elasticsearch.business.ESMappingAPIImpl.loadFields(...) + at com.dotcms.content.elasticsearch.business.ESMappingAPIImpl.toMap(...) +``` + +**Reproducibility**: Always, for every contentlet holding a String value in such a field. + +## Scope of Investigation *(mandatory)* + +- **Affected area**: Search indexing — the contentlet→index-document mapping, plus the failure + reporting surfaced to the operator during a reindex or bulk refresh. +- **Suspected surface**: `com.dotcms.content.elasticsearch.business.ESMappingAPIImpl.loadFields` + (`:990`; branch at `:1113-1116`, throwing line `:1118`, WARN+rethrow at `:1143-1145`). Despite + the `elasticsearch` package name it is the **single shared mapper for both engines** — + `com.dotcms.content.index.opensearch.OSBulkHelper:250` calls the same `toMap()`. It reads legacy + field metadata (`com.dotmarketing.portlets.structure.model.Field`), so the plan must confirm + legacy impact on the field model. +- **Related known decisions**: The ES→OpenSearch migration — the mapper is shared, so a change to + what is emitted per field changes both engines' documents at once. The plan formally consults + `dotCMS/platform-adrs`. + +## Root-Cause Hypothesis + +Four layers make a decision about such a field, and three of them key off the storage column: + +| layer | keys off | behavior for `integer1` | +|---|---|---| +| write (`FieldHandlerStrategyFactory`) | `dbColumn()` | `integerStrategy` coerces the value to `Long` | +| read (`ContentletJsonAPIImpl.getValue`, `:444`) | **nothing** | returns the value exactly as the JSON declares it | +| index (`ESMappingAPIImpl.loadFields`) | `getFieldContentlet()` | numeric branch → `DecimalFormat.format(value)` | +| index mapping (`ESMappingUtilHelper:429`) | `dataType()` | maps the field as `long` | + +The read layer validates nothing. The write path guarantees a `Long`, but a value already present +in the JSON enters unchecked. Two independent defects then compound: + +1. **Wrong discriminator.** `loadFields` picks the branch by column name. `"integer1"` starts with + `integer`, so a String reaches `numFormatter.format(valueObj)` and throws. + +2. **A single bad field kills the whole document.** The per-field `catch` logs at **WARN** — the + level that says "recoverable, carry on" — and then rethrows as `DotDataException`, aborting the + contentlet. The date branches in the same method do the opposite and degrade to `toString()`. + +Defect 2 is what turns a mis-typed field into a lost document; it would have contained defect 1 on +its own. + +## Fix Scope & Non-Goals *(mandatory)* + +**In scope**: + +- **Handle the parse failure at index time** for a Text field that carries a numeric value, + converting best-effort instead of assuming the stored value already matches the column: + - A shared best-effort conversion helper (alongside the existing `NumberUtil.toInt` / + `NumberUtil.toLong` / `NumberUtil.pad`, following the same `Supplier defaultOne` idiom). + **It must cover the floating-point path, not only integers.** The failing branch + (`ESMappingAPIImpl.java:1113-1116`) matches `float%` *and* `integer%` columns, and `NumberUtil` + currently has no `toFloat`/`toDouble` — only `toInt`, `toLong`, `toBoolean`, `asInt`. A `"54.3"` + in a `float`-backed Text field is in scope (AC-001 says *numeric* column) and must convert + rather than fall to the unconvertible path. Note also that `NumberUtil.pad(Number)` takes a + `Number`, so it cannot be used on a value that did not convert. + - **Index (`loadFields`)**: convert the value; if it converts, the numeric branch behaves exactly + as today (a stored `"54"` produces a document identical to a correctly-stored `54`, padding + included). If it does not convert, **both keys are omitted** for that field and a WARN names + the field and content type — see *Resolved Decisions → C-3* for why, and for the + `_dotraw`-padding invariant that rules out writing the raw text there. + Scope is the index path only: a Text field that carries a numeric value must survive the parse, + whichever way the value happens to be stored. +- Make a per-field serialization failure **non-fatal to the document**: the field degrades (or is + skipped) and the rest of the contentlet is still indexed, consistent with the date branches in + the same method. +- Make the failure **actionable**: the report names the field (and content type), and the log level + matches the outcome — WARN when the document is still indexed, ERROR when it is aborted. + +**Explicitly out of scope / non-goals**: + +- **Changing the index mapping generator** so it keys off the declared field type instead of + `dataType()`. That changes the mapping of existing indices, forces a reindex, and is + rollback-unsafe. +- **Repairing existing mis-typed stored values** (rewriting `contentlet_as_json`). The fix must + make these contentlets index correctly *as they are*. +- **Validating field-type/storage-column consistency at content-type save time** (the issue's + "consider" bullet). That validation already exists and already says this modelling is legal: + `FieldFactoryImpl:478` rejects a field whose `dataType()` is not in `acceptedDataTypes()`, and + `TextField.acceptedDataTypes()` deliberately includes `INTEGER` and `FLOAT`. Forbidding the + combination means removing them from that list, which would break dotCMS's own built-in content + types at bootstrap (`PageContentType:125-127`, `VanityUrlContentType:85-88`, + `ContentTypeInitializer:123` all build `ImmutableTextField` + `DataTypes.INTEGER`). The + consistency worth enforcing is **value vs. data type**, not field type vs. data type — and the + preventive lever for that is the serialization layer, also out of scope (next bullet). +- **Changing how contentlets are serialized** — making `Field.fieldValue()` pick the JSON value + type from the field's `dataType()` instead of from `value instanceof`. That is the preventive + counterpart to this fix and would also close the `ImportStarterUtil` route (the starter import + saves through `ESContentFactoryImpl`, which re-serializes every contentlet via + `contentletJsonAPI.toJson(...)`, `:1869-1872`). It is deliberately **out of scope**: it changes + every save of every contentlet in the system, which is a different blast radius and deserves its + own Red/Green cycle and its own review. Recorded under Regression Risk → *Identified risk*. +- **Hardening `ImportStarterUtil` directly.** Same reasoning — named as a known gap, not fixed + here. +- **Backfilling the already-lost documents.** A normal reindex after the fix recovers them. +- Rewriting `loadFields` or the legacy `Field` model wholesale. Progressive enhancement only. +- Per-document / per-engine failure reporting in the reindex UI beyond naming the field — that is + the companion issue referenced in #37272. + +## Regression Risk *(mandatory)* + +- **Blast radius**: `loadFields` runs for **every field of every contentlet on every index write** — + the hottest path in indexing, shared by both engines. Built-in content types use exactly this + field shape (`htmlpageasset.sortOrder`, `Vanity URL.order`), and their `_dotraw` is a + zero-padded string (`0000000000000000054.000000000000000000`) precisely so that lexicographic + sorting equals numeric sorting. **Routing those through the text branch would break page + ordering on every installation** — hence the value-based discriminator rather than a + type-based one. + + **Why `_dotraw` is the load-bearing key, in full** (this is what makes C-3 below non-negotiable): + + 1. `loadFields` writes the padded string to `keyNameText` = `_text` + (`ESMappingAPIImpl.java:1009`, `:1118`). + 2. That entry is lowercased (`:590`) and **renamed** `_text` → `_dotraw` (`:598`). + 3. By default (`CREATE_TEXT_INDEX_FIELD_FOR_NON_TEXT_FIELDS=false`) the `_text` key is then + dropped (`:607-610`), so `_dotraw` is the **only** survivor of the pair. + 4. `*_dotraw` is mapped `keyword` (`es-content-mapping.json`, `template_1`) — lexicographic, + not numeric. + 5. **Every** sort in dotCMS targets it: `addBuilderSort` appends `_dotraw` to whatever field + name the caller passes (`ContentFactoryIndexOperationsES.java:412-413`, and the OpenSearch + counterpart). + + So the zero-padding is not cosmetic — it is the *only* reason ordering by a numeric field works + at all. Anything unpadded written to that key silently reorders results. +- **Backward compatibility**: The emitted values feed index mappings and existing queries (range, + sort, exact match via `_dotraw`, VTL/GraphQL/Elastic search by field). Changing the *type* of an + emitted value is potentially rollback-unsafe during a rolling deploy. Making a previously-fatal + failure non-fatal is safe in that direction — it can only add documents that were missing. +- **Data considerations**: Documents lost to this defect reappear on the next reindex. No schema or + DB migration. Note the generated mapping for such a field is numeric — `long` for an `integer%` + column, **`double` for a `float%` column** (`ESMappingUtilHelper.java:404-406`, + `DataTypes.INTEGER → long`, `DataTypes.FLOAT → double`). Emitting a non-numeric String under the + numeric key would produce a mapper parsing exception at the engine, i.e. the same lost document + one layer down. To be unambiguous about what that forbids: the unconvertible path must not write + **a non-numeric string** under the numeric key, and per C-3 below it does not write the numeric + key at all. + +### Identified risk: unvalidated ingestion path (`ImportStarterUtil`) + +**No supported user-facing path on trunk can create the bad value.** Every save funnels through +`ContentletAPI.setContentletProperty` → `integerStrategy`, which coerces `"54"` → `54L` and rejects +a non-numeric String with `DotNumericFieldException`. This was verified for: + +| path | where | result | +|---|---|---| +| New content editor (REST / workflow fire) | `MapToContentletPopulator:279` | coerced | +| Legacy editor (Struts/dojo) | `ContentletWebAPIImpl`, `ContentletAjax` | coerced | +| Content Import portlet **and** the new import job | `ImportUtil:3460` (both `ImportContentletsAction` and `ImportContentletsProcessor` funnel here) | coerced; non-numeric values reject the line | + +`MapToContentletPopulator` has exactly one `setStringProperty` bypass (`:259`) and it applies only +to Category fields. + +**The starter / site-export import does not go through the API at all:** + +```java +// ImportStarterUtil.java:894 +APILocator.getContentletJsonAPI().toMutableContentlet(cont); // JSON → map, no coercion +// ImportStarterUtil.java:782 +FactoryLocator.getContentletFactory().save((Contentlet) obj); // factory directly — no checkin, no validation +``` + +`toMutableContentlet` (`ContentletJsonAPIImpl:244`) reuses the same `getValue()` that returns the +value exactly as the JSON declares it. A starter carrying `{"type":"Text","value":"54"}` for a +field now backed by a numeric column is written to the database **verbatim**, with nothing +reconciling the JSON value type against the field's `dataType`. + +This is the most plausible provenance of the two observed rows: a value stored as a String by an +older instance, exported, and re-imported into trunk. It also explains why the defect cannot be +reproduced through the UI, and why a dataset copy rebuilt by re-saving content through the API no +longer contains the case. + +**Risk to this fix**: none directly — the `loadFields` change makes such data index correctly +regardless of how it arrived. **Risk to the system**: `ImportStarterUtil` remains a route by which +values whose type contradicts their field's storage column enter the database unvalidated. Named +here so the plan can decide whether it warrants separate treatment; **not fixed by this change**. + +**To confirm the provenance** on the instance where it was observed: + +```sql +SELECT inode, contentlet_as_json -> 'fields' -> 'viewWeekTotal' AS stored, integer1 +FROM contentlet WHERE inode = ''; +``` + +Install-wide detector (any numeric column whose stored JSON type is not the matching numeric type): + +```sql +SELECT st.velocity_var_name AS ct, f.velocity_var_name AS field, + f.field_type, f.field_contentlet, kv.value ->> 'type' AS json_type, count(*) +FROM contentlet c +JOIN structure st ON st.inode = c.structure_inode +JOIN field f ON f.structure_inode = st.inode +CROSS JOIN LATERAL jsonb_each(c.contentlet_as_json -> 'fields') AS kv(key, value) +WHERE kv.key = f.velocity_var_name + AND ( (f.field_contentlet LIKE 'integer%' AND kv.value ->> 'type' <> 'Long') + OR (f.field_contentlet LIKE 'float%' AND kv.value ->> 'type' <> 'Float') ) +GROUP BY 1,2,3,4,5 ORDER BY 6 DESC; +``` + +(Contentlets with a null `contentlet_as_json` read from the legacy columns and do not suffer this +defect; `jsonb_each(NULL)` drops them from the result silently.) + +## Acceptance & Verification *(mandatory)* + +- **AC-001**: A contentlet holding a String value in a Text field backed by a numeric column is + indexed successfully, on both the Elasticsearch and OpenSearch write paths. +- **AC-002**: When a single field cannot be serialized, the remaining fields of that contentlet are + still indexed; the document is present in the index rather than absent. +- **AC-003**: The failure report/log for an unserializable field names the **field** (and content + type), not only the contentlet inode. +- **AC-004**: The log level matches the outcome — WARN when the document is still indexed, ERROR + when it is aborted. No WARN-then-rethrow. +- **AC-005 (regression, critical)**: A field of the same shape whose value **is** a `Number` — + `htmlpageasset.sortOrder`, `Vanity URL.order` — produces **identical emitted map entries**, + including the zero-padded `_dotraw`. Page ordering by `sortOrder` is unchanged. +- **AC-006 (regression)**: Correctly-modelled `Integer`, `Float`/`Decimal`, `Boolean`, `Date`, + `DateTime`, `Checkbox`/`Multi-Select`, `Key-Value`, `Tag`, `Category` and `Relationship` fields + produce **identical emitted map entries**, including the unique-field SHA-256 entry. +- **AC-007 (`_dotraw` invariant, non-negotiable)**: For a Text field on a numeric column, + `_dotraw` is **either the 19.18 zero-padded numeric string or absent** — never the raw + text, and never an unpadded number. Asserted directly: index a contentlet whose value does not + convert, then sort a result set by that field ascending and descending and confirm the ordering + of the *convertible* documents is unchanged in both directions. (Rationale and the full + `_text` → `_dotraw` → `keyword` → sort chain: Regression Risk → *Blast radius*.) +- **AC-008**: A value that cannot be converted to a number results in **both** keys being omitted + for that field — no numeric key, no `_dotraw` — the rest of the contentlet indexed, and one WARN + naming the field and content type. Specifically: a range query on that field does not match the + document (no false `0`), and the document sorts last on that field, which is the engines' + defined behavior for a missing sort key. +- **AC-009**: A stored String that *is* numeric produces **identical emitted map entries** to the + same contentlet stored as a number — including the zero-padded `_dotraw` — for an `integer%` + column (`"54"`) **and** a `float%` column (`"54.3"`). The assertion is at the emitted-`Map` + level, so the converted value must be of the **same runtime class** as the natively-stored path + produces: `Integer 54` and `Long 54` are not `equals`, and a test that ignores this passes or + fails for the wrong reason. +- **AC-010**: The happy path stays silent. A natively-stored number (`htmlpageasset.sortOrder`) and + a convertible String both produce **no WARN at all** — the fix must not start logging on every + page's `sortOrder`. + +- **Verification method**: + - **Unit** — a focused test over `loadFields` across (declared type × storage column × value + class), asserting the emitted map entries. The matrix must include **both** numeric columns — + `integer%` and `float%` — and both a convertible (`"54"`, `"54.3"`) and an unconvertible + (`"N/A"`) String. The `TextField`-on-`integer1`-with-String case must fail first (Red). + Assert runtime class, not just numeric equality (AC-009). + - **Integration** — `dotcms-integration`, a `*Test` class registered in the matching + `@SuiteClasses` suite: content type with a Text field on a numeric column, contentlet carrying + a String value, index it, assert the document is retrievable and the other fields populated. + Run with `./mvnw verify -pl :dotcms-integration -Dcoreit.test.skip=false -Dit.test=`. + - **Regression** — assert `htmlpageasset.sortOrder` still emits the zero-padded `_dotraw`, and + assert the sort-ordering invariant of AC-007 end-to-end (order of convertible documents + unchanged, asc and desc, with an unconvertible document present in the index). + - **Manual** — apply reproduction B on a seeded instance, run + `POST /api/v1/content/_bulkrefresh`, confirm zero failures for the affected content types. + +## Assumptions + +- A Text field on a numeric column is a **legitimate existing state** to be tolerated, not an + invalid state to be rejected. The fix makes it index correctly rather than unrepresentable. +- The value in such a field may be either a `Number` or a `String`, and both must index. +- No customer data appears in this spec: content-type and field names are the internal ones from + the migration-testing instance named in the issue. + +## Resolved Decisions + +- **C-1 — how the value is emitted.** Convert best-effort rather than discriminating by column or + by declared type. A type-based rule would route `htmlpageasset.sortOrder` through the text branch + and break page ordering on every installation. +- **C-2 — where consistency is enforced.** Not in this fix. Content-type-save-time validation is + the wrong lever (see non-goals). The right preventive lever is the serialization layer + (`Field.fieldValue()` respecting `dataType()`), but that touches every save in the system and is + tracked separately under *Identified risk*. **This fix handles the parse failure where it + surfaces — at index time — so that a Text field carrying a numeric value indexes correctly no + matter how the value was stored.** +- **C-3 — what an unconvertible value emits.** Revised after review of PR #37393. The two keys are + two separate decisions, and an earlier revision of this spec got one of them wrong. + + **`_dotraw` — forced, not chosen.** It must be padded or absent, never the raw text. `_dotraw` is + the `keyword` that *every* sort targets, and the padding is the only reason lexicographic order + equals numeric order (chain and citations in Regression Risk → *Blast radius*). Writing `"N/A"` + there puts the document after every padded digit string (`'N'` = 0x4E > `'9'` = 0x39) and + silently reorders any listing sorted by that field — with no signal to the operator. The earlier + revision's "original text under `_dotraw`, so the real value stays visible and searchable" would + have done exactly that. The value is not lost by omitting it: it remains in the database, and + the WARN names the field. + + **The numeric key — a real choice, made here.** **Omit it** rather than emit `0`. A `0` is + indistinguishable from a genuine `0` in range queries, sorts and aggregations + (`field:[0 TO 10]` matches a document whose value is actually `"N/A"`), and only the WARN + reveals the difference. Omission means the field is simply unset for that one document: no false + match, and the engines' documented missing-key behavior applies. The alternative — `0` for + document/mapping consistency — was weighed and rejected because a false in-range hit is a + correctness bug while an unset field is an honest absence. + + **This supersedes** the earlier decision ("unconvertible values index as `0` under the numeric + key, with the original text preserved in `_dotraw`"). The `_dotraw` half is a correctness fix; + the numeric-key half is a judgment call and the plan may overturn it, but only with the reason + recorded here.