Skip to content
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

Handle exceptions more consistently #11

Open
judgej opened this issue Jun 28, 2019 · 2 comments
Open

Handle exceptions more consistently #11

judgej opened this issue Jun 28, 2019 · 2 comments
Assignees
Labels
feature Feature or functionality

Comments

@judgej
Copy link
Member

judgej commented Jun 28, 2019

Something that has been bugging me, and I think I finally figured out a way forward.

The original operations generated by this code would throw exceptions for any non 2xx response. The exception would contain the raw response payload.

OpenAPI allows for any response status code to include a structured payload. By throwing an exception, the generated code was not parsing (deserialising) the payload into an object, so the payload was effectively lost.

However, returning the payload in the event of an error without throwing an exception is also not ideal. For example, hitting an endpoint that returns a 404 or 400, but fails to deliver a specified RFC7807 payload with that, results in the application getting a totally empty RFC-7807. That empty message does not even contain the response code, so the applications sees this as a 200 response, which it patently isn't. It has no access to the underlying raw response, only the deserialised (and useless, empty) model object.

The generated API has two main entry points to an operation: operation() and operationWithHttpInfo(). Both do exactly the same, but the second returns an array with the response status code and response headers, as well as the deserialised payload. There are several things I will change here:

  • Returning the headers and response code is a bit presumptuous about what may be needed. Instead, just return the full PSR-7 Response object. The application can take out what it needs.
  • If operationWithHttpInfo() is returning the raw response as well as any payload object (if there is a payload object), then it has no need to throw any exception for any response code, since all response codes are "valid", they come with a PSR-7 context. So all exceptions should be removed from that part of the operation.
  • The operation() response has no PSR-7 context; the application gets a model object, a scalar value or a null in response. The HTTP status, raw payload, headers etc. are not returned. It is here that an exception can be thrown for non-2xx responses. The exception should contain the PSR-7 response, and the deserialised payload (or null). So an application can catch an exception, see thatit's a 400 for example, look to see if there is a payload, say an RFC-7807 detail, and act accordingly.

That's how I believe the exceptions and operations should be structured.

@judgej judgej self-assigned this Jun 28, 2019
@judgej judgej added the feature Feature or functionality label Jun 28, 2019
@judgej
Copy link
Member Author

judgej commented Jun 28, 2019

The object de-serialiser should also be able to accept a PSR-7 Response or ServerResponse object directly. It can then work out how the payload it formatted (JSON, XML, etc.) correctly matched with the response headers, and whether it has a file stream or a structured data.

This should make the main API class a lot simpler too, removing a lot of repeated code.

judgej added a commit that referenced this issue Jun 28, 2019
* Do not catch exceptions from the client (it's PSR-18, so non-2xx
  responses do not result in errors.
* Do not throw exceptions in `operationWithHttpInfo()` methods at all.
* ApiException extended to accept request, response and deserialised
  response where available.
* Exceptions move to only the `operation()` methods.
@judgej
Copy link
Member Author

judgej commented Jun 28, 2019

These changes include:

  • Disabling async requests. We don't use them, there is no PR standard for them.
  • The deserialise function now accepts a PSR-7 response, which it will unwrap into a payload (model object, scalars, file stream, null).
  • The file stream response needs some work. It has not been tested as we do not use it at this time. I expect it could be a lot simpler, with a PSR-7 StreamInterface returned without the need to convert it into a SplFileObject.
  • The deserialise function needs some work when unwrapping a PSR-7 RequestInterface object, to ensure it checks the message headers are appropriate when unwrapping (no point running json_decode() if the response is not declared as JSON).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature or functionality
Projects
None yet
Development

No branches or pull requests

1 participant