Skip to content

Add SOAR-0007 proposal: Shorthand APIs for inputs and outputs #291

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

Merged

Conversation

simonjbeaumont
Copy link
Collaborator

@simonjbeaumont simonjbeaumont commented Sep 22, 2023

Motivation

We'd like to run a proposal for generating additional API to simplify providing operation inputs and handling
operation outputs for simple API calls with just one successful outcome.

Modifications

  • Add SOAR-0007: Shorthand APIs for operation inputs and outputs

(See the proposal itself for details)1

Result

n/a

Test Plan

n/a

Related Issues

#22, #104, #105, #145

Footnotes

  1. https://github.com/apple/swift-openapi-generator/pull/291/files

@gjcairo
Copy link
Contributor

gjcairo commented Sep 22, 2023

This proposal is now In Review, which will run until September 29 - feel free to leave your feedback either on this PR or on the corresponding Swift Forums post.

Thanks @simonjbeaumont for the write-up!

@yanniks
Copy link
Contributor

yanniks commented Sep 27, 2023

Also from my side, thank you for this proposal! I think it will really simplify many use-cases where the client component of the generator is used.
However, I'm wondering whether the thrown errors can be improved somehow. The challenge I see is that one might want to abstract error handling from the actual API call and catch Unauthorized or Forbidden responses in a certain way. With the current proposal, I don't see that there is a generic way of identifying whether the UnexpectedResponseError was thrown because of a 404, 403 or a 400 HTTP status code response. Could this be incorporated into this draft somehow?

@simonjbeaumont
Copy link
Collaborator Author

@yanniks thanks for taking the time to provide feedback!

I'm wondering whether the thrown errors can be improved somehow. The challenge I see is that one might want to abstract error handling from the actual API call and catch Unauthorized or Forbidden responses in a certain way. With the current proposal, I don't see that there is a generic way of identifying whether the UnexpectedResponseError was thrown because of a 404, 403 or a 400 HTTP status code response. Could this be incorporated into this draft somehow?

Could you clarify, in this event, what information would you need and what would you want to do with it?

Pulling out the relevant section of the proposal content to discuss further1:

    // A throwing computed property is generated for each documented outcome.
     var ok: Operations.getGreeting.Output.Ok {
         get throws {
             guard case let .ok(response) = self else {
                 // This error will be added to the OpenAPIRuntime library.
                 throw UnexpectedResponseError(expected: "ok", actual: self)
             }
             return response
         }
     }

For example, if there was also a documented 404 response then there would be an additional enum case, .notFound. If the response is .notFound, and you access the throwing getter .ok, the RuntimeError.unexpectedResponse will have self in it. Granted, it has been type-erased to Any and you cannot get at it through the RuntimeError, but it should suffice for debugging, e.g. logging.

This proposal is specifically for simplifying the API when you want to continue if you get a specific response and don't want to be defensive in the case that any other response is received.

In the event you do want to handle these other responses in a type-safe way, you can still do this, with the enum-based API. You can handle the ones you care about and use default for the ones you don't. And, you can choose to (or not) use the new shorthand for unpacking the content of the expected response.

switch try await client.getGreeting(/* ... */) {
case .ok(let response):
    print(try response.json.message)   // <--- using `.json` shorthand API
case .notFound(_):
    // ...
case .forbidden(_):
    // ...
case .badRequest(_):
    // ...
default:
    // handle other response..
}

If you're interested in always performing some behaviour for a given HTTP response regardless of the operation—e.g. logging errors, following redirects, or retrying on 503—then you should consider using a middleware for this.

Footnotes

  1. https://github.com/apple/swift-openapi-generator/pull/291/files#diff-ef10c19cd44e5402c57d42f931f871b1c462098e246b91bc42cae1ee8ff48da8R208-R217

@MahdiBM
Copy link
Contributor

MahdiBM commented Sep 28, 2023

Another related issue: #145 🙂

@simonjbeaumont
Copy link
Collaborator Author

Another related issue: #145 🙂

