Skip to content

[Entity Store] Make schema properties nullable#264509

Merged
orouz merged 9 commits intoelastic:mainfrom
orouz:entity_store_flk_fix2
Apr 26, 2026
Merged

[Entity Store] Make schema properties nullable#264509
orouz merged 9 commits intoelastic:mainfrom
orouz:entity_store_flk_fix2

Conversation

@orouz
Copy link
Copy Markdown
Contributor

@orouz orouz commented Apr 20, 2026

reverts #263087, which fixed error clearing via read-modify-write (updateWith) - introduced a race condition causing flaky tests (diagnosed in #264303).

fix: make error (and logExtractionState fields) nullable and set them to null directly. works correctly with mergeAttributes: true, which treats undefined as "leave unchanged" but does writes null explicitly

3 flaky runs in this PR failed on unrelated tests. targeted flaky tests all passed: #264475, #264372, #264303, #264299

@orouz orouz added release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting Entity Store (Next) Team:Core Analysis Security Solution labels Apr 20, 2026
@orouz
Copy link
Copy Markdown
Contributor Author

orouz commented Apr 20, 2026

/flaky scoutConfig:x-pack/solutions/security/plugins/entity_store/test/scout/api/playwright.config.ts:30

@kibanamachine
Copy link
Copy Markdown
Contributor

Flaky Test Runner

✅ Build triggered - kibana-flaky-test-suite-runner#11758

  • x-pack/solutions/security/plugins/entity_store/test/scout/api/playwright.config.ts x30

@kibanamachine
Copy link
Copy Markdown
Contributor

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#11758

[❌] x-pack/solutions/security/plugins/entity_store/test/scout/api/playwright.config.ts: 28/30 tests passed.

see run history

@orouz
Copy link
Copy Markdown
Contributor Author

orouz commented Apr 21, 2026

/flaky scoutConfig:x-pack/solutions/security/plugins/entity_store/test/scout/api/playwright.config.ts:30

@kibanamachine
Copy link
Copy Markdown
Contributor

Flaky Test Runner

✅ Build triggered - kibana-flaky-test-suite-runner#11769

  • x-pack/solutions/security/plugins/entity_store/test/scout/api/playwright.config.ts x30

@kibanamachine
Copy link
Copy Markdown
Contributor

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#11769

[❌] x-pack/solutions/security/plugins/entity_store/test/scout/api/playwright.config.ts: 28/30 tests passed.

see run history

@orouz orouz force-pushed the entity_store_flk_fix2 branch from eb717be to a35b9a7 Compare April 21, 2026 12:10
@orouz
Copy link
Copy Markdown
Contributor Author

orouz commented Apr 21, 2026

/flaky scoutConfig:x-pack/solutions/security/plugins/entity_store/test/scout/api/playwright.config.ts:30

@kibanamachine
Copy link
Copy Markdown
Contributor

Flaky Test Runner

✅ Build triggered - kibana-flaky-test-suite-runner#11772

  • x-pack/solutions/security/plugins/entity_store/test/scout/api/playwright.config.ts x30

@kibanamachine
Copy link
Copy Markdown
Contributor

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#11772

[❌] x-pack/solutions/security/plugins/entity_store/test/scout/api/playwright.config.ts: 29/30 tests passed.

see run history

@orouz orouz force-pushed the entity_store_flk_fix2 branch from a35b9a7 to 4a69d89 Compare April 21, 2026 14:23
@orouz
Copy link
Copy Markdown
Contributor Author

orouz commented Apr 21, 2026

/flaky scoutConfig:x-pack/solutions/security/plugins/entity_store/test/scout/api/playwright.config.ts:30

@kibanamachine
Copy link
Copy Markdown
Contributor

Flaky Test Runner

✅ Build triggered - kibana-flaky-test-suite-runner#11779

  • x-pack/solutions/security/plugins/entity_store/test/scout/api/playwright.config.ts x30

@orouz orouz force-pushed the entity_store_flk_fix2 branch from 4a69d89 to 9ca5665 Compare April 21, 2026 14:36
@kibanamachine
Copy link
Copy Markdown
Contributor

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#11779

[❌] x-pack/solutions/security/plugins/entity_store/test/scout/api/playwright.config.ts: 28/30 tests passed.

see run history

@orouz orouz marked this pull request as ready for review April 23, 2026 08:46
@orouz orouz requested review from a team as code owners April 23, 2026 08:46
Copy link
Copy Markdown
Member

@kubasobon kubasobon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two nit-picks.

@orouz orouz force-pushed the entity_store_flk_fix2 branch from 7371004 to c43f925 Compare April 23, 2026 10:07
@orouz orouz added backport:version Backport to applied version labels v9.4.0 and removed backport:skip This PR does not require backporting labels Apr 23, 2026
@orouz orouz force-pushed the entity_store_flk_fix2 branch from 1afb2cf to a95b418 Compare April 26, 2026 08:06
@orouz
Copy link
Copy Markdown
Contributor Author

orouz commented Apr 26, 2026

LGTM. Just a small question, has error.action always been init | extractLogs?

yes, but that that schema change is now reverted. thanks!

@uri-weisman
Copy link
Copy Markdown
Contributor

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 26, 2026

✅ Actions performed

Full review triggered.

@orouz orouz force-pushed the entity_store_flk_fix2 branch from 8351e52 to a75d0a8 Compare April 26, 2026 09:54
@orouz
Copy link
Copy Markdown
Contributor Author

orouz commented Apr 26, 2026

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 26, 2026

✅ Actions performed

Full review triggered.

1 similar comment
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 26, 2026

✅ Actions performed

Full review triggered.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 26, 2026

📝 Walkthrough

Walkthrough

Engine descriptor log-extraction fields are made explicitly nullable with default null. A new engine_descriptor model version 5 is added with a data backfill that maps existing optional fields into the new runtime cursor fields and normalizes error to null. The EngineDescriptorClient.updateWith API was removed; persistence now uses update(entityType, state, { mergeAttributes }). Code, fixtures, snapshots, and tests were updated to persist cleared pagination/cursor fields as null and to treat paginationId: null as undefined when building recovery queries. The public status mapper converts a null lastExecutionTimestamp to undefined.

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • 🛠️ Update Documentation: Commit on current branch
  • 🛠️ Update Documentation: Create PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 26, 2026

✅ Actions performed

Full review triggered.

2 similar comments
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 26, 2026

✅ Actions performed

Full review triggered.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 26, 2026

✅ Actions performed

Full review triggered.

@orouz
Copy link
Copy Markdown
Contributor Author

orouz commented Apr 26, 2026

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 26, 2026

✅ Actions performed

Full review triggered.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 26, 2026

✅ Actions performed

Full review triggered.

1 similar comment
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 26, 2026

✅ Actions performed

Full review triggered.

@kibanamachine
Copy link
Copy Markdown
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] affected Scout: [ observability / apm ] plugin / local-serverless-observability_complete - Custom links template validation - Create custom link with template URL and filters

Metrics [docs]

✅ unchanged

History

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
x-pack/solutions/security/plugins/entity_store/server/domain/saved_objects/engine_descriptor/types.ts (1)

246-253: ⚠️ Potential issue | 🟠 Major

Normalize legacy empty-string paginationId during the v5 backfill.

Line 252 still preserves '' from the v2 migration (paginationId ?? ''), so preexisting descriptors will not actually move to the new null sentinel after upgrading. That leaves migrated docs with mixed cursor semantics and can keep recovery logic on the legacy empty-string path.

🐛 Proposed fix
-              paginationId: logExtractionState?.paginationId ?? null,
+              paginationId:
+                logExtractionState?.paginationId === ''
+                  ? null
+                  : logExtractionState?.paginationId ?? null,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@x-pack/solutions/security/plugins/entity_store/server/domain/saved_objects/engine_descriptor/types.ts`
around lines 246 - 253, The backfillFn currently preserves the legacy
empty-string sentinel for paginationId, so update the v5 backfill to normalize
'' to null: inside backfillFn (and the logExtractionState handling) compute
paginationId from document.attributes.logExtractionState and if it's exactly the
empty string set it to null, otherwise keep its value or default to null;
reference the backfillFn, logExtractionState, and paginationId identifiers when
making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In
`@x-pack/solutions/security/plugins/entity_store/server/domain/saved_objects/engine_descriptor/types.ts`:
- Around line 246-253: The backfillFn currently preserves the legacy
empty-string sentinel for paginationId, so update the v5 backfill to normalize
'' to null: inside backfillFn (and the logExtractionState handling) compute
paginationId from document.attributes.logExtractionState and if it's exactly the
empty string set it to null, otherwise keep its value or default to null;
reference the backfillFn, logExtractionState, and paginationId identifiers when
making this change.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: c10dabf1-4268-4604-a13e-48e38c2659bd

📥 Commits

Reviewing files that changed from the base of the PR and between b670aab and feac50e.

📒 Files selected for processing (8)
  • packages/kbn-check-saved-objects-cli/src/migrations/__fixtures__/entity-engine-descriptor-v2/10.5.0.json
  • src/core/server/integration_tests/ci_checks/saved_objects/check_registered_types.test.ts
  • x-pack/solutions/security/plugins/entity_store/server/domain/logs_extraction/logs_extraction_client.test.ts
  • x-pack/solutions/security/plugins/entity_store/server/domain/logs_extraction/logs_extraction_client.ts
  • x-pack/solutions/security/plugins/entity_store/server/domain/saved_objects/engine_descriptor/constants.ts
  • x-pack/solutions/security/plugins/entity_store/server/domain/saved_objects/engine_descriptor/index.ts
  • x-pack/solutions/security/plugins/entity_store/server/domain/saved_objects/engine_descriptor/types.ts
  • x-pack/solutions/security/plugins/entity_store/server/routes/apis/status.ts

@orouz orouz merged commit 135ed57 into elastic:main Apr 26, 2026
18 checks passed
@kibanamachine
Copy link
Copy Markdown
Contributor

Starting backport for target branches: 9.4

https://github.com/elastic/kibana/actions/runs/24958258962

@kibanamachine
Copy link
Copy Markdown
Contributor

💚 All backports created successfully

Status Branch Result
9.4

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Apr 26, 2026
# Backport

This will backport the following commits from `main` to `9.4`:
- [[Entity Store] Make schema properties nullable
(#264509)](#264509)

<!--- Backport version: 9.6.6 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sorenlouv/backport)

<!--BACKPORT
[{"author":{"name":"orouz","email":"or.ouziel@elastic.co"},"sourceCommit":{"committedDate":"2026-04-26T13:51:50Z","message":"[Entity
Store] Make schema properties nullable
(#264509)","sha":"135ed57d5c599b12c2ac6c35cb5986be49b2dd05","branchLabelMapping":{"^v9.5.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport:version","Entity
Store (Next)","Team:Core Analysis","v9.4.0","v9.5.0"],"title":"[Entity
Store] Make schema properties
nullable","number":264509,"url":"https://github.com/elastic/kibana/pull/264509","mergeCommit":{"message":"[Entity
Store] Make schema properties nullable
(#264509)","sha":"135ed57d5c599b12c2ac6c35cb5986be49b2dd05"}},"sourceBranch":"main","suggestedTargetBranches":["9.4"],"targetPullRequestStates":[{"branch":"9.4","label":"v9.4.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v9.5.0","branchLabelMappingKey":"^v9.5.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/264509","number":264509,"mergeCommit":{"message":"[Entity
Store] Make schema properties nullable
(#264509)","sha":"135ed57d5c599b12c2ac6c35cb5986be49b2dd05"}}]}]
BACKPORT-->

Co-authored-by: orouz <or.ouziel@elastic.co>
rbrtj pushed a commit to walterra/kibana that referenced this pull request Apr 27, 2026
SoniaSanzV pushed a commit to SoniaSanzV/kibana that referenced this pull request Apr 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:version Backport to applied version labels Entity Store (Next) release_note:skip Skip the PR/issue when compiling release notes Team:Core Analysis Security Solution v9.4.0 v9.5.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants