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

T 072733: Update multipart form request serializer #165

Merged
merged 8 commits into from
Mar 22, 2022

Conversation

bconway99
Copy link
Contributor

@bconway99 bconway99 commented Mar 18, 2022

This pull request includes (pick all that apply):

  • Bugfixes
  • New features
  • Breaking changes (Potentially)
  • Documentation updates
  • Unit tests
  • Other

Summary

  • With the existing implementation we were having difficulties sending HTTP multipart requests to the RestAPI.
  • The issue appeared to be the boundary value that is sent alongside these requests.
  • The RestAPI endpoint in question (/Documents/LiabilityRelease?consumerId=[ID]) wasn't accepting the existing boundary value.
  • In a multipart request, a boundary acts like a marker for each key/value pair in the request's form data.

Implementation

  • This updates the serializer to remove additional boundary content.
  • This was done to ensure the endpoint responds successfully.
  • In summary this would only affect HTTP multipart requests that leverage MultipartFormRequestSerializer.

Test Plan

  • I've marked this as including breaking changes.
  • While this is a minor change I don't have visibility of all services/endpoints which leverage MultipartFormRequestSerializer.
  • This could potentially break access to certain endpoints if they are expecting the original boundary value.
  • If CI or any endpoints break for anyone please let me know.

Copy link
Contributor

@plflanagan plflanagan left a comment

Choose a reason for hiding this comment

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

I've left one comment, but I will also leave final approval to others who have more experience with conduit and understand the implications better of changing it.

Copy link

@asolovev asolovev left a comment

Choose a reason for hiding this comment

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

Overall seems fine. Added a suggestion about using "Random Unification" methods.
There is also a note in RFC2046

   WARNING TO IMPLEMENTORS:  The grammar for parameters on the Content-
   type field is such that it is often necessary to enclose the boundary
   parameter values in quotes on the Content-type line.  This is not
   always necessary, but never hurts.

It doesn't seem necessary in our case, but if there will be additional problems, we can try it as well or use a longer boundary.

@bconway99 bconway99 merged commit b28cd97 into main Mar 22, 2022
@bconway99 bconway99 deleted the task/1072733-update-multipartFormRequestSerializer branch March 22, 2022 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants