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

Fix: GET _mapping with index in query. #385

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

dblock
Copy link
Member

@dblock dblock commented Jul 8, 2024

Description

OpenSearch supports GET _mapping?index=.... If you specify it both on the path and query the query is ignored.

$ curl -u admin:$OPENSEARCH_PASSWORD -k https://localhost:9200/_mapping?index=games | jq

{
  "games": {
    "mappings": {}
  }
}
$ curl --silent -u admin:$OPENSEARCH_PASSWORD -k https://localhost:9200/games/_mapping?index=foobars | jq
{
  "games": {
    "mappings": {}
  }
}

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Contributor

github-actions bot commented Jul 8, 2024

Changes Analysis

Commit SHA: 829bbd8
Comparing To SHA: 667091c

API Changes

Summary


├─┬Paths
│ ├─┬/{index}/_mapping
│ │ └─┬GET
│ │   └─┬Parameters
│ │     └──[🔀] in (16587:11)❌ 
│ └─┬/_mapping
│   └─┬GET
│     └──[➕] parameters (16588:13)❌ 
└─┬Components
  └──[➕] parameters (16587:7)

Document Element Total Changes Breaking Changes
paths 2 2
components 1 0
  • BREAKING Changes: 2 out of 3
  • Modifications: 1
  • Additions: 2
  • Breaking Modifications: 1
  • Breaking Additions: 1

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/9899446155/artifacts/1693094826

API Coverage

Before After Δ
Covered (%) 483 (47.31 %) 483 (47.31 %) 0 (0 %)
Uncovered (%) 538 (52.69 %) 538 (52.69 %) 0 (0 %)
Unknown 24 24 0

@dblock dblock force-pushed the test-mappings branch 3 times, most recently from 35f644d to a3e9772 Compare July 8, 2024 17:24
@dblock dblock changed the title Test index mappings. Fix: GET _mapping with index in query. Jul 8, 2024
@dblock dblock force-pushed the test-mappings branch 2 times, most recently from 6a759f1 to 9ec6b34 Compare July 8, 2024 17:44
Comment on lines 1291 to 1293
operationId: indices.get_mapping.1
x-operation-group: indices.get_mapping
operationId: indices.get_index_mapping.0
x-operation-group: indices.get_index_mapping
Copy link
Collaborator

Choose a reason for hiding this comment

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

Renaming this will cause issues for client generation, as these two are already treated as the same operation merely picking between the paths based on index being specified or not

Copy link
Member Author

Choose a reason for hiding this comment

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

How should I solve this? The two APIs have effectively different parameters, as the _mapping API takes an index query parameter and the {index}/_mapping API does not. The linter complains that the two must have the same parameters.

Copy link
Collaborator

Choose a reason for hiding this comment

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

.NET's logic is that it unions all parameters from all variants and just prefers using the path over the query string if two parameters with the same name but different locations exist. Maybe we could loosen the linter slightly to just validate the variants have the same set of named parameters regardless of location?

Is there any reason to ever use this query string variant over the path variant?

Copy link
Member Author

Choose a reason for hiding this comment

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

No reason, but the API supports it. We could of course not document it, but 🤷‍♂️

WDYT @nhtruong?

Copy link
Collaborator

@nhtruong nhtruong Jul 11, 2024

Choose a reason for hiding this comment

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

@dblock Our clients support optional path parameters. Operations of the same group are only required to share an identical set of querystring params. The only requirement for path params is that if 2 operations have some overlapping path params, then those params must be identical.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The Clients Generator Guide touched on this:

If two operations have identical HTTP methods, but different paths: use the path that best matches the path parameters provided.

Copy link
Collaborator

@nhtruong nhtruong Jul 11, 2024

Choose a reason for hiding this comment

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

How should I solve this? The two APIs have effectively different parameters, as the _mapping API takes an index query parameter and the {index}/_mapping API does not. The linter complains that the two must have the same parameters.

The clients prioritize path parameters. If index is provided in the clients.indices.get_mapping({ index: 'books' }) then index will be extracted as a path param, and /{index}/_mapping will be used.

From the user's standpoint, there's not distinction between path params and query params. They are all params, including body.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ended up just adding it to possible query parameters, thanks both!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Xtansia The clients gen will need to account for this situation when there are path and query params that share the same names. Especially for method documentation and typing for the params object. Should we also enforce that params of the same name, regardless of whether they are path or query, should have identical description and schema?

@dblock dblock marked this pull request as draft July 10, 2024 16:08
@dblock dblock marked this pull request as ready for review July 11, 2024 22:38
@nhtruong nhtruong merged commit c6fbaf9 into opensearch-project:main Jul 11, 2024
12 checks passed
@dblock dblock deleted the test-mappings branch July 11, 2024 22:42
Comma-separated list of data streams, indices, and aliases used to limit the request.
Supports wildcards (`*`).
To target all data streams and indices, omit this parameter or use `*` or `_all`.
required: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

I missed this. it should have been false

Copy link
Member Author

Choose a reason for hiding this comment

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

Yikes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants