handle head requests #603

lag-of-death · 2019-09-11T12:18:27Z

Fixes #575

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

XVincentX

This looks good.

We just need to understand from a product perspective what we want to do with the HEAD method in general (skipping the whole negotiation process); this is up to @philsturgeon and in such case we might need to change a little bit the code, and move this check outside.
We should probably add some unit tests in the code since we have a pretty good tests battery.
Please add a changelog line

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

test-harness/specs/head_requests.oas3.txt

lag-of-death · 2019-09-13T16:00:18Z

@philsturgeon, could you take a look at the questions ^ ? thanks!

…o master # Conflicts: # packages/http/src/mocker/negotiator/NegotiatorHelpers.ts

…distaster

XVincentX

Dropped another bunch of comments. We're getting closer, we need to make some other changes though.

CHANGELOG.md

XVincentX · 2019-09-17T16:04:06Z

packages/http/src/mocker/HttpMocker.ts

-      chain(result => assembleResponse(result, payloadGenerator)),
-    );
+    const isHead = input.data.method === 'head';
+    const r = get(resource, ['responses', '0'], { code: 200, headers: [] });


This should only be happening if isHead is true, otherwise the result will never be used.

This won't be enough, it's simply relying on the first response; imagine my first response is a 500 instead of a 200 (or also a 204) — this is not what we want to return. We still need to go through some parts of the negotiation process. It would be a good idea to add a test to make sure it behaves correctly on this.

We still need input validation (query string and whatever)

We need to still go through the regular response checking.

I guess I was kind of wrong when I was saying we can skip most of the negotiation process, I am sorry.

I think the best way to plug the code would be here:

https://github.com/stoplightio/prism/blob/master/packages/http/src/mocker/negotiator/NegotiatorHelpers.ts#L140

We can detect here if we're dealing with an head request and in such case early return a right with an empty body skipping all the code we see. If we could write a log line as well, that'd be great.

Does that make sense?

packages/http/src/mocker/HttpMocker.ts

XVincentX · 2019-09-17T16:14:53Z

test-harness/specs/head_requests.oas3.txt

+curl --head http://localhost:4010/todos
+====expect====
+HTTP/1.1 200 OK
+nonce: abc


Can we also add a test to make sure that if we request an HEAD and this is not defined in the document, we get a 404?

Just to make sure the router stops the request entirely. I know this is covered in unit tests as well, but you never know I guess :)

…o HEAD

# Conflicts: # CHANGELOG.md

XVincentX

4 very very minor things and we can finally merge this 🚀

XVincentX · 2019-09-18T16:37:47Z

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

@@ -135,6 +134,15 @@ const helpers = {
    const { mediaTypes, dynamic, exampleKey } = desiredOptions;

    return withLogger(logger => {
+      if (_httpOperation.method === 'head') {
+        logger.warn(`Responding with an empty body to a HEAD request.`);


This should not be a warn, there's nothing to be worried about. We can use info

XVincentX · 2019-09-18T16:39:25Z

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

@@ -4,7 +4,7 @@ import { IHttpOperationConfig, JSONSchema } from '../../';

 export interface IHttpNegotiationResult {
  code: string;
-  mediaType: string;
+  mediaType?: string;


Ah ok I understand now, if the request is HEAD there is no media type we're effectively returning.

XVincentX · 2019-09-18T16:41:11Z

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

+
+          expect(Either.isRight(actualResponse)).toBeTruthy();
+
+          pipe(


use the assertRight function.

assertRight( actualResponse, response => { expect(response).not.toHaveProperty('bodyExample'); expect(response).not.toHaveProperty('mediaType'); expect(response).not.toHaveProperty('schema'); }, );

XVincentX · 2019-09-18T16:42:14Z

CHANGELOG.md

@@ -31,6 +31,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Prism is correctly handling the `csv` collection format argument property in OAS2 documents [#577](https://github.com/stoplightio/prism/pulls/577)
 - Prism is correctly returning the response when the request has `*/*` as Accept header [#578](https://github.com/stoplightio/prism/pulls/578)
 - Prism is correctly returning a single root node with the payload for XML data [#578](https://github.com/stoplightio/prism/pulls/578)
+- HEAD requests no longer fail with 406 Not Acceptable #603


This is not in the right section. It should go In the Fixed part of Unreleased section

feat: handle head requests

742e9bc

lag-of-death changed the title ~~[wip] feat: handle head requests~~ handle head requests Sep 11, 2019

lag-of-death requested review from XVincentX, philsturgeon, pytlesk4 and karol-maciaszek September 11, 2019 12:36

XVincentX suggested changes Sep 11, 2019

View reviewed changes

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

chore: add changelog line

0fc8694

philsturgeon reviewed Sep 11, 2019

View reviewed changes

test-harness/specs/head_requests.oas3.txt Outdated Show resolved Hide resolved

test: add unit test

39ce4f8

lag-of-death requested review from philsturgeon and XVincentX September 12, 2019 16:12

lag-of-death added 7 commits September 16, 2019 12:38

Merge branch 'feat/head-requests' of github.com:stoplightio/prism int…

fe5a873

…o master # Conflicts: # packages/http/src/mocker/negotiator/NegotiatorHelpers.ts

fix: adjust the code to changes in master

50cfbd7

Merge branch 'master' of github.com:stoplightio/prism into master_of_…

988f5bc

…distaster

refactor: skip negotation when responding to a HEAD request

d657949

test: rm redundant test

f5d4388

test: do not change test

38421f1

test: fix tests

2733487

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

lag-of-death force-pushed the feat/head-requests branch from ee7cc95 to 2733487 Compare September 17, 2019 15:00

lag-of-death added 2 commits September 17, 2019 17:02

Merge branch 'master' into feat/head-requests

47c9e5e

refactor: rm changes

36e129c

XVincentX suggested changes Sep 17, 2019

View reviewed changes

lag-of-death added 2 commits September 18, 2019 11:08

refactor: rm changes

f846067

Merge branch 'feat/head-requests' of github.com:stoplightio/prism int…

13f5a78

…o HEAD

lag-of-death force-pushed the feat/head-requests branch from 36e129c to 5968a89 Compare September 18, 2019 09:09

Merge branch 'master' of github.com:stoplightio/prism into HEAD

b733051

# Conflicts: # CHANGELOG.md

lag-of-death force-pushed the feat/head-requests branch from 5968a89 to b733051 Compare September 18, 2019 09:13

lag-of-death added 7 commits September 18, 2019 11:26

chore: adjust changelog

89f4495

style: rm line

5078bd7

refactor: adjust the codee

eb371a9

test: add tests

66a850c

test: add unit test

82ca8a7

refactor: rm get

0e2659c

style: add white space

e4c893d

lag-of-death requested review from XVincentX and removed request for philsturgeon September 18, 2019 10:11

Merge branch 'master' into feat/head-requests

174e48d

XVincentX suggested changes Sep 18, 2019

View reviewed changes

XVincentX added 3 commits September 18, 2019 18:45

test: use assert right

6347126

refactor: logger.info for head request detected

4c266e3

docs: fix changelog

21f034a

XVincentX force-pushed the feat/head-requests branch from 07cd1fb to 21f034a Compare September 18, 2019 16:48

XVincentX approved these changes Sep 18, 2019

View reviewed changes

XVincentX merged commit e04eb2e into master Sep 18, 2019

XVincentX deleted the feat/head-requests branch September 18, 2019 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle head requests #603

handle head requests #603

lag-of-death commented Sep 11, 2019 •

edited

Loading

XVincentX left a comment

lag-of-death commented Sep 13, 2019

XVincentX left a comment

XVincentX Sep 17, 2019

XVincentX Sep 17, 2019

XVincentX Sep 17, 2019

XVincentX left a comment

XVincentX Sep 18, 2019

XVincentX Sep 18, 2019

XVincentX Sep 18, 2019

XVincentX Sep 18, 2019

handle head requests #603

handle head requests #603

Conversation

lag-of-death commented Sep 11, 2019 • edited Loading

Checklist

What kind of change does this PR introduce?

XVincentX left a comment

Choose a reason for hiding this comment

lag-of-death commented Sep 13, 2019

XVincentX left a comment

Choose a reason for hiding this comment

XVincentX Sep 17, 2019

Choose a reason for hiding this comment

XVincentX Sep 17, 2019

Choose a reason for hiding this comment

XVincentX Sep 17, 2019

Choose a reason for hiding this comment

XVincentX left a comment

Choose a reason for hiding this comment

XVincentX Sep 18, 2019

Choose a reason for hiding this comment

XVincentX Sep 18, 2019

Choose a reason for hiding this comment

XVincentX Sep 18, 2019

Choose a reason for hiding this comment

XVincentX Sep 18, 2019

Choose a reason for hiding this comment

lag-of-death commented Sep 11, 2019 •

edited

Loading