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

Add final boundary for multipart entities with no parts #1257 #1260

Merged
merged 1 commit into from
Jul 26, 2017

Conversation

arnohaase
Copy link

@arnohaase arnohaase commented Jun 30, 2017

Fixes #1257

@akka-ci
Copy link

akka-ci commented Jun 30, 2017

Can one of the repo owners verify this patch?

@ktoso
Copy link
Member

ktoso commented Jun 30, 2017

OK TO TEST

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed validating PR that is currently being validated by Jenkins labels Jun 30, 2017
@akka-ci
Copy link

akka-ci commented Jun 30, 2017

Test PASSed.

@arnohaase
Copy link
Author

I signed the Lightbend CLA,

@jrudolph
Copy link
Member

jrudolph commented Jul 5, 2017

Hi, @arnohaase, thanks for the contribution.

The change seems reasonable, though it is unclear to me if multipart entities without any part would (or should) be allowed at all? Do have any reference for that?

@arnohaase
Copy link
Author

arnohaase commented Jul 5, 2017

The spec does not specifically forbid it, at least as far as I can see. But it does specify that the final boundary must always be present. https://www.w3.org/Protocols/rfc1341/7_2_Multipart.html

The problem appeared in a real-world problem where a multipart response was used to represent documents matching a search, with "no parts" being a valid response document.

Btw, MultipartUnmarshallers.defaultMultipartGeneralUnmarshaller can handle Multipart entities without entries already, provided the final boundary is present.

Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

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

Looks good, especially since the change is only in strict and it looks like it already behaved that way for streamed.

It'd be nice to have an easy way to execute the test both in 'strict' and 'streamed' mode, do we have a way to do that?

@jrudolph
Copy link
Member

@arnohaase

The spec does not specifically forbid it, at least as far as I can see.

It says:

In the case of multiple part messages, in which one or more different sets of data are combined in a single body

I understand it that "one or more" wouldn't include zero. But I guess that's only a nitpick.

I agree we should do it consistently for marshallers and unmarshallers and also for strict and streamed. Would be nice to have tests being run for strict and streamed variants. Is that something you would like to work on, @arnohaase or @raboof?

@raboof
Copy link
Member

raboof commented Jul 26, 2017

@jrudolph I think that'd be interesting but am not planning to do it right now - perhaps we should merge this and keep that idea for later? Created #1309 to track it.

@jrudolph
Copy link
Member

Yep, sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tested PR that was successfully built and tested by Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants