allow empty responses #606

lag-of-death · 2019-09-13T15:27:10Z

Fixes #584
Fixes #592

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?

Bug fix.

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

No empty responses were allowed.

karol-maciaszek

Discussion needed on support for q-factor in Accept header and empty content in invalid requests.

packages/http/src/mocker/negotiator/NegotiatorHelpers.ts

…to fix/empty-responses_

* chore: upgrade jsf * test: add e2e * chore: switch back to npm version * docs: changelog * test: dry

XVincentX

Dropped some comments. The code flow is still not clear but hopefully it'll be better after addressing these comments.

packages/http/src/mocker/HttpMocker.ts

packages/http/src/mocker/negotiator/NegotiatorHelpers.ts

…responses_ # Conflicts: # CHANGELOG.md

lag-of-death · 2019-09-18T14:33:53Z

@XVincentX, I believe the feedback is addressed now, ready for another round

XVincentX · 2019-09-18T17:23:36Z

@lag-of-death Can you rebase when you get a chance?

packages/http/src/mocker/negotiator/__tests__/NegotiatorHelpers.spec.ts

XVincentX

Dropped some other comments but in general I think we're getting close to the end of this 👍

packages/http-server/src/__tests__/server.oas.spec.ts

packages/http/src/mocker/negotiator/InternalHelpers.ts

packages/http/src/mocker/negotiator/NegotiatorHelpers.ts

packages/http/src/mocker/negotiator/__tests__/NegotiatorHelpers.spec.ts

XVincentX · 2019-09-20T13:23:45Z

test-harness/specs/E2E-Accept-Content-Negotiation-AC-7.oas2.txt

@@ -7,6 +7,8 @@ swagger: "2.0"
 paths:
  /todos:
    get:
+      produces:
+        - text/plain


XVincentX

Dropped another bunch of comments. We're getting there but I am already anticipating this is going to require yet another round of review.

packages/http/src/mocker/negotiator/InternalHelpers.ts

packages/http/src/mocker/negotiator/NegotiatorHelpers.ts

XVincentX · 2019-09-23T06:59:49Z

packages/http/src/mocker/negotiator/NegotiatorHelpers.ts

+                Either.fromOption(() => {
+                  logger.warn(warnMsg(mediaTypes));
+
+                  return ProblemJsonError.fromTemplate(


You're right; this does not feel right but I might be the dude introducing this so I'll look into. Why the cast though? as Error

packages/http/src/mocker/negotiator/NegotiatorHelpers.ts

XVincentX · 2019-09-23T07:45:23Z

packages/http/src/mocker/negotiator/NegotiatorHelpers.ts

-              return Either.left<Error, IHttpNegotiationResult>(
-                ProblemJsonError.fromTemplate(NOT_ACCEPTABLE, `Unable to find content for ${mediaTypes}`),
+              return pipe(
+                optionallyGetPayloadlessResponse(response, headers || [], mediaTypes),


findEmptyResponse

XVincentX · 2019-09-23T08:05:10Z

packages/http/src/mocker/negotiator/NegotiatorHelpers.ts

+        const noAcceptAndNoContentResponses =
+          !noContentResponses.length && !mediaTypes.filter((mt: string) => !mt.includes('*/*')).length;
+
+        return noAcceptAndNoContentResponses || ['400', '422'].includes(response.code);


Why are you checking for 400 and 422 responses manually? Don't they follow the same flow?

XVincentX · 2019-09-23T08:06:43Z

packages/http/src/mocker/negotiator/NegotiatorHelpers.ts

+    Option.chain(responses => {
+      return Option.fromPredicate((noContentResponses: { length: number }) => {
+        const noAcceptAndNoContentResponses =
+          !noContentResponses.length && !mediaTypes.filter((mt: string) => !mt.includes('*/*')).length;


I really think we should put a comment on this condition, I still struggle to understand it correctly.

Also, with includes Accept: application/json,*/* will match. Is this intentional? I guess so (if the client accepts / then an empty response is viable, right?)

packages/http/src/mocker/negotiator/__tests__/NegotiatorHelpers.spec.ts

Co-Authored-By: Vincenzo Chianese <vincenz.chianese@icloud.com>

packages/http/src/mocker/negotiator/NegotiatorHelpers.ts

lag-of-death · 2019-09-23T11:49:21Z

hi @XVincentX, I think I've addressed the feedback. The logic for findEmptyResponse is now greatly simplified.
The previous version was kind of redundant because some of the conditional checks were already performed up the chain - somewhere in findEmptyResponse's caller or higher.
There are also a couple of more tests so that hopefully no edge case is missed.

XVincentX

I think we're finally there. Just one final change (pipe for fromPredicate); I'm checking out the tests but quite sure we'll merge this today.

XVincentX · 2019-09-23T12:42:09Z

packages/http/src/mocker/negotiator/NegotiatorHelpers.ts

+  mediaTypes: string[],
+): Option.Option<IHttpNegotiationResult> => {
+  return pipe(
+    Option.fromPredicate((contentTypes: string[]) => {


pipe(mediaTypes, Option.fromPredicate(…), Option.map

XVincentX

We got this. Excellent test coverage.

fix: allow empty responses

ce3eb08

lag-of-death changed the title ~~[wip] fix: allow empty responses~~ allow empty responses Sep 13, 2019

chore: add changelog line

371ecad

lag-of-death requested review from XVincentX, philsturgeon, mallachari and karol-maciaszek September 13, 2019 15:44

karol-maciaszek suggested changes Sep 16, 2019

View reviewed changes

packages/http/src/mocker/negotiator/NegotiatorHelpers.ts Outdated Show resolved Hide resolved

packages/http/src/mocker/negotiator/NegotiatorHelpers.ts Outdated Show resolved Hide resolved

XVincentX mentioned this pull request Sep 17, 2019

[WIP] fix: empty security response #609

Closed

2 tasks

XVincentX and others added 3 commits September 17, 2019 15:04

Merge branch 'master' into fix/empty-responses

4ebb97c

feat: answer with an empty response

21a229f

Merge branch 'fix/empty-responses' of github.com:stoplightio/prism in…

32ea654

…to fix/empty-responses_

lag-of-death requested review from karol-maciaszek and XVincentX and removed request for XVincentX September 17, 2019 13:40

lag-of-death and others added 3 commits September 17, 2019 16:09

Merge branch 'master' into fix/empty-responses

2230159

Fix/dynamic response fields (#623)

ba81061

* chore: upgrade jsf * test: add e2e * chore: switch back to npm version * docs: changelog * test: dry

Merge branch 'master' into fix/empty-responses

b7b53ad

XVincentX force-pushed the master branch from ba81061 to f30fe42 Compare September 17, 2019 14:52

Merge branch 'master' into fix/empty-responses

714e7db

XVincentX suggested changes Sep 18, 2019

View reviewed changes

lag-of-death added 5 commits September 18, 2019 12:28

Merge branch 'master' of github.com:stoplightio/prism into fix/empty-…

72825ac

…responses_ # Conflicts: # CHANGELOG.md

refactor: use options and closures

320631f

refactor: use fromOption

c69f624

refactor: do not pass logger to whenNoContent

fa6a535

refactor: rename whenNoContent

31fb5ec

lag-of-death requested a review from XVincentX September 18, 2019 14:33

Merge branch 'master' into fix/empty-responses

ff16842

lag-of-death commented Sep 20, 2019

View reviewed changes

packages/http/src/mocker/negotiator/__tests__/NegotiatorHelpers.spec.ts Show resolved Hide resolved

XVincentX reviewed Sep 20, 2019

View reviewed changes

lag-of-death added 7 commits September 20, 2019 15:50

Merge branch 'master' into fix/empty-responses

6ed9e8d

refactor: address feedback

a76daa7

refactor: rm direct import of fromNullable

554a008

refactor: some cleanup

704bd47

refactor: use info instead of warn

147e834

refactor: adjust to mater changes

058998e

chore: add todo

ab2c882

lag-of-death changed the title ~~[wip] allow empty responses~~ allow empty responses Sep 20, 2019

lag-of-death requested a review from XVincentX September 20, 2019 15:52

Merge branch 'master' into fix/empty-responses

599ce8e

lag-of-death added this to the Oct '19 milestone Sep 23, 2019

XVincentX reviewed Sep 23, 2019

View reviewed changes

lag-of-death and others added 4 commits September 23, 2019 10:13

Update packages/http/src/mocker/negotiator/InternalHelpers.ts

9b90fe3

Co-Authored-By: Vincenzo Chianese <vincenz.chianese@icloud.com>

refactor: rename warnMsg to outputNoContentFoundMessage

de7ef0b

refactor: address some feedback

b385748

refactor: do not check if 400/422 response

7fefbd4

XVincentX reviewed Sep 23, 2019

View reviewed changes

packages/http/src/mocker/negotiator/NegotiatorHelpers.ts Outdated Show resolved Hide resolved

lag-of-death added 4 commits September 23, 2019 13:14

refactor: simplify logic

d6beda6

refactor: renaming

ca37fd9

refactor: change the way of casting

a838a83

test: adjust tests descriptions

30e0d2c

lag-of-death requested a review from XVincentX September 23, 2019 11:49

XVincentX approved these changes Sep 23, 2019

View reviewed changes

refactor: put mediaTypes in pipe

0408241

XVincentX approved these changes Sep 23, 2019

View reviewed changes

XVincentX merged commit b65c945 into master Sep 23, 2019

XVincentX deleted the fix/empty-responses branch September 23, 2019 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow empty responses #606

allow empty responses #606

lag-of-death commented Sep 13, 2019 •

edited by XVincentX

Loading

karol-maciaszek left a comment

XVincentX left a comment

lag-of-death commented Sep 18, 2019

XVincentX commented Sep 18, 2019

XVincentX left a comment

XVincentX Sep 20, 2019

XVincentX left a comment

XVincentX Sep 23, 2019

XVincentX Sep 23, 2019

XVincentX Sep 23, 2019

XVincentX Sep 23, 2019

lag-of-death commented Sep 23, 2019

XVincentX left a comment

XVincentX Sep 23, 2019

XVincentX left a comment

allow empty responses #606

allow empty responses #606

Conversation

lag-of-death commented Sep 13, 2019 • edited by XVincentX Loading

Checklist

What kind of change does this PR introduce?

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

karol-maciaszek left a comment

Choose a reason for hiding this comment

XVincentX left a comment

Choose a reason for hiding this comment

lag-of-death commented Sep 18, 2019

XVincentX commented Sep 18, 2019

XVincentX left a comment

Choose a reason for hiding this comment

XVincentX Sep 20, 2019

Choose a reason for hiding this comment

XVincentX left a comment

Choose a reason for hiding this comment

XVincentX Sep 23, 2019

Choose a reason for hiding this comment

XVincentX Sep 23, 2019

Choose a reason for hiding this comment

XVincentX Sep 23, 2019

Choose a reason for hiding this comment

XVincentX Sep 23, 2019

Choose a reason for hiding this comment

lag-of-death commented Sep 23, 2019

XVincentX left a comment

Choose a reason for hiding this comment

XVincentX Sep 23, 2019

Choose a reason for hiding this comment

XVincentX left a comment

Choose a reason for hiding this comment

lag-of-death commented Sep 13, 2019 •

edited by XVincentX

Loading