Map calculated value fields according to their configured element type - #488
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Boolean string values such as 'false' are currently normalized to true.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds element-type-aware indexing for calculated values.
Changes:
- Adds boolean, numeric, date, and text mappings with normalization.
- Registers the dedicated adapter.
- Adds unit coverage for mappings and normalization.
File summaries
| File | Description |
|---|---|
CalculatedValueAdapter.php |
Implements mapping and normalization. |
field-definition-adapters.yml |
Registers the new adapter. |
CalculatedValueAdapterTest.php |
Tests mappings and normalized values. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Calculated value fields were always mapped as text/keyword regardless of their configured element type. This made boolean, numeric and date calculated values unusable for term queries, aggregations and range filters in the search index. Add a dedicated CalculatedValueAdapter that maps the field based on the element type (boolean, numeric, date) and falls back to the existing text/keyword mapping for the text-based element types (input, textarea, html). Values are normalized accordingly, since calculators may return loosely typed values.
A plain bool cast would index the literal 'false' (and values such as 'off'/'no') as true, silently reversing filters. Use FILTER_VALIDATE_BOOLEAN with FILTER_NULL_ON_FAILURE so unrecognized values become null instead.
aae95d9 to
abd7cc9
Compare
|
Numeric calculated fields now map through NumericMappingTrait as 64-bit double instead of 32-bit float, consistent with the Numeric/QuantityValue adapters (pimcore#510), so large values are not collapsed by float32 rounding. Date normalization also accepts date strings: class calculators are typed to return strings and the query_store mode only ever yields strings, so accepting only DateTimeInterface would silently drop most calculated date values. Unparseable values degrade to null instead of failing the document. Adds an upgrade note since the mapping change requires re-indexing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the well-analyzed fix, including the reproduction and the I verified the root cause (
Unit suite, PHPStan and php-cs-fixer are green locally on PHP 8.4. |
kingjia90
left a comment
kingjia90
left a comment
There was a problem hiding this comment.
Root cause confirmed (calculatedValue registered on TextKeywordAdapter since the initial registration, elementType never consulted). Fix sits at the right boundary, covers plain + localized calculated fields (LocalizedFieldsAdapter delegates mapping and normalization to the child adapter), keeps the default text/keyword behavior for input/textarea/html, and is covered by unit tests. With the double-mapping and date-string adjustments pushed on top, this is good to go once CI is green.
|
There was a problem hiding this comment.
🟡 Changes recommended
The numeric mapping is double, while the PR description and linked issue specify float.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
Fixes pimcore/platform-version#293
Problem
Calculated value fields are always indexed with the generic text/keyword mapping, regardless of their configured element type (
calculatedValueis registered onTextKeywordAdapterinfield-definition-adapters.yml).For a
CalculatedValuefield withelementType: 'boolean'this creates atextmapping with ngram/keyword/sort subfields in the search index. Term queries, aggregations and boolean filters on the field then behave like text search, and the Studio data object grid boolean filter on such a field stops working.Reproduction
CalculatedValuefield configured withelementType: 'boolean'bin/console generic-data-index:update:indexGET <index>/_mapping/field/standard_fields.<fieldname>returnstext(with ngram/keyword/sort subfields) instead ofbooleanSolution
Add a dedicated
CalculatedValueAdapterthat maps the field according to its configured element type:booleanbooleannumericdouble(viaNumericMappingTrait, consistent with #510)datedate(strict_date_time_no_millis)input/textarea/html(default)Values are normalized per element type, since calculators are free to return loosely typed values (e.g.
'0'/'1'strings from expression results, which a boolean mapping would reject). Date strings are parsed as well, since class calculators are typed to return strings and thequery_storemode only yields strings. For the text-based element types the existingTextKeywordAdapternormalization (base64srcstripping) is preserved.An upgrade note was added, since the mapping change requires
generic-data-index:update:index+ re-indexing to take effect.Edit (maintainer): numeric maps as
doubleinstead of the originally proposedfloat, and date normalization also accepts strings — see this comment for details.Covered by unit tests (
CalculatedValueAdapterTest). Verified end-to-end on Elasticsearch 8.19: after this changegeneric-data-index:update:indexcreates the new index version with abooleanmapping and reindexing succeeds.