From 8d660a9ee0fc7509adafb990c60d3e8eeeebda7e Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Thu, 3 Sep 2026 17:21:15 -0600 Subject: [PATCH 1/3] Spec: TextField in a numeric column makes the whole contentlet unindexable (#37272) Issue-resolution spec only (PR 1 of 2). No implementation. loadFields picks the serialization branch by storage column name rather than by the value, so a Text field whose value is a String but whose column is numeric reaches DecimalFormat.format() and throws. The per-field catch logs at WARN and then rethrows, so one bad field discards the whole contentlet. Both defects date to the initial trunk import (2012); the ES->OpenSearch work only made them visible. Co-Authored-By: Claude Opus 5 (1M context) --- specs/37272-textfield-numeric-column/spec.md | 317 +++++++++++++++++++ 1 file changed, 317 insertions(+) create mode 100644 specs/37272-textfield-numeric-column/spec.md diff --git a/specs/37272-textfield-numeric-column/spec.md b/specs/37272-textfield-numeric-column/spec.md new file mode 100644 index 000000000000..66a32df7ff24 --- /dev/null +++ b/specs/37272-textfield-numeric-column/spec.md @@ -0,0 +1,317 @@ +# 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). + - **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, emit `0` under the numeric key — keeping the document + consistent with the field's `long` mapping — and the **original text** under `_dotraw`, so the + real value stays visible and searchable. Log a WARN naming the field and content type. + 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. +- **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 index mapping for such a field is `long`: 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 — so the text-degradation path must not write the numeric key. + +### 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 a byte-identical index document, + 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 byte-identical index documents, including the unique-field SHA-256 entry. +- **AC-007**: A value that cannot be converted to a number is indexed as `0` under the numeric key + (so it cannot trigger a mapper parsing exception against the field's `long` mapping) while + `_dotraw` carries the original text, and a WARN names the field and content type. +- **AC-008**: A stored String that *is* numeric (`"54"`) produces an index document identical to + the same contentlet stored as a number — including the zero-padded `_dotraw`. + +- **Verification method**: + - **Unit** — a focused test over `loadFields` across (declared type × storage column × value + class), asserting the emitted map entries. The `TextField`-on-`integer1`-with-String case must + fail first (Red). + - **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`. + - **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.** +- **Unconvertible values index as `0`** under the numeric key, with the original text preserved in + `_dotraw` and a WARN naming the field and content type. From a95a7237784b63274215402e787aa05bdb8a09b0 Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Mon, 7 Sep 2026 15:00:21 -0600 Subject: [PATCH 2/3] docs(spec): resolve PR review feedback on the unindexable-TextField spec (#37272) Addresses the four points raised in review of #37393, plus one they surfaced indirectly and which turned out to be the more consequential of the two. The _dotraw key, not the numeric key, is load-bearing. loadFields writes the padded string to _text (:1009, :1118); that entry is lowercased (:590) and renamed _text -> _dotraw (:598), and the _text key is then dropped by default (:607-610), so _dotraw is the only survivor. It is mapped keyword, and addBuilderSort appends _dotraw to every sort field (ContentFactoryIndexOperationsES:412-413). The zero-padding is therefore the only reason ordering by a numeric field works. The previous AC-007 wrote the raw text there, which silently reorders any listing sorted by that field. Changes: - AC-007 is now the _dotraw invariant: padded or absent, never raw text, with an end-to-end sort assertion. Forced by the mapping, not a design choice. - AC-008 decides the numeric key separately: omit rather than emit 0, since a 0 is indistinguishable from a genuine 0 in ranges, sorts and aggregations. Recorded as C-3 with the rejected alternative and its reason. - Best-effort conversion must cover the float path: the failing branch matches float% and integer%, and NumberUtil has no toFloat/toDouble. - Mapping is long for integer% but double for float% - corrected throughout. - "byte-identical" -> "identical emitted map entries", with the Integer 54 vs Long 54 class-identity trap called out (AC-009). - New AC-010: the happy path must emit no WARN at all. - Disambiguated the Data-considerations sentence that could be read as either "must not write a non-numeric string to the numeric key" or "must not write the numeric key at all". Spec only - no implementation. Co-Authored-By: Claude Opus 5 (1M context) --- specs/37272-textfield-numeric-column/spec.md | 103 +++++++++++++++---- 1 file changed, 85 insertions(+), 18 deletions(-) diff --git a/specs/37272-textfield-numeric-column/spec.md b/specs/37272-textfield-numeric-column/spec.md index 66a32df7ff24..71e494787d85 100644 --- a/specs/37272-textfield-numeric-column/spec.md +++ b/specs/37272-textfield-numeric-column/spec.md @@ -135,11 +135,17 @@ its own. 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, emit `0` under the numeric key — keeping the document - consistent with the field's `long` mapping — and the **original text** under `_dotraw`, so the - real value stays visible and searchable. Log a WARN naming the field and content type. + 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 @@ -187,14 +193,34 @@ its own. 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 index mapping for such a field is `long`: 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 — so the text-degradation path must not write the numeric key. + 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`) @@ -271,26 +297,45 @@ defect; `jsonb_each(NULL)` drops them from the result silently.) - **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 a byte-identical index document, + `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 byte-identical index documents, including the unique-field SHA-256 entry. -- **AC-007**: A value that cannot be converted to a number is indexed as `0` under the numeric key - (so it cannot trigger a mapper parsing exception against the field's `long` mapping) while - `_dotraw` carries the original text, and a WARN names the field and content type. -- **AC-008**: A stored String that *is* numeric (`"54"`) produces an index document identical to - the same contentlet stored as a number — including the zero-padded `_dotraw`. + 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 `TextField`-on-`integer1`-with-String case must - fail first (Red). + 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`. + - **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. @@ -313,5 +358,27 @@ defect; `jsonb_each(NULL)` drops them from the result silently.) 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.** -- **Unconvertible values index as `0`** under the numeric key, with the original text preserved in - `_dotraw` and a WARN naming the field and content type. +- **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. From 1ea0f8d0002d77ce401c7e9324eba2df32f3bf7c Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Tue, 8 Sep 2026 15:06:55 -0600 Subject: [PATCH 3/3] fix(index): a Text field on a numeric column no longer makes the contentlet unindexable (#37272) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. Handing it to DecimalFormat.format() threw IllegalArgumentException, and since the per-field catch rethrows, the whole contentlet was lost from the index — from both engines at once, in every migration phase, because the document is rendered once before the provider fan-out. loadNumericField now handles the branch. A value that is already a Number takes the identical pre-existing path — same object, same formatter call — so no document for correctly-stored content moves. A String is converted best-effort against the column's own type via the new NumberUtil.toLongOrEmpty / toFloatOrEmpty, which report failure instead of substituting a default. When the value cannot be represented numerically, BOTH keys are omitted. That pairing is forced, not stylistic: toMap derives _dotraw from _text and falls back to the bare key when it is absent, so writing the numeric key alone yields an UNPADDED _dotraw. Since _dotraw is a keyword and every sort resolves through it (addBuilderSort appends the suffix to whatever field name the caller passes), fixed-width zero padding is the only reason ordering by a numeric field is numeric rather than alphabetical. An unpadded entry silently reorders results. The numeric key is omitted rather than defaulted to 0 because a 0 is indistinguishable from a genuine 0 in range queries, sorts and aggregations, with only a WARN to reveal the difference. Deliberately out of scope, now tracked in #37462: the surrounding per-field catch still rethrows, so any other unserializable field still costs the whole document. Changing that alters how indexing fails in the hottest path in indexing, and forcing an unexpected serialization failure has no test seam — that decision belongs in its own cycle. Tests: 26/26 unit over the converters (signs, whitespace, already-a-Number, blanks, overflow, fractional-in-integer, and the non-finite NaN/Infinity that Float.parseFloat accepts but JSON cannot carry); 15/15 integration over the emission contract via toMap, including the _dotraw invariant, the omit-both contract, log level and silence on the happy path, the neighbouring branches of the edited else-if chain, and end-to-end proof through a real index that convertible values sort numerically and that an unconvertible document does not answer a range query. Registered in MainSuite1b and phase-agnostic, so the weekly OpenSearch Phase Sweep runs it without reproducing #37432. Also documents the _dotraw padding invariant in docs/backend, since the chain from loadFields to addBuilderSort is invisible from any single file and has already been broken once during review. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 1 + docs/backend/INDEX_FIELD_EMISSION.md | 105 +++ .../business/ESMappingAPIImpl.java | 65 +- .../com/dotmarketing/util/NumberUtil.java | 74 ++ .../com/dotmarketing/util/NumberUtilTest.java | 146 ++++ .../src/test/java/com/dotcms/MainSuite1b.java | 1 + .../ESMappingAPINumericFieldTest.java | 634 ++++++++++++++++++ 7 files changed, 1024 insertions(+), 2 deletions(-) create mode 100644 docs/backend/INDEX_FIELD_EMISSION.md create mode 100644 dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ESMappingAPINumericFieldTest.java 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(); + } +}