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.01k stars 6.03k forks source link

PHP SDK codegen exception for 'default' response #5586

Closed adhocore closed 7 years ago

adhocore commented 7 years ago
Description

[PHP] Bug generating *Api::*WithHttpInfo()

For default response in the schema, TestApi::testWithHttpInfo() is generated as:

try {
  list($response, $statusCode, $httpHeader) = $this->apiClient->callApi(.......);
  return [..., $statusCode, $httpHeader];
} catch (ApiException $e) {
  switch ($e->getCode()) {
    case 200:
      $data = ...; break;
    case 0:
      $data = ...; break;
  }
  throw $e;
}

Should it not be like this one:

switch ($e->getCode()) {
  case 200:
    $data = ...; break;
  default:
    $data = ...; break;
}
Swagger-codegen version

2.2.2

Swagger declaration file content or url

https://pastebin.com/fcwebCHe

Command line used for generation

java -jar swagger-codegen-cli.jar generate -i resources/schema/spec.json -l php -Dmodel -o sdk/php

Steps to reproduce
Related issues
Suggest a Fix

Thanks :)

wing328 commented 7 years ago

@adhocore I believe non-2xx response will result in ApiException.

adhocore commented 7 years ago

i mean it is not about 2xx it is about case 0 please see the generated code section:

switch ($e->getCode()) {
    case 200:
      $data = ...; break;
    case 0:
      $data = ...; break;
}

there is very slim chance that non 2xx code is but 0. 0 is not even http status code. since we are defaulting anything except 200 to be caught as error, should it not be

switch ($e->getCode()) {
  case 200:
    $data = ...; break;
  default:
    $data = ...; break;
}
wing328 commented 7 years ago

The code usually refers to HTTP status code but it can contain 0 if my memory serves me well for the situation in which the error happens in the client side (e.g. network issue that the client cannot reach the server)

adhocore commented 7 years ago

that is network disconnected issue but is this the only case for defaulting? ofcourse no. all errors be it 0 or 4xx or 5xx are caught by default: section and it resonates to the schema too. but 0? :)

wing328 commented 7 years ago

@adhocore I think I know what you mean now. 'defaultresponse was incorrectly shown as 0. Like what you said, it should be mapped to thedefault` switch case.

Would you have time to contribute the fix? If yes, I can show you some good starting points.

arnested commented 7 years ago

I just had a look at this and it seems @wing328 fixed this earlier today in commit ef35b80bd873281266389941382a6ecea65eb17a.

wing328 commented 7 years ago

@adhocore let us know if you still see issues in the latest master.