search

cognitect-labs / vase

Data driven microservices
Eclipse Public License 1.0
373 stars 42 forks source link

Transact action should support common encoding formats- JSON, Transit, edn, and Form-POST #62

Open ohpauleez opened 7 years ago

ohpauleez commented 7 years ago

This addresses issue #45

Resolving the "payload" parameter for transact is handled by a separate function. Currently, this function also grabs form params as a last effort (which are not nested under payload).

ddeaguiar commented 7 years ago

Unrelated to this PR but I noticed that the merged-parameters fn doesn't include transit-params. Should it?

ohpauleez commented 7 years ago

It does now :) -- nice catch.

ddeaguiar commented 7 years ago

I think we'll want to add form-params to merged-parameters as well. Another thing is that the caller is expecting a collection of maps but :form-params will just be a map. AFAIK, you can't submit a collection.

ddeaguiar commented 7 years ago

@ohpauleez I spent some time thinking about this PR and thought that extracting resolve-payload-parameter to an an interceptor or interceptors may provide more flexibility. I'd consider at least two interceptors here. One for json, transit and edn params and another for form params because they require some transformation, at least based on the current usage in transact-action-exprs. I'd have transact-action-exprs depend on a new params key, like :vase-params, whose contents should be a collection of txdata, which would be populated by these interceptors.

In the future this could even be made configurable so that developers could specify which content-types are supported – a consequence of this is that such information could be feed into specification generation tooling for things like Swagger.

I'd be happy to adopt this PR and hash this out if you're up for it.

ddeaguiar commented 7 years ago

Alternatively, resolve-payload-parameter can stick around and support json, transit and end payloads but :form-param support could be moved to an interceptor. I think a new params key would still be needed in this case, though.

ddeaguiar commented 7 years ago

Ping @ohpauleez just following up. This PR is related to issue #78 as well.

CalebMacdonaldBlack commented 6 years ago

I'd really see this PR make it in to send edn instead of json

ddeaguiar commented 5 years ago

@ohpauleez I'm guessing this work is sitting on the back burner. Would it be ok if I pushed it forward?