swagger-api / swagger-codegen

swagger-codegen contains a template-driven engine to generate documentation, API clients and server stubs in different languages by parsing your OpenAPI / Swagger definition.
http://swagger.io
Apache License 2.0
17.04k stars 6.03k forks source link

[typescript\angular2] Deal with 201 response status #4397

Open jeusdi opened 7 years ago

jeusdi commented 7 years ago
Description

Currently, typescript\angular2 api.service.mustache template handles 204 responses status in order return an undefined value directly instead of call a response.json:

return this.createWithHttpInfo(user, passwd, extraHttpRequestParams)
            .map((response: Response) => {
                if (response.status === 204) {
                    return undefined;
                } else {
                    return response.json();
                }
            });

I've implemented an JAXRS RESt endpoint like this:

@Override
    public Response create(String username, String passwd) throws UsernameAlreadyExistsCommtyException, RepositorySystemException {
        String userId = this.userService.create(username, passwd);
        return Response.created(this.uriInfo.getAbsolutePathBuilder().path(userId).build()).build();
    }

As you can see, this method returns an create-201 response status code. So, there's no content inresponse's body. Instead, there's a location header.

Could you provide an straightforward approach to deal with this?

Swagger-codegen version

2.2.3-SNAPSHOT

wing328 commented 7 years ago

@jeusdi would returning response.json() for status code 204 meet your requirement?

Given that the function call is for obtaining HTTP info (status code, headers, body), it should not return undefined even if the status code is 204

agoncal commented 7 years ago

Using a Response.created is a nice pattern in POST : it does not return the entity (saves bandwith), the status code is 201 (more expressive), the location is set in the header, therefore, the body is empty. Perfect.

So with the code at the moment, return response.json() actually breaks. It would be better to handle the 201. Something like :

      .map((response: Response) => {
        switch (response.status) {
          case 204 :
            return undefined;
          case 201 :
            return response;
          default :
            return response.json();
        }

WDYT ? In case of 201 it returns the response (without jsonifying it, as it is empty) so we can check the header location.

wing328 commented 7 years ago

@agoncal may I know if you've time to contribute the enhancement?

https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/resources/typescript-angular2/api.mustache#L47

acramatte commented 7 years ago

@agoncal that would only cover the case where 201 returns no body right ? I may be missing something but to me the specs don't specify whether 201 should return a body or not.

The solution I have used so far was to use a dummy try - catch

.map((response: Response) => {
    try {
        return response.json();
    } catch(error) {
        return undefined
    }
});
agoncal commented 7 years ago

@wing328 Yes, I think I could contribute. But @acramatte brings an interesting question. As we know, in REST, there are thousands of way to do the same thing ;o) With JAX-RS, when you create a new resource you are supposed to use the created method and you must pass a URI because the signature is :

    public static ResponseBuilder created(URI location) {
        return status(Status.CREATED).location(location);
    }

But you can also add the entity if you want to :

Response.created(createdURI).entity(createdEntity).build();

So, we can have a 201 with no body (just a location) and a 201 with both a body and a location. So maybe the solution of @acramatte is better (or something like it).

WDYT guys ?

wing328 commented 7 years ago

So maybe the solution of @acramatte is better (or something like it).

@agoncal that approach looks good to me too as we use similar approach in other API client(s) if I remember correctly.

koppor commented 7 years ago

@agoncal In that case, a good client would have like access to both? The server-generated resource location (header) and the new entity.

I would favor a special response object in the case of 201:

{
  'location': response.headers.get('location'),
  'data': response.json()
}

I am aware that this is a complex object and is not as easy to handle as the other things.

In summary, I see following solutions:

1) Solution as above - complex object 2) Returning the new location in case no body exists + Returning .json() in case a body exists. 3) Parameterizing the code generation to do either 1) or 2)

2) Has the drawback if both the body and the new location are relevant, only one of them can be accessed.

Finally, could you explain to me what @acramatte solution (https://github.com/swagger-api/swagger-codegen/issues/4397#issuecomment-288116653) is like? How can I access the location header if I get undefined back in the case of no body?

For me, solution 2) is fine as I return back the location header only.

koppor commented 7 years ago

This issue is a follow up on https://github.com/swagger-api/swagger-codegen/issues/990.

acramatte commented 7 years ago

@koppor The client has two methods per endpoint e.g.: 1) public getUsers(...): Observable {} 2) public getUsersWithHttpInfo(...): Observable< Response > {}

If you need access to the header you would use the 2nd option right ? Then it is up to your application to deal with undefined. Or am I missing something ?

koppor commented 7 years ago

@acramatte Oh, you are right. An update from 2.2.1 to 2.2.3 made it generate the WithHttpInfo() methods.