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

toStrict the entity when parsing multiple form fields #2283

Merged
merged 3 commits into from Dec 11, 2018

Conversation

Projects
None yet
3 participants
@raboof
Copy link
Member

raboof commented Nov 20, 2018

Fixes the 'simple case' ('A') of #73

To avoid consuming the entity for each field. Of course slurping in the
entire entity is not very elegant, but this is what both the form-data and
the multipart marshallers already do further down the line.

@raboof raboof requested a review from jrudolph Nov 20, 2018

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Nov 20, 2018

Test FAILed.

raboof added some commits Nov 20, 2018

toStrict the entity when parsing multiple form fields
To avoid consuming the entity for each field. Of course slurping in the entire
entity is not very elegant, but this is what both the form-data and the
multipart marshallers already do further down the line.

@raboof raboof force-pushed the reproduceFormFieldsError branch from 2f2c254 to c127aec Nov 20, 2018

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Nov 20, 2018

Test FAILed.

@jrudolph
Copy link
Member

jrudolph left a comment

A question below

@@ -115,7 +118,7 @@ object StrictForm {
def tryUnmarshalToMultipartForm: Future[StrictForm] =
for {
multiPartFD multipartUM(entity).fast
strictMultiPartFD multiPartFD.toStrict(10.seconds).fast // TODO: make timeout configurable
strictMultiPartFD multiPartFD.toStrict(toStrictTimeout).fast

This comment has been minimized.

@jrudolph

jrudolph Nov 20, 2018

Member

Good 👍

Show resolved Hide resolved akka-http/src/main/scala/akka/http/scaladsl/common/StrictForm.scala
Show resolved Hide resolved ...ain/scala/akka/http/scaladsl/server/directives/FormFieldDirectives.scala
@jrudolph

This comment has been minimized.

Copy link
Member

jrudolph commented Nov 20, 2018

Great that you are tackling this issue, @raboof. It's a long-standing nuisance.

To avoid consuming the entity for each field. Of course slurping in the
entire entity is not very elegant

There isn't any way around it for this style of API which assumes random access to form fields.

For streaming formField access we would need a completely different API.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Dec 10, 2018

Test FAILed.

@raboof raboof force-pushed the reproduceFormFieldsError branch from 0798338 to f453180 Dec 10, 2018

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Dec 10, 2018

Test PASSed.

@raboof

This comment has been minimized.

Copy link
Member

raboof commented Dec 10, 2018

This PR solves the A case. By applying toStrict in more places we could probably also fix the B case at the expense of doing too many toStrict calls in some cases. It's not entirely obvious to me how to fit this in with the magnets and recursive types, though.

I'd propose including this PR first and leaving the B case for later, given that this probably already helps for common cases.

@jrudolph

This comment has been minimized.

Copy link
Member

jrudolph commented Dec 11, 2018

doing too many toStrict calls in some cases.

Do you mean, because currently each single form field already tries to read the entity fully, we would toStrict multiple times even for the formField(singleField`) case? Or which case do you mean?

@raboof

This comment has been minimized.

Copy link
Member

raboof commented Dec 11, 2018

doing too many toStrict calls in some cases.

Do you mean, because currently each single form field already tries to read the entity fully, we would toStrict multiple times even for the formField(singleField`) case? Or which case do you mean?

I meant if we'd insert a toStrict in front of any formField/formFields directive, it would also toStrict the entity even when there is no other nested formField/formFields directive in the hierarchy. That might not be a big deal, but it was also not obvious how to do it 😄.

@jrudolph
Copy link
Member

jrudolph left a comment

LGTM, still needs more fixing but that can be done later indeed.

@jrudolph jrudolph merged commit 9ae3ded into master Dec 11, 2018

4 checks passed

Jenkins PR Validation Test PASSed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@jrudolph jrudolph deleted the reproduceFormFieldsError branch Dec 11, 2018

@jrudolph jrudolph added this to the 10.1.6 milestone Dec 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment