tazatechnology / openapi_spec

Dart based OpenAPI specification generator and parser
BSD 3-Clause "New" or "Revised" License
9 stars 6 forks source link

Request returns void when response body is empty #47

Closed mthongvanh closed 11 months ago

mthongvanh commented 11 months ago

When the path entry returns only a status code the generated request returns void.

I think it would be better to return a response object, because I think most users will expect a success response. If they forget to wrap in a try-catch they might miss errors.

Considering the current implementation, I am not sure if this is feasible or not.

One option would be to have the custom response object implement BaseResponse and pass the http library's response into the custom class' constructor.

If you think this is a worthwhile enhancement, I would happy to take a look.

Here's an example with only a status code:

 /copy:
    post:
      tags:
      - model management
      summary: Copy a Model
      description: Copy a model. Creates a model with another name from an existing model.
      operationId: copyModel
      requestBody:
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/CopyRequest'
      responses:
        '200':
          description: OK
davidmigloz commented 11 months ago

I think the client should basically abstract all the HTTP-related stuff. So, in this case, I believe it is correct to return just void. Same as if you call list.add('item') in dart, it will return void or throw an exception if the list is unmodifiable.

Another topic is whether throwing exceptions vs returning a Result wrapper (like Either) is a better approach. However the standard error handling approach from Dart is using exception, so I believe if we want to be as pure as possible that should be the way to go (users can always create their own wrappers using Either if they want).

mthongvanh commented 11 months ago

i can see where you are coming from in terms of the client and trying to keep it similar to other dart APIs. also, i agree that a result wrapper would be nicer, so i think that's why i find it strange not to propagate the empty 200 response.

walsha2 commented 11 months ago

I find it strange not to propagate the empty 200 response

What is the end user going to do with a propagated 200 response with no content? Is there a practical use case?

I think the client should basically abstract all the HTTP-related stuff.

This is the driving design principal here and my main frustration with almost every other client generator. Mission: "Take my HTTP API architecture, as defined by an OpenAPI spec, and convert it into callable, typed (custom schemas), Dart code and not have to burden the end user with any of the inner workings of the HTTP call or response parsing"

I am not particularly keen on mixing HTTP responses with defined types in the function returns of the client. I have not really seen any client, in any language, do this really. As an example, the OpenAI Python client offers access to the responses, although the user needs to be explicit. The top level, primary client, abstracts away all the HTTP aspects and if you want the responses you need to use . with_raw_response

https://github.com/openai/openai-python#accessing-raw-response-data-eg-headers


I would also like to point out that the generated client provides an onRequest and onResponse middleware function that the end user can override to capture the responses/requests for any call. So maybe this is something that does not live in the generated client, but maybe something that the end user extends?

mthongvanh commented 11 months ago

What is the end user going to do with a propagated 200 response with no content? Is there a practical use case?

Just to verify that the operation did indeed complete as expected, but I think that the onResponse would cover that use case.

Understanding the project a little better, I can see how the change doesn't really fit. Thanks for clarifying!