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

feat: Support other content-types for HTTP streams #2761

Closed
edgarrmondragon opened this issue Nov 13, 2024 · 0 comments · Fixed by #2762
Closed

feat: Support other content-types for HTTP streams #2761

edgarrmondragon opened this issue Nov 13, 2024 · 0 comments · Fixed by #2762
Assignees
Labels
kind/Feature New feature or request valuestream/SDK
Milestone

Comments

@edgarrmondragon
Copy link
Collaborator

Feature scope

Taps (catalog, state, tests, etc.)

Description

The RESTStream implementation assumes that request and response bodies are JSON. This is a reasonable default, but it currently makes it harder than it should it be to override these assumptions for other types of APIs, like ones based on XML.

We hardcode the json= parameter common across many of requests APIs here:

return self.build_prepared_request(
method=http_method,
url=url,
params=params,
headers=headers,
json=request_data,
)

By looking at the requests source1, here

        if not data and json is not None:
            # urllib3 requires a bytes-like body. Python 2's json.dumps
            # provides this natively, but Python 3 gives a Unicode string.
            content_type = "application/json"

            try:
                body = complexjson.dumps(json, allow_nan=False)
            except ValueError as ve:
                raise InvalidJSONError(ve, request=self)

            if not isinstance(body, bytes):
                body = body.encode("utf-8")

and here

            # Add content-type if it wasn't explicitly provided.
            if content_type and ("content-type" not in self.headers):
                self.headers["Content-Type"] = content_type

it's clear that the same default behavior can be achieved by instead using the data= parameter and serializing the JSON payload ourselves. We might also be able to do this in a non-breaking way.

Footnotes

  1. https://requests.readthedocs.io/en/latest/_modules/requests/models/#PreparedRequest.prepare_body

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/Feature New feature or request valuestream/SDK
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant