Fixes 28727: flatten testCaseResolutionStatusDetails index mapping to match its schema (Part A) - #31839
Open
TeddyCr wants to merge 3 commits into
Open
Fixes 28727: flatten testCaseResolutionStatusDetails index mapping to match its schema (Part A)#31839TeddyCr wants to merge 3 commits into
TeddyCr wants to merge 3 commits into
Conversation
…pping to match its schema The resolution-status index mapping declared a `resolved` wrapper object under `testCaseResolutionStatusDetails` that no document can ever carry. The schema models that field as a oneOf over assigned.json / resolved.json, and resolved.json declares testCaseFailureReason / testCaseFailureComment / resolvedBy directly with "additionalProperties": false — so a `resolved` key is illegal, not merely absent. Doc building is generic (SearchIndex uses JsonUtils.getMap and TestCaseResolutionStatusIndex only adds fqnParts/@timestamp/parent relationships), so the indexed document has always been flat. Consequences: the 10x boost on `testCaseResolutionStatusDetails.resolved.testCaseFailureComment` matched nothing in any language, and the real flat fields were only indexed by accident via "dynamic": true — as analyzed text with the standard analyzer, so exact-match on testCaseFailureReason failed and CJK/Russian comment search was broken. Move the three properties up one level in all four locale mappings and repoint the boost. Each locale keeps its own analyzer (om_analyzer for en/ru, om_analyzer_jp for jp, ik_max_word for zh) and jp/zh keep their fields.ngram subfield. "dynamic": true is kept: after flattening, every property of both oneOf branches is mapped explicitly, so it is no longer load-bearing, but it remains the forward-compatibility net that lets a newly added schema property be indexed rather than silently dropped. Flipping it is a separate hardening decision. Keeping it also gives un-reindexed clusters a partial improvement immediately, since the repointed boost now targets a path dynamic mapping already created. UPGRADE: this requires a reindex of test_case_resolution_status_search_index. No DB migration is involved — IndexMappingVersionTracker hashes each mapping and the smart-reindex plan recreates just the changed index. But `migrate` alone is not enough and will not silently no-op: on any cluster that has ever resolved an incident, dynamic mapping already created testCaseResolutionStatusDetails.testCaseFailureReason as text, so the PUT _mapping that updateIndexes() sends is rejected 400 on both engines with "mapper [...] cannot be changed from type [text] to [keyword]" (reproduced on OpenSearch 3.4.0 and Elasticsearch 9.3.0). The rejection is atomic, so no property in that request lands, and on the OpenSearch path the cause is swallowed by OpenSearchIndexManager's catch block. Only the recreate path applies the new mapping and drops the stale `resolved` sub-object. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ch behavior tests Nothing indexed a Resolved-variant resolution-status document and nothing asserted the boost field list, which is why the dead `resolved` wrapper survived. SearchConsumerFieldBehaviorIT now indexes both oneOf branches (Assigned and Resolved) into the real per-language OpenSearch mappings and adds three probes: - the Incident Manager free-text search, built from TestCaseResolutionStatusIndex.getFields() so repointing a boost at a path no document carries fails the test; - an exact-match term on testCaseFailureReason, which distinguishes an explicitly mapped keyword from a field merely picked up by "dynamic": true; - _analyze on testCaseFailureComment per locale, which pins the per-locale analyzer and would catch a flatten that homogenized them. IndexMappingNestedFieldConsistencyTest gains a mapping-to-schema invariant that reads the oneOf branch schemas at runtime rather than hardcoding field names, so adding a property to assigned.json / resolved.json fails until the mappings follow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…roves incidentFailureReasonFilterReturnsIncidentInAllLanguages reported "Incident Manager failure-reason exact match ... is broken in language(s)", but no production search query filters on testCaseResolutionStatusDetails.testCaseFailureReason — the UI reads it from _source, and IncidentTcrsSyncHandler / TestCaseResolutionStatusRepository read it from the task payload. The name and message would send someone hunting for a broken feature that does not exist. The test is valuable as a mapping-type probe: it is what distinguishes an explicitly declared keyword from a field merely picked up by "dynamic": true as analyzed text, on which an exact term can never match. Rename it to failureReasonSupportsExactMatchTermInAllLanguages, reword the assertion to describe the type guarantee rather than a feature, and add a javadoc saying why the probe exists. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
Code Review ✅ ApprovedFlattens the testCaseResolutionStatusDetails index mapping to match its JSON schema, repointing the incident comment boost and enabling multi-language search support. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes:
Fixes #28727
Part A only. Issue #28727 has two parts. This PR fixes Part A (the
testCaseResolutionStatusDetailsmapping↔schema mismatch). Part B — validating highlight fields at Search Settings save time by reusingSearchSourceBuilderFactory.isHighlightUnsafeField— is explicitly out of scope here and remains open. Please do not close the issue on merge beyond Part A.I flattened the
testCaseResolutionStatusDetailsindex mapping so it matches its JSON Schema, and repointed the resolution-comment search boost at the path documents actually carry.The mapping declared a
resolvedwrapper object that the schema makes structurally impossible.testCaseResolutionStatus.jsonmodels the field asoneOf [assigned.json, resolved.json], and both branches are"additionalProperties": false—resolved.jsondeclarestestCaseFailureReason/testCaseFailureComment/resolvedBydirectly. So aresolvedkey could never legally appear in a document, and indeed never did: doc building is generic (SearchIndex→JsonUtils.getMap(entity);TestCaseResolutionStatusIndexonly addsfqnParts,@timestamp, and parent relationships). The consequence was a dead 10× boost —testCaseResolutionStatusDetails.resolved.testCaseFailureCommentinTestCaseResolutionStatusIndex.getFields()matched nothing, in every language."dynamic": trueon that node was masking the bug: it indexed the real flat fields anyway, but as standard-analyzertextrather than the declared types. That broke exact match ontestCaseFailureReason(analyzed text never matches an exact term) and broke CJK/Cyrillic comment search (the standard analyzer splits Japanese/Chinese into single characters and does not stem Russian).Type of change:
High-level design:
Approach — the issue's option 1: correct the mapping to the schema, don't reshape the document. The schema and the document are already flat and are the source of truth; only the mapping was wrong. Reshaping the document to match the mapping would mean writing a wrapper key that
additionalProperties: falseforbids.Files changed (7):
openmetadata-spec/.../elasticsearch/{en,jp,ru,zh}/test_case_resolution_status_index_mapping.jsontestCaseFailureReason/testCaseFailureComment/resolvedByup one level, out of theresolvedwrapper. Pure unwrap — no other edit.openmetadata-service/.../search/indexes/TestCaseResolutionStatusIndex.javatestCaseResolutionStatusDetails.testCaseFailureComment(one line).openmetadata-service/src/test/.../IndexMappingNestedFieldConsistencyTest.javaopenmetadata-integration-tests/.../SearchConsumerFieldBehaviorIT.javaThere is no separate OpenSearch resource tree — the same four files serve both engines, with
OsUtils.enrichIndexMappingForOpenSearchrewriting at runtime.Per-locale analyzers deliberately preserved, not homogenized. en/ru keep
om_analyzer; jp keepsom_analyzer_jp(kuromoji) and itsfields.ngram; zh keepsik_max_wordand itsfields.ngram. Verified byte-for-byte: en ≡ ru in this subtree, and unwrapping the baseresolvedobject withjqyields output identical to this branch in all four locales."dynamic": truekept — deliberate. After flattening, every property of bothoneOfbranches is mapped explicitly, sodynamicis no longer load-bearing for correctness. It is retained as the forward-compatibility net: a property added toassigned.json/resolved.jsonstill gets indexed rather than silently dropped, and the new unit test fires immediately in that case. Flipping it tofalseis a separate hardening decision and would couple a silent-data-loss behavior change to a mapping correction.Alternative rejected: rewriting the document in
TestCaseResolutionStatusIndexto emit aresolvedwrapper. That would contradict the schema, break the UI and Python (both already read/write the flat shape), and invalidate the existingtestCaseResolutionStatusDetails.assignee.namefilter pattern.This change requires a reindex of
test_case_resolution_status_search_index.migratealone is not sufficient and will log a mapping-update failure for that index.migratereachessearchRepository.updateIndexes()(OpenMetadataOperations.java:1196), which issuesPUT _mapping. On any cluster that has ever resolved an incident,dynamic: truehas already created those fields astext, so declaring themkeywordis an incompatible type change and the request is rejected 400:Three things worth knowing:
…testCaseFailureReason, Elasticsearch 9.3.0 reported…resolvedBy.idfor the identical request. Same 400, same atomicity; a different field name is not a different bug.OpenSearchIndexManagerlogs"Failed to Update Open Search index {}"without the exception, so an operator sees one bare error line and a silently stale mapping.The supported path is
./bootstrap/openmetadata-ops.sh reindex. It is also the only path that drops the staleresolvedsub-object, sincePUT _mappingcannot remove a field.IndexMappingVersionTrackerhashes each mapping, so the smart-reindex plan picks uptestCaseResolutionStatusand recreates just that index — no DB migration is involved.Mitigating point: because
"dynamic": truewas retained, un-reindexed clusters still get a partial improvement immediately — the repointed boost now targets a path dynamic mapping has already created (as standard-analyzer text) rather than a path with no values at all. Full correctness (declaredkeyword, per-locale analyzers) arrives with the reindex.Tests:
Use cases covered
testCaseResolutionStatusDetailsis indexed and searchable at all — previously no test anywhere indexed a Resolved-variant document.testCaseFailureReasonsupports exact-match term lookup, i.e. its declaredkeywordtype is actually in effect.assignee.namefilter, test-case/origin-entity filters) is unchanged.Unit tests
openmetadata-service/src/test/java/org/openmetadata/service/search/IndexMappingNestedFieldConsistencyTest.javaresolutionStatusDetailsMappingMustMatchSchemaPropertiesasserts the mapped subfield set equals the union of thepropertiesof everyoneOfbranch, read from the JSON Schema at runtime rather than hardcoded — so adding a property toassigned.json/resolved.jsonfails this test until the mappings follow. It guards the class of bug, not just this instance.Tests run: 7, Failures: 0, Errors: 0, Skipped: 0(1 failure before the fix).Coverage on the changed class — measured, and honestly short of the 90% target. Ran
mvn -P static-code-analysis -pl openmetadata-service test jacoco:reportscoped to the search tests.TestCaseResolutionStatusIndex.javaline coverage is 62.8% (27/43):<init>buildSearchIndexDocInternalgetEntityTypeNamesetParentRelationshipsgetEntitygetFields— the method this PR changesThe 0% on
getFieldsis a measurement artifact, not an untested line:getFields()is executed directly by the new integration test (incidentResolutionCommentSearchReturnsIncidentInAllLanguagesbuilds its query fromTestCaseResolutionStatusIndex.getFields()), and that test lives inopenmetadata-integration-tests— a separate Maven module whose execution this-pl openmetadata-servicejacoco run does not observe. The changed line is covered by an automated test that fails without the fix; it is simply not covered by a unit test in the measured module. The remaining gaps (setParentRelationships,getEntity) are pre-existing and untouched by this PR.Backend integration tests
openmetadata-integration-tests/.openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/SearchConsumerFieldBehaviorIT.javaThis class boots a real OpenSearch 3.4.0 testcontainer with analysis-kuromoji and analysis-ik, creates the real mapping for every language in
IndexMappingLanguage(en/jp/ru/zh), indexes real documents and runs the actual consumer queries. I added a Resolved-variant document alongside the existing Assigned one, plus three tests:incidentResolutionCommentSearchReturnsIncidentInAllLanguagesmost_fields/operator:andmulti_matchbuilt fromTestCaseResolutionStatusIndex.getFields()— returns the incident when searching a word unique to its comment. Because the field list comes from production code, repointing a boost at a dead path fails this test.is broken in language(s): [en, jp, zh, ru], 0 hits everywherefailureReasonSupportsExactMatchTermInAllLanguagestermontestCaseResolutionStatusDetails.testCaseFailureReasonreturns the incident, i.e. the declaredkeywordis in effect. A type probe, not a product feature — nothing filters on the reason today; it is what distinguishes "explicitly mapped" from "accidentally indexed bydynamic:true".is broken in language(s): [en, jp, zh, ru]resolutionCommentUsesLanguageAnalyzerInAllLanguages_analyzeon the comment field yields each locale's expected token, proving the per-locale analyzer survived the flatten.ru expected 'продаж' but got [ежемесячные, продажи],zh expected '销售' but got [北, 京, 大, 学, 的, 销, 售, 报, 表],jp expected '東京' but got [東, 京, タワー, の, 売, 上]Tests run: 20, Failures: 0, Errors: 0, Skipped: 0(3 failures before the fix; the 17 pre-existing tests passed throughout, so the added document perturbs nothing).Elasticsearch — verified by hand only. The automated IT runs OpenSearch 3.4.0. On a real ES 9.3.0 container the branch mapping creates cleanly, the flat Resolved document indexes, and both the boost query and the
testCaseFailureReasonterm filter return 1 hit — against 0 hits on the base mapping. This was done manually during review; no automated ES coverage exists for this index, so a future ES-only regression would not be caught by CI.Ingestion integration tests
ingestion/.../sample_data.pyalready writes the flat shape and needed no change.Playwright (UI) tests
InlineTestCaseIncidentStatus.component.tsx), which is part of the evidence that the mapping, not the document, was wrong.Manual testing performed
Steps 1 and the reindex/mapping inspection below are the reproducible procedure; steps marked (not verified) were not executed against a running OpenMetadata stack — the equivalent effects are covered by the automated IT against a real engine, and the
PUT _mappingrejection in step 3a was reproduced directly against both engines rather than throughopenmetadata-ops.sh.mvn install -pl openmetadata-spec -DskipTeststhenmvn install -nsu -pl openmetadata-service -DskipTests.docker compose -f docker/development/docker-compose.yml up -d; log in as admin../bootstrap/openmetadata-ops.sh reindex. ExpectIndex mapping changed for entity: testCaseResolutionStatusand only that entity in the plan; a secondreindexreports no changes.migrate-only path fails as described in the upgrade note above (400, atomic, cause swallowed on OpenSearch).["assignee","resolvedBy","testCaseFailureComment","testCaseFailureReason"]and noresolved. Then checktestCaseFailureCommentcarries"type":"text"with the locale's analyzer (om_analyzeren/ru,om_analyzer_jpjp,ik_max_wordzh) and, for jp/zh, afields.ngramsubfield.FalsePositive, commentflakyupstream feed was backfilled.hits.total.value: 0; post-fix the incident is returned. Re-run with&explain=trueand confirm the winning clause istestCaseResolutionStatusDetails.testCaseFailureComment^10.0, ranking a comment match above atestCaseResolutionStatusType^1.0match.SearchListFilter→testCaseResolutionStatusDetails.assignee.name) and the incident detail panel still renders reason/comment/resolvedBy.Gates run on the rebased branch (spec + service installed from the working tree first):
mvn spotless:check -nsu -pl openmetadata-service,openmetadata-integration-tests0 needs changes to be cleanin both modules,BUILD SUCCESSmvn test -nsu -pl openmetadata-service -Dtest='org.openmetadata.service.search.**.*Test,SearchListFilterTest'Tests run: 2231, Failures: 0, Errors: 0, Skipped: 0mvn test -nsu -pl openmetadata-integration-tests -Dtest=SearchConsumerFieldBehaviorITTests run: 20, Failures: 0, Errors: 0, Skipped: 0UI screen recording / screenshots:
Not applicable — this PR contains no UI changes. The diff touches only ES/OS mapping resources, one backend boost map, and two test files; nothing under
openmetadata-ui/.Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.openmetadata-spec/src/main/resources/elasticsearch/, not JSON Schemas underjson/schema/— they drive no code generation, somake generatedoes not apply. No SQL migration is needed either: mapping drift is tracked by content hash inIndexMappingVersionTrackerand resolved by reindex (see the upgrade note). Precedent: 14b3a57 (fix(search): add nested customPropertiesTyped mapping to 5 entity indexes #30501) changed 20 mapping JSONs with no SQL and no version bump.mainand pass here; the RED output is quoted in the tables above.🤖 Generated with Claude Code
Greptile Summary
The PR aligns the test-case resolution-status search mapping with the schema’s flat resolved-details shape and updates the resolution-comment boost to target the field documents actually contain.
testCaseFailureReason,testCaseFailureComment, andresolvedByin all four locale mappings while preserving locale-specific analyzers.Confidence Score: 5/5
The PR appears safe to merge, with the documented reindex requirement remaining necessary for existing deployments.
The flattened mappings match the closed schema branches and serialized document shape, the search boost targets the resulting live field, and the added tests exercise the corrected behavior across every supported mapping language.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR A[TestCaseResolutionStatus entity] --> B[Generic JSON search document] B --> C[testCaseResolutionStatusDetails] C --> D[assignee] C --> E[testCaseFailureReason] C --> F[testCaseFailureComment] C --> G[resolvedBy] F --> H[Locale-specific analyzer] F --> I[Boosted incident search query]Reviews (1): Last reviewed commit: "Fixes #28727: name the failure-reason te..." | Re-trigger Greptile