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

Allow unrestricted body again #2383

Merged
merged 2 commits into from Nov 30, 2023

Conversation

pquentin
Copy link
Member

@pquentin pquentin commented Nov 23, 2023

Closes #2181

Allows using the body parameter again, not only to allow performance optimizations and use cases where the elasticsearch-specification is not up to date, but also to facilitate migration from 7.x. See #2181 for a detailed discussion. This will be released as part of the next minor release of elasticsearch-py 8.x, likely early next year.

The following rules apply:

  • if the query does not accept body parameters, nothing changes
  • if the body and required body parameter(s) are unset (such as query for search), the query fails with a ValueError about the required parameter(s) (as we can't rely on typing or the default TypeError anymore)
  • if only the body is set, then it will be used without validation:
    • as is if already encoded as a string or bytes
    • or serialized to JSON if it's a mapping
  • if both the body and specific parameter(s) are set:
    • if at least one key is the same, the query fails with a ValueError exception because we don't know which one to use
    • if the keys are all different, the specific parameters are merged in the body, with a deprecation warning (while this could be an useful way to only use body for parameters missing from elasticsearch-specification, it's confusing and will be removed in a future version)

Note that the above was carefully written to take into account the case where:

  • a single parameter is used as a body:

    # those calls were already working
    es.index(index="test_index", document={"foo": "bar"})
    es.index(index="test_index", document=json.dumps({"foo": "bar"}))
    es.index(index="test_index", document=orjson.dumps({"foo": "bar"}))
    # and those are now possible
    es.index(index="test_index", body={"foo": "bar"})
    es.index(index="test_index", body=json.dumps({"foo": "bar"}))
    es.index(index="test_index", body=orjson.dumps({"foo": "bar"}))
  • multiple parameters are used to reconstruct the body:

    # this currently fails as `ignore_missing_component_templates` is not in the specification
    es.indices.put_index_template(
        name="my_template0",
        index_patterns=["my_index*"],
        data_stream={},
        composed_of=["my_data_stream", "my_data_stream_2"],
        ignore_missing_component_templates=["my_data_stream", "my_data_stream_2"],
        priority=1000,
    )
    # this works but the body isn't validated (passing as string and bytes as above works too)
    es.indices.put_index_template(
        name="my_template1",
        body={
            "index_patterns": ["my_index*"],
            "data_stream": {},
            "composed_of": ["my_data_stream", "my_data_stream_2"],
            "ignore_missing_component_templates": ["my_data_stream", "my_data_stream_2"],
            "priority": 1000,
        },
    )
    # using body *and* parameters works but is deprecated:
    es.indices.put_index_template(
        name="my_template2",
        body={"ignore_missing_component_templates": ["my_data_stream", "my_data_stream_2"]},
        index_patterns=["my_index*"],
        data_stream={},
        composed_of=["my_data_stream", "my_data_stream_2"],
        priority=1002,
    )

The useful commit to review is 7bae67e, as the other only updates the generated code.

@pquentin pquentin marked this pull request as ready for review November 28, 2023 12:57
@pquentin pquentin force-pushed the allow-unrestricted-body-again branch from 65713ad to 4c9d752 Compare November 28, 2023 14:14
Copy link
Member

@JoshMock JoshMock left a comment

Choose a reason for hiding this comment

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

This looks great! I looked at the one commit, 7bae67e, and it seemed pretty straightforward to me. 👍

@pquentin pquentin merged commit 23fb15d into elastic:main Nov 30, 2023
11 checks passed
@pquentin pquentin deleted the allow-unrestricted-body-again branch November 30, 2023 07:15
@pquentin
Copy link
Member Author

I looked at the one commit, 7bae67e, and it seemed pretty straightforward to me. 👍

Really appreciate this comment as I've made a few false starts and worked hard to make this straightforward. 🙇

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.

Please bring back body parameter to API calls
2 participants