Skip to content

fix: handle null path parameter in RestNodesCapabilitiesAction and pr… #113413

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 5 commits into from
Sep 27, 2024

Conversation

nbenliogludev
Copy link
Contributor

@nbenliogludev nbenliogludev commented Sep 23, 2024

Fix: handle null path parameter in RestNodesCapabilitiesAction and prevent NPE

  • Added null check for the 'path' parameter in RestNodesCapabilitiesAction to avoid NullPointerException.
  • If 'path' is not provided, assign an empty string to ensure safe handling in the URLDecoder and other parts of the code.
  • Updated the response to return a valid node capabilities result with a successful outcome.
  • Ensured the request behaves properly when no path is provided, preventing crashes.

Copy link

cla-checker-service bot commented Sep 23, 2024

💚 CLA has been signed

@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v9.0.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Sep 23, 2024
…event NPE

- Added null check for the 'path' parameter in RestNodesCapabilitiesAction to avoid NullPointerException.
- If 'path' is not provided, assign an empty string to ensure safe handling in the URLDecoder and other parts of the code.
- Updated the response to return a valid node capabilities result with a successful outcome.
- Ensured the request behaves properly when no path is provided, preventing crashes.
@nbenliogludev nbenliogludev force-pushed the fix/npe-capabilities-null-path branch from 027e805 to 9796c99 Compare September 23, 2024 18:46
@nbenliogludev
Copy link
Contributor Author

@elasticsearchmachine Could you please assign a reviewer to this PR? Thank you!

@astefan astefan added :Core/Infra/REST API REST infrastructure and utilities v9.0.0 and removed needs:triage Requires assignment of a team area label v9.0.0 labels Sep 27, 2024
@astefan astefan requested a review from thecoop September 27, 2024 08:26
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Sep 27, 2024
@thecoop thecoop added the >bug label Sep 27, 2024
@thecoop thecoop self-assigned this Sep 27, 2024
@@ -54,16 +54,26 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli
? new NodesCapabilitiesRequest(client.getLocalNodeId())
: new NodesCapabilitiesRequest();

// Handle the 'path' parameter safely, assign an empty string if null
String path = request.param("path");
if (path != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of checking for null, we can just use the default value parameter:

.path(request.param("path", "/"))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @thecoop,

I've made the requested change to use request.param("path", "/") as the default value for the path parameter instead of checking for null. Please let me know if there are any further adjustments needed.

Thank you!

@thecoop
Copy link
Member

thecoop commented Sep 27, 2024

@elasticmachine test this

@thecoop
Copy link
Member

thecoop commented Sep 27, 2024

@nbenliogludev Could you update your branch with latest from main? The changelog also needs to be updated - run ./gradlew :validateChangelogs for more info

@thecoop
Copy link
Member

thecoop commented Sep 27, 2024

@elasticmachine update branch

@thecoop
Copy link
Member

thecoop commented Sep 27, 2024

@elasticmachine test this

@nbenliogludev
Copy link
Contributor Author

@thecoop, All checks have passed, and the branch is up to date. Could I please get a review and approval for the merge?

@thecoop thecoop merged commit 929b388 into elastic:main Sep 27, 2024
16 checks passed
@thecoop
Copy link
Member

thecoop commented Sep 27, 2024

All merged, thanks for the fix!

matthewabbott pushed a commit to matthewabbott/elasticsearch that referenced this pull request Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/REST API REST infrastructure and utilities external-contributor Pull request authored by a developer outside the Elasticsearch team Team:Core/Infra Meta label for core/infra team v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants