Skip to content

Correctly handle non-integers in nested paths in the remove processor #127006

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.

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

Merged
merged 7 commits into from
Apr 18, 2025

Conversation

joegallo
Copy link
Contributor

Fixes a bug that was introduced by #125232.

During path traversal, if we find a non-integer path for the next access inside a list, then that doesn't necessarily mean we must throw an exception -- rather, we should only throw an exception on a non-integer if ignoreMissing is false. That is, we should treat accessing the "foo" index inside a list as just another missing path (for example).

See also #126417 which is similar to this, but not the same. 😬

If we don't throw, then the index wouldn't have been initialized, so
now it's declared as being -1 -- note that this means that in the
event of a non-integer it'll fall through to the next block with
`if (index < 0` and that that one also won't throw. This seemed
simpler to me than adding in an early return.
@joegallo joegallo added >bug :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP Team:Data Management Meta label for data/management team auto-backport Automatically create backport pull requests when merged v8.19.0 v9.1.0 labels Apr 17, 2025
@joegallo joegallo requested a review from parkertimmins April 17, 2025 14:53
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@elasticsearchmachine
Copy link
Collaborator

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

@joegallo
Copy link
Contributor Author

joegallo commented Apr 17, 2025

@parkertimmins after you've finished reviewing this on its own terms, but before you add a ✅, let's step through the removeField method together line by line and see if we agree that all the cases are being correctly handled.

@joegallo
Copy link
Contributor Author

joegallo commented Apr 17, 2025

Copy link
Contributor

@parkertimmins parkertimmins left a comment

Choose a reason for hiding this comment

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

lgtm

@joegallo joegallo merged commit b46bee4 into elastic:main Apr 18, 2025
17 checks passed
@joegallo joegallo deleted the remove-processor-noninteger-in-path branch April 18, 2025 15:47
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged >bug :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP Team:Data Management Meta label for data/management team v8.19.0 v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants