Skip to content

Support for Free-Form Query Parameters #295

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

Closed
bowenwr opened this issue Jan 12, 2021 · 8 comments · Fixed by #316 or benchling/openapi-python-client#39
Closed

Support for Free-Form Query Parameters #295

bowenwr opened this issue Jan 12, 2021 · 8 comments · Fixed by #316 or benchling/openapi-python-client#39
Labels
✨ enhancement New feature or improvement

Comments

@bowenwr
Copy link
Contributor

bowenwr commented Jan 12, 2021

Is your feature request related to a problem? Please describe.

Given an API that accepts arbitrarily-named query parameters like:

/my-endpoint/?dynamic_param1=value&dynamic_param2=value2

We'd like to be able to append arbitrary key/values to the query string search.

Given a current YAML snippet like:

    parameters:
        - in: query
          name: dynamicFields
          schema:
            type: object
            additionalProperties: true

The parameter generated is schema_field: Union[Unset, ModelNameSchemaField] = UNSET, and it's also sent as the parameter named schema_field instead of using the arbitrary keys.

Describe the solution you'd like

Generate the parameter above with additionalProperties as schema_field: Union[Unset, None, Dict[str, Any]] = UNSET. When schema_field is a dict, it will then send values for all keys in the query parameters instead of as schema_field.

If multiple parameters are defined having additionalProperties, it will treat all of them as arbitrary keys. If two parameters were to define the same dynamically named key, we make no guarantees about which one is sent. I imagine it would be the last parameter encountered with additionalProperties. Alternatively, we could raise an exception instead of making silent assumptions about the collision.

Describe alternatives you've considered

Rather than modeling the field in Open API, allow every GET method to accept an additional_properties: Dict[str, str] parameter which would append all the keys as query parameters. The name additional_properties might need to be configurable to avoid collision with APIs using that parameter name already.

@bowenwr bowenwr added the ✨ enhancement New feature or improvement label Jan 12, 2021
@bowenwr
Copy link
Contributor Author

bowenwr commented Jan 12, 2021

@dtkav Any thoughts here?

@dtkav
Copy link
Contributor

dtkav commented Jan 13, 2021

Sound right to me. Note that in additionalProperties you can also define a valid type for the value.
Here's an example from the OAS 3.0 spec:

in: query
name: freeForm
schema:
  type: object
  additionalProperties:
    type: integer
style: form

There may be some official guidance on what to do in the case that multiple query params are free-form objects.

@dtkav
Copy link
Contributor

dtkav commented Jan 13, 2021

Also FYI @bowenwr - OpenAPI 3.1 adds patternProperties, which I believe will allow us to define a common prefix for a parameter, such as fields.*.

@dbanty
Copy link
Collaborator

dbanty commented Jan 16, 2021

So I actually don't know what the generated clients will do with objects in query params right now, my guess is the wrong thing 😁. I think to do this right we want to start supporting and enforcing the style and explode attributes for properties. Then we treat additionalProperties like any other properties and you can define how you want them. So passing an object to a query with style: "form" (the default) and explode: true would get you the ?dynamic_param1=value&dynamic_param2=value2 you're looking for.

The default behavior (what we do today since we don't support style & explode) should technically be ?freeForm=dynamic_param1,value,dynamic_param2,value2. So at a minimum we should make sure this is our default behavior for objects.

More Info

  1. We already have an issue Support array query parameters with explode #197 related to explode that would also be solved by a more general solution.
  2. Search for "Style Examples" on https://swagger.io/specification/ to see more combos that might interest you

@forest-benchling
Copy link
Collaborator

forest-benchling commented Jan 20, 2021

Indeed, it has a very weird behavior right now:

# Set HTTPX_LOG_LEVEL=debug
>>> httpx.request('GET', 'https://httpbin.org/get', params={"dynamicParams": {"param1": "val1"}})
DEBUG [2021-01-19 21:16:51] httpx._client - HTTP Request: GET https://httpbin.org/get?dynamicParams=%7B%27param1%27%3A+%27val1%27%7D "HTTP/1.1 200 OK"
<Response [200 OK]>

From my reading of the httpx documentation, it seems like it won't handle any serialization for us. It allows you to pass in params as a string, but then it does some manipulation:

>>> httpx.request('GET', 'https://httpbin.org/get', params=".blue.black.brown")
DEBUG [2021-01-19 21:18:42] httpx._client - HTTP Request: GET https://httpbin.org/get "HTTP/1.1 200 OK"
<Response [200 OK]>
>>> httpx.request('GET', 'https://httpbin.org/get', params=";blue=brown")
DEBUG [2021-01-19 21:19:50] httpx._client - HTTP Request: GET https://httpbin.org/get?blue=brown "HTTP/1.1 200 OK"
<Response [200 OK]>

So I'm thinking we want to stop using params in the httpx call, and construct the URL with parameters appended ourselves. Does that sound right @dbanty @bowenwr ?

@forest-benchling
Copy link
Collaborator

forest-benchling commented Jan 20, 2021

@dbanty @bowenwr Please check out this code plan before I start implementation:

Code Plan:

  • Add style and explode to class Property
    • Default value of explode depends on style, this can be done in __post_init__
    • Default value of style depends on in
  • Thread through style and explode correctly from Parameter into _property_from_data, etc.
  • Serialize parameters into URL (based on style and explode) in endpoint_module.pyi. For this issue, headers, cookies, and path will be out of scope as I don't know how to get it to work with httpx.
  • E2E Tests: Should I just add a variety of different parameters in openapi.json? Anything else I need to do—-does the test just run mypy and that’s it?

Additionally, I would like to propose calling it out of scope to change the type signature from schema_field: Union[Unset, ModelNameSchemaField] = UNSET to schema_field: Union[Unset, None, Dict[str, Any]] = UNSET. Would that still be usable enough @bowenwr? I think the style and explode changes are big enough as-is, and the type signature change seems to be a broader question of using Dict for inline object schemas rather than creating a new model, rather than something specific to query parameters.

@bowenwr
Copy link
Contributor Author

bowenwr commented Jan 20, 2021

@forest-benchling That should still work for our purposes. Thanks!

@forest-benchling
Copy link
Collaborator

After going down this path, I realized that today we already only generate query params with the behavior of style=form, explode=True. So it would be consistent to make Dicts behave that way as well, even without needing to add explicit support for style and explode yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or improvement
Projects
None yet
4 participants