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

Fix Content-Type custom payload bugs #888

Merged
merged 2 commits into from
Apr 16, 2024
Merged

Fix Content-Type custom payload bugs #888

merged 2 commits into from
Apr 16, 2024

Conversation

marina-p
Copy link
Contributor

When a custom payload body is used to work around unsupported content types, the Content-Type header must also be modified via the dictionary. This functionality had several bugs, which are fixed in this change. After this update, using either restler_custom_payload_header or restler_custom_payload with the request-specific syntax for dictionary payloads now works as expected, for example:

"restler_custom_payload": {
"/stores/{storeId}/order/post/Content-Type": [ "xml" ]
}

This change also fixes the same bugs with per-request syntax with restler_custom_payload_query.

Testing:

  • manual testing with demo_server
  • added new compiler tests

When a custom payload body is used to work around unsupported content types, the Content-Type
header must also be modified via the dictionary.  This functionality had several bugs, which
are fixed in this change.  After this update, using either `restler_custom_payload_header` or
`restler_custom_payload` with the request-specific syntax for dictionary payloads now works
as expected, for example:

  "restler_custom_payload": {
    "/stores/{storeId}/order/post/Content-Type": [ "xml" ]
  }

This change also fixes the same bugs with per-request syntax with restler_custom_payload_query.

Testing:
- manual testing with demo_server
- added new compiler tests
@marina-p marina-p requested a review from wilbaker April 12, 2024 21:13
sprintf "%s/%s/%s" endpoint (method.ToLower()) propertyNameOrPath

member x.isRequestTypePayloadName endpoint (method:string) (propertyValue:string) =
let prefix = sprintf "%s/%s" endpoint (method.ToLower())
Copy link
Member

@wilbaker wilbaker Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should prefix include a trailing / like it does here in getRequestTypePayloadPrefix?

        member x.getRequestTypePayloadPrefix endpoint (method:string) = 
            sprintf "%s/%s/" endpoint (method.ToLower())

Wondering if there could be an issue here with this code and propertyValue.StartsWith(prefix) incorrectly matching with a prefix that's actually a substring of propertyValue:

prefix = "foo/bar"
propertyValue = "foo/bar2" #Resolved

// No headers in Swagger
let injectedCustomPayloadHeaderParameters = getInjectedHeaderOrQueryParameters [] CustomPayloadType.Header
if injectedCustomPayloadHeaderParameters |> Seq.isEmpty then
// TODO: remove this branch and update grammar.py test baselines
Copy link
Member

@wilbaker wilbaker Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanted to check if this TODO is for this PR, or if it's to be addressed in future set of changes. #Pending

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's for a future set of changes, updating the grammar.json files is a bit time consuming - in addition to the compiler updates, need to make sure the engine can consume the new grammar.json files and this particular change could introduce a regression.

Copy link
Member

@wilbaker wilbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanted to make sure the prefix check in Dictionary.fs is correct

Copy link
Member

@wilbaker wilbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me. FYI that I have not tested/validated these changes myself. Thanks!

@marina-p marina-p merged commit c75e59e into main Apr 16, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants