feature: add server validation the Http Client #351

chris-miaskowski · 2019-06-12T15:54:00Z

This PR adds server validation the Prism Http Server and Prism Http Client.

What changed?

We have exposed a query parameter __server that you can use to check whether the server matches the one in the spec.

This is now supported in the CLI

prism mock spec.json
curl -X GET /pet?__server=http://someserver.com/basePath

and in the JS-API

prism.process({
   url: {
      baseUrl: 'http://xyz.com/api'
   }
})

More details in the README.md file

SO-257

…-257/server-validation

SO-257

chris-miaskowski · 2019-06-12T15:55:05Z

@XVincentX @philsturgeon @lag-of-death
Feel free to start reviewing folks. I have to add a bunch of e2e tests and update the readme but other than that it's done.

Pay special attention to tests so see whether we're all in alignment.

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

packages/http/src/__tests__/http-prism-instance.spec.ts

XVincentX · 2019-06-12T21:56:36Z

@chris-miaskowski I fixed the build for you.

packages/core/src/factory.ts

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

packages/http/src/__tests__/http-prism-instance.spec.ts

packages/http/src/forwarder/HttpForwarder.ts

lag-of-death · 2019-06-13T08:36:52Z

packages/http/src/router/index.ts

@@ -63,36 +63,42 @@ export const router: IRouter<IHttpOperation, IHttpRequest, IHttpConfig> = {
      };
    });

+    matches = matches.filter(match => match.pathMatch !== MatchType.NOMATCH);


this route function is so long and full of ifs, mutations and throwing errors. The cognitive load one has to invest to understand it is a bit too much. Can this be refactored before it's too late?

I want to ditch it completely and use a routing library, such as https://github.com/delvedor/find-my-way

Happy to see this refactored though.

@XVincentX to your point on the library I'm happy to replace the code if you find a library that does what we need. We have a very special use case here - we not only route but check the server validity.

@lag-of-death I'm happy to see what I can do if you can suggest a refactoring.

at least we could move all these ifs that throw errors and make an array of "tuples" like so:
[{predicate: () => false, action: () => throw 'A'}, {predicate: () => true, action: () => throw 'B'}]
and move this array out of route

Hey I'd like to leave that for some other time. I'm usually all for boy scouting but lots of things will change in the code base in next weeks/months so that may soon be irrelevant.

XVincentX · 2019-06-13T08:42:05Z

P.S: I'm jumping on this in the afternoon.

SO-257

…htio/prism into feature/so-257/server-validation

SO-257

README.md

chris-miaskowski · 2019-06-13T13:04:39Z

Thanks for the review everyone! I hear you all!
As much as I'd love to clean up the code I'm not sure if I will have the time to boyscout it now.
Will see what I can do but the priority for me now is to get the feature in.

README.md

XVincentX

I've added a first batch. I want to have a second look though

CONTRIBUTING.md

README.md

examples/petstore.oas3.json

packages/http/package.json

packages/http/src/__tests__/http-prism-instance.spec.ts

packages/http/src/forwarder/HttpForwarder.ts

SO-257

…htio/prism into feature/so-257/server-validation

…-257/server-validation

SO-257

…htio/prism into feature/so-257/server-validation

SO-257

Co-Authored-By: Phil Sturgeon <me@philsturgeon.uk>

SO-257

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

SO-257

…htio/prism into feature/so-257/server-validation

- don't embarrass yourself Chris! Read the docs! SO-257

…htio/prism into feature/so-257/server-validation

XVincentX

Some suggestions, but I think this is good to go.

I'd probably avoid the string comparison and parse the content instead, but it shouldn't be blocking this from being merged.

XVincentX · 2019-06-14T13:11:30Z

README.md

+#### Server Validation
+
+OpenAPI lets API spec authors make only certain servers available, and they also allow certain operations to be restricted to certain servers. Make sure the server URL you plan to use is a valid server this the particular operation you are attempting.
+by providing a `__server` query param.


I guess this should be on the same line

packages/core/src/factory.ts

XVincentX · 2019-06-14T13:14:41Z

packages/core/src/factory.ts

