Skip to content

Add support for preamble and epilogue in multipart entities#427

Merged
ok2c merged 1 commit intoapache:5.3.xfrom
arturobernalg:feature/support_preamble_and_epilog
Mar 23, 2023
Merged

Add support for preamble and epilogue in multipart entities#427
ok2c merged 1 commit intoapache:5.3.xfrom
arturobernalg:feature/support_preamble_and_epilog

Conversation

@arturobernalg
Copy link
Copy Markdown
Member

This pull request adds support for preamble and epilogue in multipart entities for the Apache HttpComponents project. The AbstractMultipartFormat class has been updated to include preamble and epilogue as additional parameters in the constructor. The HttpRFC6532Multipart, HttpRFC7578Multipart, and HttpStrictMultipart classes have also been updated to support these parameters in their constructors.

Please let me know if there are any concerns or suggestions. Thank you!

@arturobernalg arturobernalg force-pushed the feature/support_preamble_and_epilog branch 2 times, most recently from 6c8e153 to d487b1e Compare March 18, 2023 06:36
switch (modeCopy) {
case LEGACY:
form = new LegacyMultipart(charsetCopy, boundaryCopy, multipartPartsCopy);
form = new LegacyMultipart(charsetCopy, boundaryCopy, multipartPartsCopy, preamble, epilogue);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@arturobernalg Looks good to me. I would not add preamble and epilogue in the LEGACY mode, though.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Changed.

@michael-o
Copy link
Copy Markdown
Member

Is this one addressing https://issues.apache.org/jira/browse/HTTPCLIENT-2064?

Previously, multipart entities did not support adding a preamble or epilogue to the message. This commit adds support for these features by modifying the AbstractMultipartFormat class to accept preamble and epilogue strings in its constructor. The HttpRFC6532Multipart, HttpRFC7578Multipart, and HttpStrictMultipart classes are updated to pass these parameters to the parent constructor when creating instances of multipart entities.

This change allows users to include custom content at the beginning and end of their multipart messages, which can be useful in certain scenarios such as adding metadata or information about the message contents.
@arturobernalg arturobernalg force-pushed the feature/support_preamble_and_epilog branch from f44f11e to b06b7a9 Compare March 19, 2023 12:59
@arturobernalg arturobernalg requested a review from ok2c March 19, 2023 13:00
Copy link
Copy Markdown
Member

@ok2c ok2c left a comment

Choose a reason for hiding this comment

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

@michael-o Does the change-set meet your expectations?

@michael-o
Copy link
Copy Markdown
Member

@michael-o Does the change-set meet your expectations?

Yes, it does. But no testing valid input according to RFC. But I guess that is: shit in, shit out.

@arturobernalg
Copy link
Copy Markdown
Member Author

arturobernalg commented Mar 23, 2023

@michael-o Does the change-set meet your expectations?

Yes, it does. But no testing valid input according to RFC. But I guess that is: shit in, shit out.

Hey @michael-o
Do i have to do any change/improvement? maybe some extra test?
TY

@arturobernalg arturobernalg requested a review from ok2c March 23, 2023 13:12
@michael-o
Copy link
Copy Markdown
Member

@michael-o Does the change-set meet your expectations?

Yes, it does. But no testing valid input according to RFC. But I guess that is: shit in, shit out.

Hey @michael-o Do i have to do any change/improvement? maybe some extra test? TY

No, this change is fine. Everything else I have mentioned here is beyond this PR.

@ok2c ok2c merged commit 84540c4 into apache:5.3.x Mar 23, 2023
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.

3 participants