Skip to content

Conversation

@pgomulka
Copy link
Contributor

Previously removed in #46943
parsing type field in term lookup is now possible with rest
compatible api. The type field is ignored

relates main meta issue #51816
relates type removal meta issue #54160

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
  • If submitting code, have you checked that your submission is for an OS and architecture that we support?
  • If you are submitting this code for a class then read our policy for that.
Previously removed in elastic#46943 parsing type field in term lookup is now possible with rest compatible api. The type field is ignored relates main meta issue elastic#51816 relates type removal meta issue elastic#54160
@pgomulka pgomulka self-assigned this Jun 24, 2021
@pgomulka pgomulka added :Core/Infra/REST API REST infrastructure and utilities v8.0.0 labels Jun 24, 2021
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jun 24, 2021
@elasticmachine
Copy link
Collaborator

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

@pgomulka pgomulka mentioned this pull request Jun 24, 2021
66 tasks
@pgomulka
Copy link
Contributor Author

this PR should also fix 'roles/30_prohibited_role_query/Test use prohibited query inside role query',
this test sends

security.put_role: name: "role" body: "{\n \"cluster\": [\"all\"],\n \"indices\": [\n {\n \"names\"\ : \"index\",\n \"privileges\": [\"all\"],\n \"query\" : {\n \ \ \"terms\" : { \"field\" : { \"index\" : \"_index\", \"type\" : \"_type\"\ , \"id\" : \"_id\", \"path\" : \"_path\"} }\n }\n }\n ]\n}\n" 

and should simply fail validation. However it fails on parsing a type field.
The reason is that, the validation is done in a transport layer.
https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleAction.java#L46

Ideally we should have the validation in the rest layer I think. Upon the creation of RoleDescriptor - would be able to parametrise the parser with a RestApiVersion

The alternative would be to serialise a RestApiVersion in PutRoleRequest and perform the validation as it is in transport layer.

Copy link
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

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

LGTM

@pgomulka pgomulka merged commit 90f2271 into elastic:master Jun 28, 2021
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Jul 19, 2021
…plate Previously the compatibility layer was alwayre returning an _doc for get template. This commit does not return _doc in mappings when mappings are empty. Returning just {} empty object also moving term lookups tests which are already fixed (relates elastic#74544) relates elastic#70966 relates main meta issue elastic#51816 relates types removal meta elastic#54160
pgomulka added a commit that referenced this pull request Jul 19, 2021
…plate (#75448) Previously the compatibility layer was always returning an _doc in mappings for get template. This commit does not return _doc in empty mappings. Returning just {} empty object (v7 and v8 behaviour) also moving term lookups tests which are already fixed (relates #74544) relates #70966 relates main meta issue #51816 relates types removal meta #54160
ywangd pushed a commit to ywangd/elasticsearch that referenced this pull request Jul 30, 2021
…plate (elastic#75448) Previously the compatibility layer was always returning an _doc in mappings for get template. This commit does not return _doc in empty mappings. Returning just {} empty object (v7 and v8 behaviour) also moving term lookups tests which are already fixed (relates elastic#74544) relates elastic#70966 relates main meta issue elastic#51816 relates types removal meta elastic#54160
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 >enhancement Team:Core/Infra Meta label for core/infra team v8.0.0-alpha1

4 participants