@@ -37,23 +39,41 @@ export function factory<Resource, Input, Output, Config, LoadOpts>(
        // build the config for this request
        const configMerger = configMergerFactory(defaultConfig, customConfig, c);
        const configObj: Config | undefined = configMerger(input);


This is probably an old leftover.

Suggested change

const configObj: Config | undefined = configMerger(input);

const configObj: Config = configMerger(input);

XVincentX · 2019-06-14T13:26:00Z

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

+  });
+
+  const expectedPayload = (serverUrl: string) =>
+    `{"type":"https://stoplight.io/prism/errors#NO_SERVER_MATCHED_ERROR","title":"Route not resolved, no server matched.","status":404,"detail":"The base url ${serverUrl} hasn\'t been matched with any of the provided servers"}`;


I'm wondering if there's a way to check out this stuff without using this templated string.

XVincentX · 2019-06-14T13:26:55Z

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

+
+    expect(response.statusCode).toBe(422);
+    expect(response.payload).toEqual(
+      '{"type":"https://stoplight.io/prism/errors#UNPROCESSABLE_ENTITY","title":"Invalid request body payload","status":422,"detail":"Your request body is not valid: [{\\"path\\":[\\"body\\"],\\"code\\":\\"type\\",\\"message\\":\\"should be object\\",\\"severity\\":0}]"}',


Yeah, it might be worth parse this content

XVincentX · 2019-06-14T13:27:22Z

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

+    expect(response.payload).toEqual(
+      '{"type":"https://stoplight.io/prism/errors#UNPROCESSABLE_ENTITY","title":"Invalid request body payload","status":422,"detail":"Your request body is not valid: [{\\"path\\":[\\"body\\"],\\"code\\":\\"type\\",\\"message\\":\\"should be object\\",\\"severity\\":0}]"}',
+    );
+    await server.fastify.close();


This should be in a after statement

XVincentX · 2019-06-14T13:28:01Z

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

@@ -228,6 +295,67 @@ describe.each([['petstore.oas2.json'], ['petstore.oas3.json']])('server %s', fil
    }
  });

+  describe('server validation: given __server query param', () => {


According to Phil (or at least, if I understood correctly), these should be two nested describe blocks

XVincentX · 2019-06-14T13:28:34Z

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

+    });
+
+    // oas2 does not support overriding servers
+    if (file === 'petstore.oas3.json') {


This is ok for now, but maybe we could have oas3 specific tests instead of branching

philsturgeon · 2019-06-14T14:07:23Z

README.md

+On the other hand, putting a server which is not defined in the specification, for example:
+
+```bash
+curl -X GET localhost:4010/pet?__server=ftp://acme.com/api


Oh this is weird that we introduced FTP here just without any explanation, just snuck it in there. I honestly do not know if FTP is even allowed in OpenAPI, it doesn't mentioned either way but... it has never come up. https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#serverObject

Chris Miaskowski added 6 commits June 11, 2019 13:46

tests: clean up test cases and fixtures

aa6dac5

SO-257

fix: a bug when we incorrectly matched a path

e1679fa

SO-257

feat: add __server query param support

867437c

SO-257

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

a974de7

…-257/server-validation

feat: invalid path (forwarded) returns warning

9ed6460

SO-257

refactor: uncomment tests

0190e96

SO-257

chris-miaskowski requested review from philsturgeon, XVincentX and lag-of-death June 12, 2019 15:54

philsturgeon reviewed Jun 12, 2019

View reviewed changes

packages/http-server/src/__tests__/server.oas.spec.ts Show resolved Hide resolved

philsturgeon reviewed Jun 12, 2019

View reviewed changes

packages/http/src/__tests__/http-prism-instance.spec.ts Outdated Show resolved Hide resolved

XVincentX added 2 commits June 12, 2019 23:48

Merge branch 'master' into feature/so-257/server-validation

dff02bc

fix: require correct stuff

ef681d8

lag-of-death reviewed Jun 13, 2019

View reviewed changes

packages/core/src/factory.ts Show resolved Hide resolved

lag-of-death reviewed Jun 13, 2019

View reviewed changes

packages/http-server/src/__tests__/server.oas.spec.ts Outdated Show resolved Hide resolved

lag-of-death reviewed Jun 13, 2019

View reviewed changes

packages/http/src/__tests__/http-prism-instance.spec.ts Outdated Show resolved Hide resolved

lag-of-death reviewed Jun 13, 2019

View reviewed changes

packages/http/src/forwarder/HttpForwarder.ts Show resolved Hide resolved

lag-of-death reviewed Jun 13, 2019

View reviewed changes

Chris Miaskowski and others added 6 commits June 13, 2019 11:40

tests: templated servers handling

57fd3c5

SO-257

Merge branch 'feature/so-257/server-validation' of github.com:stoplig…

e5540c2

…htio/prism into feature/so-257/server-validation

refactor: simplify test

55f4266

refactor: simplify test

dbb0fdb

docs: add __servers example

55a7181

SO-257

Merge branch 'master' into feature/so-257/server-validation

b58d1ba

philsturgeon reviewed Jun 13, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

philsturgeon reviewed Jun 13, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

philsturgeon reviewed Jun 13, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

XVincentX reviewed Jun 13, 2019

View reviewed changes

Chris Miaskowski and others added 21 commits June 14, 2019 10:07

tests: fix test harness case

dabb6be

SO-257

Merge branch 'feature/so-257/server-validation' of github.com:stoplig…

847757b

…htio/prism into feature/so-257/server-validation

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

ad27ec0

…-257/server-validation

Merge branch 'master' into feature/so-257/server-validation

c55b6c7

tests: add __server e2e tests

2af250a

SO-257

Merge branch 'feature/so-257/server-validation' of github.com:stoplig…

920831e

…htio/prism into feature/so-257/server-validation

refactor: use more predictable naming

4628804

SO-257

Update README.md

fb3a6e2

Co-Authored-By: Phil Sturgeon <me@philsturgeon.uk>

docs: convert json example to yaml

3549d14

SO-257

Update README.md

6fa6528

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

Update README.md

7ac1345

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

refactor: minor tweaks

291ea54

SO-257

Merge branch 'feature/so-257/server-validation' of github.com:stoplig…

38f2698

…htio/prism into feature/so-257/server-validation

refactor: minor improvements

c90493f

docs: remove stale hint

7e2ae92

chore: move deps to root

3853fcc

- don't embarrass yourself Chris! Read the docs! SO-257

refactor: minor improvements

6676cb1

Merge branch 'master' into feature/so-257/server-validation

053b700

refactor: minor improvements

d7e4d85

Merge branch 'feature/so-257/server-validation' of github.com:stoplig…

aeb9e62

…htio/prism into feature/so-257/server-validation

refactor: minor improvements

9fc71d2

XVincentX approved these changes Jun 14, 2019

View reviewed changes

chris-miaskowski merged commit 6f1ece9 into master Jun 14, 2019

chris-miaskowski deleted the feature/so-257/server-validation branch June 14, 2019 13:30

philsturgeon reviewed Jun 14, 2019

View reviewed changes

feature: add server validation the Http Client #351

feature: add server validation the Http Client #351

chris-miaskowski commented Jun 12, 2019 •

edited

chris-miaskowski commented Jun 12, 2019

XVincentX commented Jun 12, 2019

lag-of-death Jun 13, 2019

XVincentX Jun 13, 2019

chris-miaskowski Jun 13, 2019

lag-of-death Jun 13, 2019

chris-miaskowski Jun 14, 2019

XVincentX commented Jun 13, 2019

chris-miaskowski commented Jun 13, 2019

XVincentX left a comment

XVincentX left a comment

XVincentX Jun 14, 2019

XVincentX Jun 14, 2019

XVincentX Jun 14, 2019

XVincentX Jun 14, 2019

XVincentX Jun 14, 2019

XVincentX Jun 14, 2019

XVincentX Jun 14, 2019

philsturgeon Jun 14, 2019

	const configObj: Config \| undefined = configMerger(input);
	const configObj: Config = configMerger(input);

feature: add server validation the Http Client #351

feature: add server validation the Http Client #351

Conversation

chris-miaskowski commented Jun 12, 2019 • edited

chris-miaskowski commented Jun 12, 2019

XVincentX commented Jun 12, 2019

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 Jun 13, 2019

chris-miaskowski commented Jun 13, 2019

XVincentX left a comment

Choose a reason for hiding this comment

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

chris-miaskowski commented Jun 12, 2019 •

edited