amzn/smoke-framework

body shouldn't be required for all requests

Closed this issue · 7 comments

robinsenior commented

Smoke will throw a validationError if the input doesn't contain a body, but AFAIK, HTTP doesn't require a body for all methods, e.g. GET.

tachyonics commented

Hi Robin, thanks for raising this issue. We need to develop a more complex mechanism in JSONPayloadHTTP1OperationDelegate[1] for mapping an input HTTP request to the input of an operation, which is necessary for requests such as GET where the input might be constructed from the query string and tokens in the path.

A completely custom implementation of the OperationDelegate[2] protocol could also achieve this but it make sense to provide this functionality out-of-the-box as part of JSONPayloadHTTP1OperationDelegate. I will keep this issue open and we look into developing a proposal for how to do this.

[1] https://github.com/amzn/smoke-framework/blob/master/Sources/SmokeOperations/JSONPayloadHTTP1OperationDelegate.swift
[2] https://github.com/amzn/smoke-framework/blob/master/Sources/SmokeOperations/OperationDelegate.swift

  • ❤️2
gonzalolarralde commented

I was looking into this same problem, my approach to work around it was assume that if an InputType can be decoded from an empty dictionary it should be a valid transformation, so I modified the check that looks for an empty body for an attempt to instantiate the InputType from “{}”, or otherwise fail.

If you guys think this could be a valid solution I’d be more than happy to provide a PR.

Cheers!

tachyonics commented

For get requests, my feeling is that we want to provide the ability to decode the query string and/or url tokens into the InputType for use in the operation. Decoding from an empty dictionary would solve this problem for an operation that has no input at all but that use case would probably be solved more cleanly by adding operation function signatures that explicitly have no input.

gonzalolarralde commented

That's an interesting point, I wouldn't expect query string processing to be limited to get only, but it'd be good to know if there's an architectural decision taken on whether if query strings would be treated in the same way as body input and how/if it'd be merged with the body input.

I've been working on an implementation for parameterized URIs (eg. /items/{:id}) and I'm preparing an issue to ask if there's a plan to add either Middlewares to support this and a variety of features that can provide additional "inputs" (headers, sessions, query strings and so on) or if there's an architectural path planned for all this points.

tachyonics commented

Thanks Gonzalo,

I need to write up some ideas around how we want to support this. So there is a rough plan, I need to write it down and get some feedback. Having support for parameterized URIs would definitely be part of this so I'd be looking forward to any thoughts you have around this-

But roughly the ideas I have are similar to what we have done for our clients (which we just open sourced so I can now reference) - that is you can specify a custom transform[1] of how the input[2] is merged together.

[1] https://github.com/amzn/smoke-http/blob/master/Sources/SmokeHTTPClient/HTTPClientDelegate.swift
[2] https://github.com/amzn/smoke-http/blob/master/Sources/SmokeHTTPClient/HTTPRequestInputProtocol.swift

tachyonics commented

I have finally got around to coding up a proposal for this-

These are based on an input and output protocol that dictates how parts of the http request and response make up the operation input and output-

  1. https://github.com/amzn/smoke-framework/blob/generic_improvements/Sources/SmokeOperationsHTTP1/OperationHTTP1InputProtocol.swift
  2. https://github.com/amzn/smoke-framework/blob/generic_improvements/Sources/SmokeOperationsHTTP1/Operation1HTTPOutputProtocol.swift

These can be used by something like the JSONPayloadHTTP1OperationDelegate implementation-

Codable types can also still be used by specifying in the handler selector which part of the request and response they come from.

I am also working on an example service package to demonstrate how this can be used for a REST-like service.

Feedback on these changes would definitely be appreciated.

tachyonics commented

I have submitted changes to address this and they have now been merged and the plan is to ship these as part of release 1.0.0.

Resolving this issue. Feel free to reopen if there are any concerns.