Thanks. I'd inconsistently referenced the issues between the proposal and the PR description 🤦 #145 was already referenced in the proposal itself, at least 🙂

  • Proposal: 22, 104, 145
  • PR description: 22, 104, 105

I'll clean both up to be the union.

@yanniks
Copy link
Contributor

yanniks commented Sep 28, 2023

@simonjbeaumont did not really think of using a middleware. In that case, I really support this proposal. Thanks for detailed reply!

@simonjbeaumont
Copy link
Collaborator Author

@simonjbeaumont did not really think of using a middleware. In that case, I really support this proposal. Thanks for detailed reply!

No problem!

simonjbeaumont added a commit to apple/swift-openapi-runtime that referenced this pull request Oct 3, 2023
### Motivation

The review period for SOAR-0007 (Shorthand APIs for inputs and outputs)
has now concluded. This pull request adds the required SPIs to the
runtime library to throw a runtime error when the response and/or body
does not match that of the shorthand API being used.

For further context, please review the proposal itself.[^1] 

[^1]: apple/swift-openapi-generator#291

### Modifications

- Extend `internal enum RuntimeError: Error` with two new cases:
    - `.unexpectedResponseStatus(expectedStatus:response:)`
    - `.unexpectedResponseBody(expectedContent:body:)`
- Add SPI for generated code, to throw these errors:
- `@_spi(Generated) public
throwUnexpectedResponseStatus(expectedStatus:response:)`
- `@_spi(Generated) public
throwUnexpectedResponseBody(expectedStatus:body:)`

### Result

Runtime library has two SPIs that can be used by the generator to
implement the shorthand throwing getter APIs described in SOAR-0007.

### Test Plan

Companion PR in swift-openapi-generator.

---------

Signed-off-by: Si Beaumont <[email protected]>
Co-authored-by: Honza Dvorsky <[email protected]>
simonjbeaumont added a commit that referenced this pull request Oct 3, 2023
…tputs (#308)

### Motivation

The review period for SOAR-0007 (Shorthand APIs for inputs and outputs)
has now concluded. This pull request adds the required SPIs to the
runtime library to throw a runtime error when the response and/or body
does not match that of the shorthand API being used.

For further context, please review the proposal itself.[^1] 

[^1]: #291

### Modifications

- Add support for computed properties with effects (e.g. throwing
getters).
- Generate a protocol extension to `APIProtocol` with an overload for
each operation that lifts each of the parameters of `Input.init` as
function parameters.
- Generate a throwing computed property for each enum case related to a
documented outcome, which will return the associated value for the
expected case, or throw a runtime error if the value is a different enum
case.

### Result

Code that used to be written like this 

```swift
// before
switch try await client.getGreeting(.init()) {
case .ok(let response):
    switch response.body {
    case .json(let body):
        print(body.message)
    }
case .undocumented(statusCode: _, _):
    throw UnexpectedResponseError()
}

// after
print(try await client.getGreeting().ok.body.json.message)
//                     ^          ^  ^       ^
//                     |          |  |       `- (New) Throws if body did not conform to documented JSON.
//                     |          |  |
//                     |          |  `- (New) Throws if HTTP response is not 200 (OK).
//                     |          |
//                     |          `- (New) No need to wrap parameters in input value.
//                     |
//                     `- (Existing) Throws if there is an error making the API call.
```

### Test Plan

This PR includes updates to the various tests:
- `SnippetBasedReferenceTests`
- `FileBasedReferenceTests`
- `PetstoreConsumerTests`

### Related Issues

- Resolves #22 
- Resolves #104 
- Resolves #145

---------

Signed-off-by: Si Beaumont <[email protected]>
@simonjbeaumont simonjbeaumont marked this pull request as ready for review October 3, 2023 17:42
@simonjbeaumont simonjbeaumont enabled auto-merge (squash) October 3, 2023 17:42
@simonjbeaumont
Copy link
Collaborator Author

@swift-server-bot test this please

@simonjbeaumont simonjbeaumont merged commit 5e47a80 into apple:main Oct 3, 2023
@czechboy0 czechboy0 added the semver/none No version bump required. label Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants