Skip to content

T 072733: Update multipart form request serializer#165

Merged
bconway99 merged 8 commits intomainfrom
task/1072733-update-multipartFormRequestSerializer
Mar 22, 2022
Merged

T 072733: Update multipart form request serializer#165
bconway99 merged 8 commits intomainfrom
task/1072733-update-multipartFormRequestSerializer

Conversation

@bconway99
Copy link
Copy Markdown
Contributor

@bconway99 bconway99 commented Mar 18, 2022 •
edited
Loading

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.

plflanagan
plflanagan reviewed Mar 18, 2022
View reviewed changes
Copy link
Copy Markdown
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.

asolovev
asolovev approved these changes Mar 22, 2022
View reviewed changes
Copy link
Copy Markdown

@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

Reviewers

@asolovev asolovev asolovev approved these changes

@christopherellwood christopherellwood Awaiting requested review from christopherellwood

@mattweight mattweight Awaiting requested review from mattweight

+2 more reviewers

@plflanagan plflanagan plflanagan left review comments

@anthony-lipscomb-dev anthony-lipscomb-dev anthony-lipscomb-dev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@bconway99 bconway99

Labels

good first issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bconway99 @anthony-lipscomb-dev @asolovev @plflanagan