fix(check-saved-objects): treat text→keyword field downgrade as a newly introduced problem#260499
Merged
gsoldevila merged 2 commits intoelastic:mainfrom Apr 1, 2026
Conversation
…ly introduced problem Previously, the pre-existing vs. newly-introduced distinction for name/title field type checks was based on whether the field key was present in the baseline snapshot. This caused a regression where a field downgraded from 'text' to 'keyword' was silently classified as pre-existing (key already present) instead of being caught as a new problem. The fix derives both categories consistently from invalidity in the baseline: a field is pre-existing only if it was already invalid (non-text) in the 'from' snapshot. A field that was valid (text) or absent before but is now invalid (keyword) is always treated as newly introduced and triggers a hard failure. Adds a dedicated test covering the text→keyword downgrade case. Made-with: Cursor
Contributor
|
Pinging @elastic/kibana-core (Team:Core) |
Contributor
💚 Build Succeeded
Metrics [docs]
History
|
jeramysoucy
pushed a commit
to jeramysoucy/kibana
that referenced
this pull request
Apr 1, 2026
…ly introduced problem (elastic#260499) ## Summary Follow-up to elastic#260280, addressing the edge case raised in [this review comment](https://github.com/elastic/kibana/pull/260280/changes/BASE..8b520af6534a998b849dd3567b3b659c470e60ea#r3016163498). ### The bug The pre-existing vs. newly-introduced classification in `validateNameTitleFieldTypesExistingType` was based on whether the field *key* was present in the baseline snapshot (`'properties.name.type' in from.mappings`). This correctly catches `absent → keyword`, but silently misclassifies `text → keyword` as pre-existing — because the key is already in `from.mappings`, even though the field was previously valid. The result: a developer who downgrades a `name`/`title` field from `text` to `keyword` gets no warning and no failure from the CI check. ### The fix Both `preExisting` and `newlyIntroduced` are now derived from the set of fields that were **already invalid in `from`**, not merely present: ```ts const alreadyInvalidInFrom = new Set( getInvalidNameTitleFields(from).map(({ fieldName }) => fieldName) ); const preExisting = invalidInTo.filter(({ fieldName }) => alreadyInvalidInFrom.has(fieldName)); const newlyIntroduced = invalidInTo.filter(({ fieldName }) => !alreadyInvalidInFrom.has(fieldName)); ``` This handles all four scenarios correctly: | `from` type | `to` type | Result | |---|---|---| | absent | `keyword` | ❌ fail (newly introduced) | | `text` | `keyword` | ❌ fail (newly introduced — was this PR's bug) | | `keyword` | `keyword` |⚠️ warn (pre-existing, cannot be fixed without reindex) | | `keyword` | `text` | ✅ nothing (fixed) | ## Test plan - [ ] New test `'should throw when a name or title field is downgraded from text to keyword'` in `existing_type.test.ts` covers the regression case - [ ] All 40 tests in the `validate_changes` suite pass Made with [Cursor](https://cursor.com)
eokoneyo
pushed a commit
to davismcphee/kibana
that referenced
this pull request
Apr 2, 2026
…ly introduced problem (elastic#260499) ## Summary Follow-up to elastic#260280, addressing the edge case raised in [this review comment](https://github.com/elastic/kibana/pull/260280/changes/BASE..8b520af6534a998b849dd3567b3b659c470e60ea#r3016163498). ### The bug The pre-existing vs. newly-introduced classification in `validateNameTitleFieldTypesExistingType` was based on whether the field *key* was present in the baseline snapshot (`'properties.name.type' in from.mappings`). This correctly catches `absent → keyword`, but silently misclassifies `text → keyword` as pre-existing — because the key is already in `from.mappings`, even though the field was previously valid. The result: a developer who downgrades a `name`/`title` field from `text` to `keyword` gets no warning and no failure from the CI check. ### The fix Both `preExisting` and `newlyIntroduced` are now derived from the set of fields that were **already invalid in `from`**, not merely present: ```ts const alreadyInvalidInFrom = new Set( getInvalidNameTitleFields(from).map(({ fieldName }) => fieldName) ); const preExisting = invalidInTo.filter(({ fieldName }) => alreadyInvalidInFrom.has(fieldName)); const newlyIntroduced = invalidInTo.filter(({ fieldName }) => !alreadyInvalidInFrom.has(fieldName)); ``` This handles all four scenarios correctly: | `from` type | `to` type | Result | |---|---|---| | absent | `keyword` | ❌ fail (newly introduced) | | `text` | `keyword` | ❌ fail (newly introduced — was this PR's bug) | | `keyword` | `keyword` |⚠️ warn (pre-existing, cannot be fixed without reindex) | | `keyword` | `text` | ✅ nothing (fixed) | ## Test plan - [ ] New test `'should throw when a name or title field is downgraded from text to keyword'` in `existing_type.test.ts` covers the regression case - [ ] All 40 tests in the `validate_changes` suite pass Made with [Cursor](https://cursor.com)
paulinashakirova
pushed a commit
to paulinashakirova/kibana
that referenced
this pull request
Apr 2, 2026
…ly introduced problem (elastic#260499) ## Summary Follow-up to elastic#260280, addressing the edge case raised in [this review comment](https://github.com/elastic/kibana/pull/260280/changes/BASE..8b520af6534a998b849dd3567b3b659c470e60ea#r3016163498). ### The bug The pre-existing vs. newly-introduced classification in `validateNameTitleFieldTypesExistingType` was based on whether the field *key* was present in the baseline snapshot (`'properties.name.type' in from.mappings`). This correctly catches `absent → keyword`, but silently misclassifies `text → keyword` as pre-existing — because the key is already in `from.mappings`, even though the field was previously valid. The result: a developer who downgrades a `name`/`title` field from `text` to `keyword` gets no warning and no failure from the CI check. ### The fix Both `preExisting` and `newlyIntroduced` are now derived from the set of fields that were **already invalid in `from`**, not merely present: ```ts const alreadyInvalidInFrom = new Set( getInvalidNameTitleFields(from).map(({ fieldName }) => fieldName) ); const preExisting = invalidInTo.filter(({ fieldName }) => alreadyInvalidInFrom.has(fieldName)); const newlyIntroduced = invalidInTo.filter(({ fieldName }) => !alreadyInvalidInFrom.has(fieldName)); ``` This handles all four scenarios correctly: | `from` type | `to` type | Result | |---|---|---| | absent | `keyword` | ❌ fail (newly introduced) | | `text` | `keyword` | ❌ fail (newly introduced — was this PR's bug) | | `keyword` | `keyword` |⚠️ warn (pre-existing, cannot be fixed without reindex) | | `keyword` | `text` | ✅ nothing (fixed) | ## Test plan - [ ] New test `'should throw when a name or title field is downgraded from text to keyword'` in `existing_type.test.ts` covers the regression case - [ ] All 40 tests in the `validate_changes` suite pass Made with [Cursor](https://cursor.com)
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
4 participants
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.
Summary
Follow-up to #260280, addressing the edge case raised in this review comment.
The bug
The pre-existing vs. newly-introduced classification in
validateNameTitleFieldTypesExistingTypewas based on whether the field key was present in the baseline snapshot ('properties.name.type' in from.mappings). This correctly catchesabsent → keyword, but silently misclassifiestext → keywordas pre-existing — because the key is already infrom.mappings, even though the field was previously valid.The result: a developer who downgrades a
name/titlefield fromtexttokeywordgets no warning and no failure from the CI check.The fix
Both
preExistingandnewlyIntroducedare now derived from the set of fields that were already invalid infrom, not merely present:This handles all four scenarios correctly:
fromtypetotypekeywordtextkeywordkeywordkeywordkeywordtextTest plan
'should throw when a name or title field is downgraded from text to keyword'inexisting_type.test.tscovers the regression casevalidate_changessuite passMade with Cursor