-
Notifications
You must be signed in to change notification settings - Fork 16
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
Panic when attempting to update a Flex instance configuration due to an incorrect media type in the request #49
Comments
Thank you for the in depth issue and details! A corresponding issue in twilio-oai would be appreciated to outline the missing request body schema. We are aware our library doesn't currently support JSON ingress but it's definitely on the horizon! We'd greatly appreciate any PR you're willing to submit (even as a proof-of-concept) to support our effort. If you're submitting PRs to multiple repos (i.e. one for updating mustache templates here, plus any corresponding changes in twilio-go) please be sure they include links to each other in the description. This issue has been added to our internal backlog to be prioritized. Pull requests and +1s on the issue summary will help it move up the backlog. |
Thanks @eshanholtz, I'll take a look at putting together a few PRs. I'll get the issue raised on the twilio-oai repo too |
…JSON media type for the request body The Go Generator has been updated to add a vendor extension to set `x-is-json` to true when the consume media type is `application/json` and `x-is-form` when the consumes media type is `application/x-www-form-urlencoded` The `x-is-json` vendor extension is used to generate the code to handle calling the new PostJson function or the corresponding HTTP method function on the request handler. This is designed to only work with Post requests at the moment but could be extended in the future if needed. This PR aids in resolving twilio#49 however once this PR is merged twilio/twilio-oai#36 will need to be finished to fully resolve the issue This change relies on twilio/twilio-go#83 and the tests cannot be updated until this PR is merged and released This change also fixes an issue with JSON struct tags for the Params structs being `html-escaped`. I have disabled the escaping by using `{{{}}}` as this was highlighted during linting the Go SDK repo
This change supports the multiple PR's to fix twilio/twilio-oai-generator#49 where the SDK panics when trying to update a Flex Configuration.
Hi @eshanholtz, Apologies for the delay in raising these PRs. I have raised 2 PR's (twilio/twilio-go#83 and twilio/twilio-go#84) against the Go SDK repo and 1 PR against this repo (#54) to lay the groundwork for resolving this issue. Unfortunately, until twilio/twilio-oai#36 is resolved, Flex Configuration cannot be updated via the Go SDK. I have tested this locally by replacing the operations for the
With this updated spec, the following code was generated for the Go SDK
I have tested the modified
Any feedback is greatly appreciated |
…JSON media type for the request body The Go Generator has been updated to add a vendor extension to set `x-is-json` to true when the consume media type is `application/json` and `x-is-form` when the consumes media type is `application/x-www-form-urlencoded` The `x-is-json` vendor extension is used to generate the code to handle calling the new PostJson function or the corresponding HTTP method function on the request handler. This is designed to only work with Post requests at the moment but could be extended in the future if needed. This PR aids in resolving twilio#49 however once this PR is merged twilio/twilio-oai#36 will need to be finished to fully resolve the issue This change relies on twilio/twilio-go#83 and the tests cannot be updated until this PR is merged and released This change also fixes an issue with JSON struct tags for the Params structs being `html-escaped`. I have disabled the escaping by using `{{{}}}` as this was highlighted during linting the Go SDK repo
…JSON media type for the request body The Go Generator has been updated to add a vendor extension to set `x-is-json` to true when the consume media type is `application/json` and `x-is-form` when the consumes media type is `application/x-www-form-urlencoded` The `x-is-json` vendor extension is used to generate the code to handle calling the new PostJson function or the corresponding HTTP method function on the request handler. This is designed to only work with Post requests at the moment but could be extended in the future if needed. This PR aids in resolving twilio#49 however once this PR is merged twilio/twilio-oai#36 will need to be finished to fully resolve the issue This change relies on twilio/twilio-go#83 and the tests cannot be updated until this PR is merged and released This change also fixes an issue with JSON struct tags for the Params structs being `html-escaped`. I have disabled the escaping by using `{{{}}}` as this was highlighted during linting the Go SDK repo
This change supports the multiple PR's to fix twilio/twilio-oai-generator#49 where the SDK panics when trying to update a Flex Configuration.
Issue Summary
Hi, I have noticed that you cannot currently update a Twilio Flex instance configuration using the Twilio Go SDK.
Currently on every POST request the
Content-Type
header is hardcoded to beapplication/x-www-form-urlencoded
as seen hereThis is suitable for the majority of POST requests to the various Twilio API however to update the Flex Configuration, JSON needs to be sent in the request body.
To fix this I believe it will require changes in the Go SDK with the request handler and client being modified. Changes will also be required inside of the API mustache template in this repo.
I was thinking that inside the request handler in the Go SDK, the current Post function would be updated to create a new map that combines the
Content-Type
header (with the value ofapplication/x-www-form-urlencoded
) with the header map which is passed into the function. I was going with this approach to prevent mutating the headers map but if you want the header can either be passed in as another argument to SendRequest or the value could be appended to the headers map.The current logic to set the
Content-Type
header inside of send request would be deleted. Another functionPostJson
would be added which would accept the path, body (of typemap[string]interface{}
) and the headers. This function would set the value of theContent-Type
header toapplication/json
.In the generator, the API mustache template file will be updated to check if the request media type is
application/json
then the initialization of the data map (url.Values) and population of the params data into the map will be replaced with the params struct being marshalled into a map of typemap[string]interface{}
using the build-inMarshal
function in thejson
package. Instead of the Post function being called on the request handler the PostJson function will be called.If the request media type is not supplied or is
application/x-www-form-urlencoded
then the current implementation will be used, as per the current implementation.The JSON and YML Open API templates don't currently include the request body schema which would be used to generate the input/ params struct, so the changes outlined above will resolve the issue highlighted however the Flex configuration cannot be updated using the SDK until the definition has been updated.
As this is an issue with the Open API templates do you want the issue for this raised on the twilio-oai repo?
This implementation should be flexible enough to be replicated to add support for creating a Serverless Asset Version and creating a Serverless Function Version via the SDK, both of which require form data to be supplied.
If you are happy with the changes I am happy to raise a PR for this.
Steps to Reproduce
Code Snippet
Exception/Log
Technical details:
The text was updated successfully, but these errors were encountered: