fix: proper form data decoding #649

karol-maciaszek · 2019-09-25T13:29:18Z

Checklist

Tests have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce?

Bugfix

What is the current behavior? What is the new behavior?

Currently, body encoded with form-data is improperly decoded. We are using qs package which is not capable of decoding all styles, especially those described in encoding > style.

The new behavior is that we support all encoding styles through our deserializers.

Does this PR introduce a breaking change?

No.

…to feat/allow-reserved

…er-form-data-decoding # Conflicts: # CHANGELOG.md # packages/http/src/validator/validators/body.ts # test-harness/specs/parameters-ac7.oas2.txt # test-harness/specs/parameters-ac8.oas2.txt # test-harness/specs/validate-body-params/form-byte-format-ok.oas2.txt # test-harness/specs/validate-body-params/form-data-invalid-request.oas2.txt # test-harness/specs/validate-body-params/form-data-invalid-request.oas3.txt

XVincentX · 2019-09-30T09:28:38Z

Now that I think about it, would it make sense to extract this logic into a different/external library? I am quite sure this could be used somewhere else in Stoplight @philsturgeon @marbemac

karol-maciaszek · 2019-09-30T11:39:04Z

Now that I think about it, would it make sense to extract this logic into a different/external library? I am quite sure this could be used somewhere else in Stoplight @philsturgeon @marbemac

I think about two library candidates: request-validation, request-deserialize.
Former checks req against spec, latter deals with all those crazy serialization styles.

At least request-deserialization is a unique code. At the time of writing (1yr ago), there was no similar tool.

lag-of-death · 2019-09-30T13:56:28Z

packages/http/src/validator/validators/body.ts

+  target: any,
+): Either.Either<IPrismDiagnostic[], any> {
+  if (
+    Option.getEq(fromEquals((a: string, b: string) => typeIs.is(a, b) as boolean)).equals(


Suggested change

Option.getEq(fromEquals((a: string, b: string) => typeIs.is(a, b) as boolean)).equals(

Option.getEq(fromEquals((a: string, b: string) => !!typeIs.is(a, b))).equals(

Yes, please let's void casts.

philsturgeon · 2019-09-30T16:48:06Z

Now that I think about it, would it make sense to extract this logic into a different/external library? I am quite sure this could be used somewhere else in Stoplight @philsturgeon @marbemac

I think we could hold off for a while on doing this. We have talked a bit recently about how the validation logic in general needs to be refactored, about how middlewares could be useful, and about how it could become its own Lerna package.

I think all of these things could be done together in the future while we refactor things, and maybe it could become its own package outside of the repo. Either way we can kick that can down the road whilst we sort out Prism more IMO.

XVincentX

@StefanDywersant

Ok I took a very deep view at this PR to understand the Either situation and this validator is actually really difficult to manage properly.

As you probably noticed, this is not just validating the target, but it is also mutating the target — and then at the same time it's not really doing anything useful with it. And so while we should be using Either, for our particular use case (and make it compatible with the rest of the code base) we need to convert back to an Option

In general, reasoning about this function is hard and that led you to make some mistakes. I've pointed out some of them but iterating on this would have been just infinite and so I took some time and refactored (and prepared a PR) -> https://github.com/stoplightio/prism/pull/667/files

If you're ok with it, take some time to take a look at it and ask all the questions you want. If you won't be able to understand anything it's just fine; keep in mind that my code is still horrible and it's just trying to put some minimal order into something that needs to be refactored.

With the ball in the future, I would say the validator was nor worth converting, for the current state of the code.

packages/http/src/validator/validators/body.ts

XVincentX · 2019-10-01T08:35:52Z

packages/http/src/validator/validators/body.ts

+  target: any,
+): Either.Either<IPrismDiagnostic[], any> {
+  if (
+    Option.getEq(fromEquals((a: string, b: string) => typeIs.is(a, b) as boolean)).equals(


Yes, please let's void casts.

XVincentX · 2019-10-01T08:36:35Z

packages/http/src/validator/validators/body.ts

+      const property = encoding.property;
+      const value = encodedUriParams[property];
+
+      if (!allowReserved && typeof value === 'string' && value.match(/[\/?#\[\]@!$&'()*+,;=]/)) {


I would say we're trying to employ fp concepts where it makes sense; it does not have to be imperative and more importantly it's not a purity race. If it makes sense and it helps us to have a more predictable code — let's go for it.

XVincentX · 2019-10-01T14:31:01Z

packages/http/src/validator/validators/__tests__/body.spec.ts

+          ),
+        ).toEqual([]);
+      });
+    });


Maybe we could

add a failing test

Break down this super test into a set of sub tests? (nested, array…)

added failing test

I simplified the test to only ensure that deserializer works properly

XVincentX · 2019-10-01T14:32:02Z

packages/http/src/validator/validators/body.ts

+    return pipe(
+      Either.fromNullable([])(schema),
+      Either.map(() => target),
+      Either.chain(partial(maybeValidateFormBody, schema!, content, Option.fromNullable(mediaType))),


The reason why we can't really use partial here is that we need to use the schema that's returned from the Either.fromNullable computation, not the one coming from the function parameters (which can be null and you're effectively using the ! to make it appear ok)

XVincentX · 2019-10-01T14:33:12Z

packages/http/src/validator/validators/body.ts

-        }
+    return pipe(
+      Either.fromNullable([])(schema),
+      Either.map(() => target),


target is already available from the function parameter and does not need any change or "fp-isation", so we can just use that directly.

XVincentX · 2019-10-01T14:33:49Z

packages/http/src/validator/validators/body.ts

+    return pipe(
+      schema ? Either.right(target) : Either.left([]),
+      Either.chain(partial(maybeValidateFormBody, schema!, content, mediaType)),
+      Either.chain(partial(validateBody, schema!)),


As I mentioned in the previous comment, the problem is that we need to use the schema parameter coming from the chain, not from the function's parameters, where the ! is required.

* refactor: stuff * refactor: separate in functions * refactor: correct things * file: restore

XVincentX · 2019-10-01T16:10:27Z

packages/http/src/validator/validators/body.ts

-    Option.fromPredicate(mt => !!!typeIs.is(mt, ['application/x-www-form-urlencoded'])),
-    Option.chain(() => Option.some(validateBody(schema, target))),
+    Option.fromPredicate(mt => !typeIs.is(mt, ['application/x-www-form-urlencoded'])),
+    Option.map(() => validateBody(schema, target)),


Oh very nice catch, I didn't realise it!

XVincentX · 2019-10-01T16:14:03Z

packages/http/src/validator/validators/body.ts

+}
+
+function validateBody(schema: JSONSchema, target: any): IPrismDiagnostic[] {
+  return validateAgainstSchema(target, schema);


We can probably remove this redundant thing.

XVincentX

Thanks a lot for this, I hope it was not that painful to make it happen. I'm merging when tests will pass.

karol-maciaszek · 2019-10-01T16:52:50Z

Thanks a lot for this, I hope it was not that painful to make it happen. I'm merging when tests will pass.

I learned lots while working on this + thank you for guiding while doing so!

karol-maciaszek added 14 commits September 20, 2019 16:13

feat: encoding / allowReserved support

cae6fb5

fix: fixed placement of query-string parsing

a576234

Merge branch 'master' into feat/allow-reserved

fbcd1cb

fix: added shallow body parsing

df5a5aa

Merge branch 'feat/allow-reserved' of github.com:stoplightio/prism in…

4083c65

…to feat/allow-reserved

refactor: prettyfied code

ef42970

Merge branch 'master' into feat/allow-reserved

41a6674

fix: dependencies fixes

d7132e7

Merge branch 'master' into feat/allow-reserved

1cd49c0

fix: added missing dependency

91876a8

Merge branch 'feat/allow-reserved' of github.com:stoplightio/prism in…

2c8f320

…to feat/allow-reserved

docs: added changelog line

bf43e34

Merge branch 'master' into feat/allow-reserved

f0e71ae

fix: proper form-data decoding

8e868ab

karol-maciaszek added t/bug WIP Work In Progress labels Sep 25, 2019

karol-maciaszek self-assigned this Sep 25, 2019

karol-maciaszek added 7 commits September 26, 2019 13:04

chore: test, fpism

1c61016

refactor: more FPizms

e15a7fc

chore: increased readability

a32eee8

chore: removed e2e test

f8885db

chore: FP

3828bb7

chore: removed unneeded package

c887f7d

karol-maciaszek changed the title ~~[WIP] fix: proper form data decoding~~ fix: proper form data decoding Sep 27, 2019

karol-maciaszek removed the WIP Work In Progress label Sep 27, 2019

Merge branch 'master' into fix/proper-form-data-decoding

5615b8b

karol-maciaszek requested review from XVincentX, lag-of-death and philsturgeon September 27, 2019 14:34

fix: fixed import

27ea5e1

karol-maciaszek added 6 commits September 30, 2019 13:53

fix: applied cr suggestions

37db80e

fix: missing option creation

20d0aaa

fix: equals, linting

898d47f

fix: linting, binding

25a6368

fix: return type annotations for monads

f06aedb

fix: func name change

88ef1ae

karol-maciaszek requested review from lag-of-death and XVincentX September 30, 2019 13:26

lag-of-death reviewed Sep 30, 2019

View reviewed changes

lag-of-death approved these changes Sep 30, 2019

View reviewed changes

XVincentX added 3 commits October 1, 2019 09:45

Merge branch 'master' into fix/proper-form-data-decoding

29fc4da

Merge branch 'master' into fix/proper-form-data-decoding

e4f4b2a

Merge branch 'master' into fix/proper-form-data-decoding

a9636e9

XVincentX suggested changes Oct 1, 2019

View reviewed changes

XVincentX and others added 4 commits October 1, 2019 16:36

Merge branch 'master' into fix/proper-form-data-decoding

426ab1d

Form data encoding improvements (#667)

76415ec

* refactor: stuff * refactor: separate in functions * refactor: correct things * file: restore

fix: minor fixes

6a70136

test: simplified test

fe0abbe

XVincentX reviewed Oct 1, 2019

View reviewed changes

karol-maciaszek added 2 commits October 1, 2019 18:20

test: added failing test

5ca0237

fix: final touches

107573f

XVincentX approved these changes Oct 1, 2019

View reviewed changes

XVincentX merged commit 9b0763e into master Oct 1, 2019

XVincentX deleted the fix/proper-form-data-decoding branch October 1, 2019 16:52

fix: proper form data decoding #649

fix: proper form data decoding #649

karol-maciaszek commented Sep 25, 2019 •

edited

XVincentX commented Sep 30, 2019

karol-maciaszek commented Sep 30, 2019 •

edited

lag-of-death Sep 30, 2019

XVincentX Oct 1, 2019

philsturgeon commented Sep 30, 2019

XVincentX left a comment

XVincentX Oct 1, 2019

XVincentX Oct 1, 2019

XVincentX Oct 1, 2019

karol-maciaszek Oct 1, 2019

XVincentX Oct 1, 2019

XVincentX Oct 1, 2019

XVincentX Oct 1, 2019

XVincentX Oct 1, 2019

XVincentX Oct 1, 2019

XVincentX left a comment

karol-maciaszek commented Oct 1, 2019

	Option.getEq(fromEquals((a: string, b: string) => typeIs.is(a, b) as boolean)).equals(
	Option.getEq(fromEquals((a: string, b: string) => !!typeIs.is(a, b))).equals(

fix: proper form data decoding #649

fix: proper form data decoding #649

Conversation

karol-maciaszek commented Sep 25, 2019 • edited

Checklist

What kind of change does this PR introduce?

What is the current behavior? What is the new behavior?

Does this PR introduce a breaking change?

XVincentX commented Sep 30, 2019

karol-maciaszek commented Sep 30, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philsturgeon commented Sep 30, 2019

XVincentX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XVincentX left a comment

Choose a reason for hiding this comment

karol-maciaszek commented Oct 1, 2019

karol-maciaszek commented Sep 25, 2019 •

edited

karol-maciaszek commented Sep 30, 2019 •

edited