Compare content type parameters separately #1159

XVincentX · 2020-05-08T20:48:45Z

This pull request implements a backup strategy to ignore parameters in content type when is possible. Since they're not standard (apart for the q value) we won't assume anything about it and drop them in some cases

Closes #1152

marcelltoth

You could add a changelog if this is something that is user-visible. I'm not sure, TBH 😄

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

packages/http/src/validator/index.ts

XVincentX · 2020-05-11T23:57:28Z

.github/CODEOWNERS

@@ -1 +1 @@
-*       @XVincentX @marcelltoth @karol-maciaszek
+*       @XVincentX @karol-maciaszek


XVincentX · 2020-05-11T23:58:50Z

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

+      pipe(
+        mediaTypes
+          .map(mt => contentType.parse(mt))
+          .filter(mt => Object.keys(mt.parameters).length > 0)
+          .map(mt => mt.type),
+        fromArray,
+        O.chain(mediaTypesWithNoParameters => findBestHttpContentByMediaType(contents, mediaTypesWithNoParameters))


This is basically a "retry with only the Q parameter"

XVincentX · 2020-05-11T23:59:40Z

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

-          exampleKey,
-        },
-        response
+      return pipe(


This diff is huge but all I did here for real was to move the if(mediatypes) in the pipe

XVincentX · 2020-05-12T00:00:00Z

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

@@ -289,19 +291,19 @@ const helpers = {
            pipe(
              previous,
              O.alt(() => {
-                logger.trace(`Unable to find a ${statusCodes[index]} response definition`);
+                logger.debug(`Unable to find a ${statusCodes[index]} response definition`);


trace is not caught by our signals logger, so I moved it back to debug as it should be

XVincentX · 2020-05-12T00:00:24Z

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

-                  findEmptyResponse(
-                    response,
-                    response.headers || [],
-                    get(contentWithExamples, 'mediaType', get(responseWithSchema, 'schema')) || ['*/*']


contentWithExamples is always undefined in this branch, that's why I've removed it

karol-maciaszek

Good work and nice package of improvements. I dropped a few comments.

karol-maciaszek · 2020-05-12T12:09:15Z

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

+      pipe(
+        mediaTypes
+          .map(mt => contentType.parse(mt))
+          .filter(mt => Object.keys(mt.parameters).filter(k => k !== 'q').length > 0)


Suggested change

.filter(mt => Object.keys(mt.parameters).filter(k => k !== 'q').length > 0)

.filter(({ parameters }) => Object.keys(parameters).find(k => k !== 'q'))

or for more clarity:

Suggested change

.filter(mt => Object.keys(mt.parameters).filter(k => k !== 'q').length > 0)

.filter(({ parameters }) => !!Object.keys(parameters).find(k => k !== 'q'))

Yeah good point. @karol-maciaszek how about this: bbb4c73

karol-maciaszek · 2020-05-12T12:14:41Z

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

@@ -10,7 +10,7 @@ describe('InternalHelpers', () => {
      };

      it('should respect the q parameter', () => {
-        const possibleResponse = findBestHttpContentByMediaType(avaiableResponses, [
+        const possibleResponse = findBestHttpContentByMediaType(avaiableResponses.contents, [


Suggested change

const possibleResponse = findBestHttpContentByMediaType(avaiableResponses.contents, [

const possibleResponse = findBestHttpContentByMediaType(availableResponses.contents, [

Will need updates in the code above.

karol-maciaszek · 2020-05-12T12:25:19Z

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

+        );
+      });
+
+      describe('multiple media types avaiable', () => {


Suggested change

describe('multiple media types avaiable', () => {

describe('multiple media types available', () => {

karol-maciaszek · 2020-05-12T12:29:46Z

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

+                { mediaType: 'application/json; version=1; q=1' },
+                { mediaType: 'application/json; version=1; q=0.6' },


Suggested change

{ mediaType: 'application/json; version=1; q=1' },

{ mediaType: 'application/json; version=1; q=0.6' },

{ mediaType: 'application/json; version=1; q=0.6' },

{ mediaType: 'application/json; version=1; q=1' },

karol-maciaszek · 2020-05-12T12:33:00Z

packages/http/src/validator/__tests__/index.spec.ts

+    };
+
+    describe('and the requeste media type matches, but has parameter', () => {
+      const result = validator.validateMediaType([content], 'application/vnd.archa.api+json; version=1');


I have bad experience in tracking errors outside it() functions. Anything against moving to it()?

XVincentX · 2020-05-12T13:30:13Z

You could add a changelog if this is something that is user-visible.

@marcelltoth I added the changelog line indeed.

packages/http/src/validator/__tests__/index.spec.ts

Co-authored-by: Karol Maciaszek <karol@maciaszek.pl>

karol-maciaszek

Ship it!

* compare parameters separately * tmp: mediaType from nullable * use isMatch * Parse media type only once * tmp changes * keep the chain as much as possible * add launch.json instructions * remove incorrect assumption * you can alwasy create an empty response * avoid nesting * create empty response cannot fail * do not bloat with stuff we do not need * restore stuff * restore * chore: restrict surface * reduce stuff * pipe all together * replace with debug * bye bye marcell * test: add validator test * use realish media types * remove parameters and try again * eeek * docs: changelog * test: add * test: refactor and add * test: harness * remove only * keep the q parameter * add yet another test * refactor: use some * spell error * spell error again * reverse order to make sure test is not passing by chance * move data retr in it block * move tests in internal helpers where they should be2 * test fix * Update packages/http/src/validator/__tests__/index.spec.ts Co-authored-by: Karol Maciaszek <karol@maciaszek.pl> * Update packages/http/src/validator/__tests__/index.spec.ts Co-authored-by: Karol Maciaszek <karol@maciaszek.pl> Co-authored-by: Karol Maciaszek <karol@maciaszek.pl>

compare parameters separately

115bf8d

XVincentX requested review from karol-maciaszek and marcelltoth as code owners May 8, 2020 20:48

XVincentX force-pushed the fix/ct-parameters branch from b0af738 to 86c3e58 Compare May 9, 2020 02:44

tmp: mediaType from nullable

7675796

XVincentX force-pushed the fix/ct-parameters branch from 86c3e58 to 7675796 Compare May 9, 2020 04:10

use isMatch

cd31e13

XVincentX force-pushed the fix/ct-parameters branch from 0978cc4 to cd31e13 Compare May 9, 2020 04:49

XVincentX added 2 commits May 8, 2020 23:51

Parse media type only once

09ba09b

tmp changes

bb708f8

marcelltoth reviewed May 11, 2020

View reviewed changes

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

packages/http/src/validator/index.ts Outdated Show resolved Hide resolved

XVincentX added 2 commits May 11, 2020 10:08

Merge branch 'master' into fix/ct-parameters

255882a

keep the chain as much as possible

0c265e0

XVincentX force-pushed the fix/ct-parameters branch from f2679b5 to 0c265e0 Compare May 11, 2020 16:06

add launch.json instructions

b734453

XVincentX force-pushed the fix/ct-parameters branch from 344df58 to aa54ed6 Compare May 11, 2020 16:56

remove incorrect assumption

c286dd6

XVincentX force-pushed the fix/ct-parameters branch from aa54ed6 to c286dd6 Compare May 11, 2020 17:39

XVincentX added 7 commits May 11, 2020 13:09

you can alwasy create an empty response

965c411

avoid nesting

943aa66

create empty response cannot fail

5cb630d

do not bloat with stuff we do not need

7b8dbc3

restore stuff

3413eb2

restore

9e1c085

chore: restrict surface

d4d4ab9

XVincentX force-pushed the fix/ct-parameters branch from afa29ef to d4d4ab9 Compare May 11, 2020 19:18

reduce stuff

5302c23

XVincentX force-pushed the fix/ct-parameters branch from dbbb502 to 5302c23 Compare May 11, 2020 19:23

XVincentX added 2 commits May 11, 2020 14:37

pipe all together

e8e7136

replace with debug

581c1be

XVincentX force-pushed the fix/ct-parameters branch from f50bed8 to 393b2c9 Compare May 11, 2020 23:32

test: harness

2286cd2

XVincentX force-pushed the fix/ct-parameters branch from 393b2c9 to 2286cd2 Compare May 11, 2020 23:41

remove only

2f85784

XVincentX force-pushed the fix/ct-parameters branch from b40fd4f to 2f85784 Compare May 12, 2020 00:01

keep the q parameter

2ab18ef

XVincentX commented May 12, 2020

View reviewed changes

XVincentX requested a review from pytlesk4 May 12, 2020 00:11

XVincentX force-pushed the fix/ct-parameters branch 3 times, most recently from 7e37d17 to 2ab18ef Compare May 12, 2020 00:36

add yet another test

1314478

XVincentX requested a review from marcelltoth May 12, 2020 00:54

karol-maciaszek suggested changes May 12, 2020

View reviewed changes

XVincentX added 7 commits May 12, 2020 08:32

refactor: use some

bbb4c73

spell error

bb632bf

spell error again

99de7b7

reverse order to make sure test is not passing by chance

cbdadf3

move data retr in it block

a3f0368

move tests in internal helpers where they should be2

cb8892e

test fix

790472d

karol-maciaszek reviewed May 12, 2020

View reviewed changes

packages/http/src/validator/__tests__/index.spec.ts Outdated Show resolved Hide resolved

karol-maciaszek reviewed May 12, 2020

View reviewed changes

packages/http/src/validator/__tests__/index.spec.ts Outdated Show resolved Hide resolved

XVincentX and others added 2 commits May 12, 2020 08:44

Update packages/http/src/validator/__tests__/index.spec.ts

324a42c

Co-authored-by: Karol Maciaszek <karol@maciaszek.pl>

Update packages/http/src/validator/__tests__/index.spec.ts

8977a70

Co-authored-by: Karol Maciaszek <karol@maciaszek.pl>

karol-maciaszek approved these changes May 12, 2020

View reviewed changes

XVincentX merged commit 7f8fe02 into master May 12, 2020

XVincentX deleted the fix/ct-parameters branch May 12, 2020 13:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compare content type parameters separately #1159

Compare content type parameters separately #1159

XVincentX commented May 8, 2020 •

edited

Loading

marcelltoth left a comment

XVincentX May 11, 2020

marcelltoth May 12, 2020

karol-maciaszek May 12, 2020

XVincentX May 11, 2020

XVincentX May 11, 2020

XVincentX May 12, 2020

XVincentX May 12, 2020

karol-maciaszek left a comment

karol-maciaszek May 12, 2020

XVincentX May 12, 2020

karol-maciaszek May 12, 2020

XVincentX May 12, 2020

karol-maciaszek May 12, 2020

XVincentX May 12, 2020

karol-maciaszek May 12, 2020

XVincentX May 12, 2020

karol-maciaszek May 12, 2020

XVincentX commented May 12, 2020

karol-maciaszek left a comment

		@@ -1 +1 @@
		* @XVincentX @marcelltoth @karol-maciaszek
		* @XVincentX @karol-maciaszek

	.filter(mt => Object.keys(mt.parameters).filter(k => k !== 'q').length > 0)
	.filter(({ parameters }) => Object.keys(parameters).find(k => k !== 'q'))

	.filter(mt => Object.keys(mt.parameters).filter(k => k !== 'q').length > 0)
	.filter(({ parameters }) => !!Object.keys(parameters).find(k => k !== 'q'))

	const possibleResponse = findBestHttpContentByMediaType(avaiableResponses.contents, [
	const possibleResponse = findBestHttpContentByMediaType(availableResponses.contents, [

	describe('multiple media types avaiable', () => {
	describe('multiple media types available', () => {

		{ mediaType: 'application/json; version=1; q=1' },
		{ mediaType: 'application/json; version=1; q=0.6' },

Compare content type parameters separately #1159

Compare content type parameters separately #1159

Conversation

XVincentX commented May 8, 2020 • edited Loading

marcelltoth 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

karol-maciaszek 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 commented May 12, 2020

karol-maciaszek left a comment

Choose a reason for hiding this comment

XVincentX commented May 8, 2020 •

edited

Loading