Skip to content

Remove base rest handler unconsumed parameter filter to force response parameter validation.#94711

Open
howardhuanghua wants to merge 6 commits intoelastic:mainfrom
TencentCloudES:rm_resp_filter
Open

Remove base rest handler unconsumed parameter filter to force response parameter validation.#94711
howardhuanghua wants to merge 6 commits intoelastic:mainfrom
TencentCloudES:rm_resp_filter

Conversation

@howardhuanghua
Copy link
Copy Markdown
Contributor

This PR is a follow-up enhancement for #94136.
Remove base rest handler unconsumed parameter filter and force validating response parameters in rest prepare request layer.

@elasticsearchmachine elasticsearchmachine added v8.8.0 needs:triage Requires assignment of a team area label labels Mar 24, 2023
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

@howardhuanghua please enable the option "Allow edits and access to secrets by maintainers" on your PR. For more information, see the documentation.

@elasticsearchmachine elasticsearchmachine added the external-contributor Pull request authored by a developer outside the Elasticsearch team label Mar 24, 2023
@dnhatn dnhatn added the :Core/Infra/REST API REST infrastructure and utilities label Mar 24, 2023
@elasticsearchmachine elasticsearchmachine added Team:Core/Infra Meta label for core/infra team and removed needs:triage Requires assignment of a team area label labels Mar 24, 2023
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Member

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

So yes I think we should validate "response" params up front, there is no point in processing a request and then blowing up when serialising the response because one of these parameters was invalid. But we have to actually do some meaningful validation, we can't just blindly consume the parameters like this PR proposes. For instance we should check that boolean parameters will parse as booleans, ClusterState$Metric won't throw an IllegalArgumentException, and so on...

@howardhuanghua
Copy link
Copy Markdown
Contributor Author

But we have to actually do some meaningful validation

OK. I would do case by case validation. That should be a huge project.

@DaveCTurner
Copy link
Copy Markdown
Member

I think it's not all that big a task, there's only these things that I can see:

  • boolean parameters. That's almost all of them, and these are easy to validate
  • AbstractCatAction#RESPONSE_PARAMS (some of which are boolean)
  • ?settings_filter (just a raw string, no real validation needed)
  • ?delimiter as used in the SQL code

@howardhuanghua
Copy link
Copy Markdown
Contributor Author

OK. I am going to add specific validations.

@howardhuanghua
Copy link
Copy Markdown
Contributor Author

Hi @DaveCTurner , sorry for the delay. I have updated this PR and added related pre-validation logic for response paramaters. Would you please help to check again? Thanks.

@gmarouli gmarouli added v8.9.0 and removed v8.8.0 labels Apr 26, 2023
@quux00 quux00 added v8.11.0 and removed v8.10.0 labels Aug 16, 2023
@mattc58 mattc58 added v8.12.0 and removed v8.11.0 labels Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

: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.4.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants