Is your feature request related to a problem? Please describe.
In #16694, I tried to add typing information on the generated API clients, but the issues are a bit larger than expected.
Going from bottom (REST client) to up (the model API client):
REST API client
By "REST API client", I mean the RESTClientObject object.
The REST API client request() method can return very different type of responses:
- A basic
ApiResponse, a generic HTTP response
- Any kind of higher-level object via
response_types_map: the client can extract the payload of the response and deserialize it into any kind of object if the payload matches the format of that object (so, an int, datetime, or any model; but actually, almost any type)
This has several issues:
- This high variability in the kind of responses the client can return makes it very difficult to type correctly.
- The different response types change depending on parameters passed to the REST client. This makes the typing even more difficult to implement and pushes all the type verification to the client of the object.
- Finally, it adds the higher-level deserialization logic of the response into the REST Client, which could be seen more as a low-level client.
Model API client
By "Model API client", I mean the API client created for each model.
I'm taking a hypothetical Foo model with basic CRUD methods as an example, that would produce a FooApi class.
We can find the following kind of methods:
- A
get_foo_by_id method returning:
Foo (usual case)
None in some cases, sometimes for errors too
- A
get_foo_by_id_with_http_info, which could return either:
- An
ApiResponse: a generic HTTP response
- An instance of the associated
Foo model, if _return_http_data_only is False
- There are other methods that can return more complex objects, like
List[Foo], etc. but the mechanism is the same as explained above.
In addition, the urllib3 client can also produce an AsyncResult with one of the 2 behaviors explained just above.
This behavior doesn't exist for the aiohttp-based client.
This also has several issues, similar to those of the underlying REST Client explained above:
- This high variability in the kind of responses the client can return makes it very difficult to type correctly.
- The behavior can change drastically, depending on the parameters passed
- The deserialization logic is done in the REST Client, although it's only (mostly) used in the high-level APIs
get_foo_by_id and similar.
- The
AsyncResult responses create additional complexity in the typing of the responses, since it also depends on the parameter async_req to be set.
Describe the solution you'd like
After some good suggestions from @fa0311, @robertschweizer and @wing328 , I'd like to make the propose the following changes:
Remove all the parameters that change the response type and move the associated behaviors somewhere else:
Remove _return_http_data_only
Remove the deserialization behavior away from the RESTClientObject class and move it into the xxx() methods of the model API client:
1. The xxx_with_http_info() methods would not be able to deserialize the response to high-level objects anymore
2. The xxx_with_http_info() methods would only return ApiResponses object instead
3. The deserialization logic would be moved outside of the REST Client:
1. It actually doesn't have any state and could be free functions instead.
2. This can all be moved to a dedicated deserialization module.
Remove async_req
Extract the AsyncResult into dedicated *_async() methods.
- This means that, for each
xxx() + xxx_with_http_info() methods, this would add 2 new methods:
xxx_async()
xxx_with_http_info_async()
- The behavior of the 2 new methods would be the same as the original methods, except the processing of the response would be done only when the actual result is fetched from the
AsyncResult object.
- This would require to refactor a bit the code to make these behaviors reusable beteen
xxx()/xxx_async() and xxx_with_http_info()/xxx_with_http_info_async():
- The deserilization logic for a
xxx() method would be moved into a _xxx_deserialization() method
xxx() would call this method directly before returning its result
xxx_async() would call this method when the result is fetched.
Create a new ApiRequest object
Finally, to makes it easier to reuse the code between the sync/async methods, I'd like to introduce a new ApiRequest object:
- The behavior from the
*_with_http_info() would be moved to a dedicated "private" method
- This method would take all the arguments and build a new
ApiRequest object containing the right parameters to execute the request
- This method could be called both by the
xxx_with_http_info() and the xxx_with_http_info_async() methods
- The REST Client, instead of taking many parameters, now only takes the
ApiRequest object and uses whatever it needs from that.
Additional context
These changes are not really backward compatible. All the parameters removed will not be supported anymore.
Each changes could be done separately, although the typing benefits would only be seen once all the changes have been done
Ideally, the best order would be:
- introducing the new
ApiRequest object and use it both from the *_with_http_info() methods and the REST Client. The REST Client can be kept backward compatible for the time being.
async_req can be extracted then be extracted. This simply duplicates the methods, removes the parameter and reuses the ApiRequest created above.
- The deserialization logic can finally be moved and the typing simplified
Is your feature request related to a problem? Please describe.
In #16694, I tried to add typing information on the generated API clients, but the issues are a bit larger than expected.
Going from bottom (REST client) to up (the model API client):
REST API client
The REST API client
request()method can return very different type of responses:ApiResponse, a generic HTTP responseresponse_types_map: the client can extract the payload of the response and deserialize it into any kind of object if the payload matches the format of that object (so, an int, datetime, or any model; but actually, almost any type)This has several issues:
Model API client
I'm taking a hypothetical
Foomodel with basic CRUD methods as an example, that would produce aFooApiclass.We can find the following kind of methods:
get_foo_by_idmethod returning:Foo(usual case)Nonein some cases, sometimes for errors tooget_foo_by_id_with_http_info, which could return either:ApiResponse: a generic HTTP responseFoomodel, if_return_http_data_onlyisFalseList[Foo], etc. but the mechanism is the same as explained above.In addition, the
urllib3client can also produce anAsyncResultwith one of the 2 behaviors explained just above.This behavior doesn't exist for the
aiohttp-based client.This also has several issues, similar to those of the underlying REST Client explained above:
get_foo_by_idand similar.AsyncResultresponses create additional complexity in the typing of the responses, since it also depends on the parameterasync_reqto be set.Describe the solution you'd like
After some good suggestions from @fa0311, @robertschweizer and @wing328 , I'd like to make the propose the following changes:
Remove all the parameters that change the response type and move the associated behaviors somewhere else:
Remove
_return_http_data_onlyRemove the deserialization behavior away from the
RESTClientObjectclass and move it into thexxx()methods of the model API client:1. The
xxx_with_http_info()methods would not be able to deserialize the response to high-level objects anymore2. The
xxx_with_http_info()methods would only returnApiResponses object instead3. The deserialization logic would be moved outside of the REST Client:
1. It actually doesn't have any state and could be free functions instead.
2. This can all be moved to a dedicated
deserializationmodule.Remove
async_reqExtract the
AsyncResultinto dedicated*_async()methods.xxx()+xxx_with_http_info()methods, this would add 2 new methods:xxx_async()xxx_with_http_info_async()AsyncResultobject.xxx()/xxx_async()andxxx_with_http_info()/xxx_with_http_info_async():xxx()method would be moved into a_xxx_deserialization()methodxxx()would call this method directly before returning its resultxxx_async()would call this method when the result is fetched.Create a new
ApiRequestobjectFinally, to makes it easier to reuse the code between the sync/async methods, I'd like to introduce a new
ApiRequestobject:*_with_http_info()would be moved to a dedicated "private" methodApiRequestobject containing the right parameters to execute the requestxxx_with_http_info()and thexxx_with_http_info_async()methodsApiRequestobject and uses whatever it needs from that.Additional context
These changes are not really backward compatible. All the parameters removed will not be supported anymore.
Each changes could be done separately, although the typing benefits would only be seen once all the changes have been done
Ideally, the best order would be:
ApiRequestobject and use it both from the*_with_http_info()methods and the REST Client. The REST Client can be kept backward compatible for the time being.async_reqcan be extracted then be extracted. This simply duplicates the methods, removes the parameter and reuses theApiRequestcreated above.