Skip to content

Support Fields API in conditional ingest processors #131581

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Jul 23, 2025
Merged

Support Fields API in conditional ingest processors #131581

merged 8 commits into from
Jul 23, 2025

Conversation

eyalkoren
Copy link
Contributor

Retrying to add field API support for conditional ingest processors.

@jdconrad I propose a review from fresh, as it is a small one.
Apart from the other things we discussed, note that SourceMapFieldScript doesn't implement getMetadata, I am not sure if there are implications for that.

eyalkoren added 3 commits July 20, 2025 16:47
@eyalkoren
Support Fields API in conditional ingest processors
b509846 
Fixing whitelist

Fix access to dotted field from source in yaml test

Separate field access and field modification APIs

Extending yaml test to cover all field access APIs

Separate WriteField and SourceMapField tests

Add test verifying that iterator() API does not allow removal

Small test change

Caching script instance instead of script factory

Update docs/changelog/121914.yaml

Initial review changes

Removing params from execute

Remove todo

Removing Field's methods from whitelists

[CI] Auto commit changes from spotless

Not relying on specific error type in yaml test
@eyalkoren
Caching script factory instead of script instance
4cbde0c
@eyalkoren
Removing former changelog file
3bb6c20
@eyalkoren eyalkoren requested a review from jdconrad July 20, 2025 14:07
@eyalkoren eyalkoren self-assigned this Jul 20, 2025
@eyalkoren eyalkoren requested a review from a team as a code owner July 20, 2025 14:07
@eyalkoren eyalkoren added :Core/Infra/Core Core issues without another label v9.2.0 labels Jul 20, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jul 20, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine
Copy link
Collaborator

Hi @eyalkoren, I've created a changelog YAML for you.

elasticsearchmachine and others added 3 commits July 20, 2025 14:16
@cla-checker-service CLA Checker Service
Copy link

cla-checker-service bot commented Jul 22, 2025 •
edited
Loading

💚 CLA has been signed

@eyalkoren
Copy link
Contributor Author

eyalkoren commented Jul 22, 2025 •
edited
Loading

@jdconrad I just realized that SourceFieldScript should not expose getCtx() - this script is meant to expose the non-mutable field APIs. The way it does that is by returning SourceMapField, which is the new type we added exactly for this purpose. The fact that it returns the ctx map it receives in its constructor means that it becomes the responsibility of its users to ensure immutability, which is not desirable. Besides, it's not its purpose to expose the source map as is.
I fixed that in 95f1c93.

I would argue that IngestConditionalScript as well should be the one enforcing immutability rather than what uses it, but this is not as bad as it has very specific purpose to serve, thus likely remain being used in a single place.

jdconrad
jdconrad approved these changes Jul 22, 2025
View reviewed changes
Copy link
Contributor

@jdconrad jdconrad left a comment

Choose a reason for hiding this comment

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

This looks good to me. Definitely seems cleaner to use the factory instead anyway :).

@jdconrad
Copy link
Contributor

Immutability vs performance has been challenging to balance appropriately with some of the input data structures to scripts in general. I would hope that once most users reach scripting the expectations change a bit where users are expected to have a larger knowledge base to prevent incompatible mutations.

@eyalkoren eyalkoren merged commit 558cc7a into elastic:main Jul 23, 2025
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdconrad jdconrad jdconrad approved these changes

Assignees

@eyalkoren eyalkoren

Labels
:Core/Infra/Core Core issues without another label >enhancement Team:Core/Infra Meta label for core/infra team v9.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eyalkoren @elasticsearchmachine @jdconrad