Skip to content
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

[Transform] Adding null check to fix potential NPE #96785

Merged
merged 10 commits into from Jun 22, 2023

Conversation

AjeetNathawat
Copy link
Contributor

@AjeetNathawat AjeetNathawat commented Jun 13, 2023

if CompositeAggregation is null then it can cause NPE in extractResults so adding null check to avoid it

@elasticsearchmachine elasticsearchmachine added v8.9.0 needs:triage Requires assignment of a team area label external-contributor Pull request authored by a developer outside the Elasticsearch team labels Jun 13, 2023
@hendrikmuhs
Copy link
Contributor

Thanks for the contribution. See my comment, the listener has to be called.

Would you also be willing to contribute a unit test? You can take this method as example:

https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/pivot/PivotTests.java#L233

It would be nice if you could add a new method called testPreview that covers your fix, the "null" and the "empty" case.

@AjeetNathawat
Copy link
Contributor Author

Thanks for the contribution. See my comment, the listener has to be called.

Would you also be willing to contribute a unit test? You can take this method as example:

https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/pivot/PivotTests.java#L233

It would be nice if you could add a new method called testPreview that covers your fix, the "null" and the "empty" case.

Sure I will create test case with the reference.

@hendrikmuhs hendrikmuhs added :ml/Transform Transform v7.17.11 and removed needs:triage Requires assignment of a team area label labels Jun 13, 2023
@hendrikmuhs hendrikmuhs self-assigned this Jun 13, 2023
@elasticsearchmachine elasticsearchmachine added the Team:ML Meta label for the ML team label Jun 13, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/ml-core (Team:ML)

@hendrikmuhs
Copy link
Contributor

Thanks for the update, time to run all tests:

@elasticmachine test this please

@AjeetNathawat
Copy link
Contributor Author

AjeetNathawat commented Jun 13, 2023

after looking at the logs it seems I made a blunder, let me modify my changelog as per the required enum and style check

@AjeetNathawat
Copy link
Contributor Author

I've added the required UT, please do review

@hendrikmuhs
Copy link
Contributor

@elasticmachine test this please

@hendrikmuhs
Copy link
Contributor

@AjeetNathawat the easiest way to fix the checkstyle issues is to run spotlessApply, see https://github.com/elastic/elasticsearch/blob/main/CONTRIBUTING.md#java-language-formatting-guidelines

Before pushing the changes to your branch, please run precommit to execute all checks.

@AjeetNathawat
Copy link
Contributor Author

I've done the formatting now and ran the precommit task its successful now, thanks for pointing it out

@AjeetNathawat
Copy link
Contributor Author

Hello @przemekwitek Thanks for the suggestion, I've addressed your comment.

Copy link
Contributor

@przemekwitek przemekwitek left a comment

Choose a reason for hiding this comment

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

LGTM

@przemekwitek przemekwitek added v8.9.1 v8.8.2 auto-backport-and-merge Automatically create backport pull requests and merge when ready >bug and removed >non-issue labels Jun 22, 2023
@przemekwitek przemekwitek changed the title Adding null check to fix potential NPE [Transform] Adding null check to fix potential NPE Jun 22, 2023
@przemekwitek przemekwitek merged commit c9e37ef into elastic:main Jun 22, 2023
14 checks passed
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
7.17 Commit could not be cherrypicked due to conflicts
8.8
8.9

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 96785

hendrikmuhs pushed a commit to hendrikmuhs/elasticsearch that referenced this pull request Jun 22, 2023
hendrikmuhs pushed a commit to hendrikmuhs/elasticsearch that referenced this pull request Jun 22, 2023
elasticsearchmachine pushed a commit that referenced this pull request Jun 22, 2023
Co-authored-by: AjeetNathawat <72258466+AjeetNathawat@users.noreply.github.com>
elasticsearchmachine pushed a commit that referenced this pull request Jun 22, 2023
Co-authored-by: AjeetNathawat <72258466+AjeetNathawat@users.noreply.github.com>
@joegallo joegallo added v7.17.11 and removed v7.17.0 labels Jun 23, 2023
@przemekwitek
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
7.17

Questions ?

Please refer to the Backport tool documentation

przemekwitek pushed a commit to przemekwitek/elasticsearch that referenced this pull request Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport-and-merge Automatically create backport pull requests and merge when ready >bug external-contributor Pull request authored by a developer outside the Elasticsearch team :ml/Transform Transform Team:ML Meta label for the ML team v7.17.11 v8.9.0 v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants