Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: respect the required parameter in the body #450

Merged
merged 30 commits into from
Jul 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
f51591f
feat: kill the validator registry concept for body
XVincentX Jul 10, 2019
bc7ea22
test: kill tests that aren't needed anymore
XVincentX Jul 10, 2019
0277d37
test: kill snapshot
XVincentX Jul 10, 2019
ec67ffe
test: keep the test that make sense
XVincentX Jul 10, 2019
9538e5f
test: use a verb that does not exist for real
XVincentX Jul 10, 2019
8229744
Merge branch 'master' into feat/validator-registry-off
XVincentX Jul 10, 2019
34d1346
Merge branch 'master' into feat/validator-registry-off
XVincentX Jul 10, 2019
3e74fa7
test: fix the documents
XVincentX Jul 10, 2019
4985851
test: add harness test 1
XVincentX Jul 10, 2019
c15d35b
test: harness part 2
XVincentX Jul 10, 2019
60be4c8
test: add shell:true flag
XVincentX Jul 10, 2019
fc933db
fix: put better error message when stuff does not match
XVincentX Jul 10, 2019
2f94dfb
test: adjust responses
XVincentX Jul 10, 2019
f890bc4
chore(deps): [security] bump lodash.template from 4.4.0 to 4.5.0 (#447)
dependabot-preview[bot] Jul 10, 2019
7572966
chore(deps): [security] bump lodash from 4.17.11 to 4.17.13 (#448)
dependabot-preview[bot] Jul 11, 2019
aadffed
use bash for harness on windows
XVincentX Jul 11, 2019
df980e9
Merge branch 'master' into feat/validator-registry-off
XVincentX Jul 11, 2019
f77f040
test: no harness on windows
XVincentX Jul 11, 2019
bdbbfa3
test: add failing test
XVincentX Jul 11, 2019
4682ba4
docs: precise yet another limitation
XVincentX Jul 11, 2019
76d2fd9
fix: add validation error if body is required but not present
XVincentX Jul 11, 2019
65ebac9
feat: validate only when necessary
XVincentX Jul 11, 2019
bf8d637
test:fix
XVincentX Jul 11, 2019
082e52d
chore: cleanup
XVincentX Jul 11, 2019
3928e74
test: make test behave as they should
XVincentX Jul 11, 2019
a071b25
chore: only post has body
XVincentX Jul 11, 2019
62efb4d
test:fix
XVincentX Jul 11, 2019
a30aa19
Merge branch 'master' into feat/body-validation-required
XVincentX Jul 12, 2019
1f1780a
Merge branch 'master' into feat/body-validation-required
XVincentX Jul 12, 2019
ecee804
test: do not repeat the test stuff and generalise
XVincentX Jul 12, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions packages/core/src/factory.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { DiagnosticSeverity } from '@stoplight/types';
import { toError } from 'fp-ts/lib/Either';
import { fromEither, left2v, right2v, tryCatch } from 'fp-ts/lib/TaskEither';
import { fromEither, left2v, right2v } from 'fp-ts/lib/TaskEither';
import { configMergerFactory, PartialPrismConfig, PrismConfig } from '.';
import { IPrism, IPrismComponents, IPrismConfig, IPrismDiagnostic, PickRequired, ProblemJsonError } from './types';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"swagger": "2.0",
"paths": {
"/pet": {
"get": {
"post": {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case you're wondering, get should not really have body parameters.

"produces": ["application/json"],
"consumes": ["application/json"],
"parameters": [
Expand Down
6 changes: 3 additions & 3 deletions packages/http-server/src/__tests__/server.oas.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,12 @@ describe('GET /pet?__server', () => {
}
});

describe('GET /pet with invalid body', () => {
describe('POST /pet with invalid body', () => {
it('returns correct error message', async () => {
const server = await instantiatePrism(resolve(__dirname, 'fixtures', 'getOperationWithBody.oas2.json'));

const response = await server.fastify.inject({
method: 'GET',
method: 'POST',
url: '/pet',
payload: {
id: 'strings are not valid!',
Expand All @@ -78,7 +78,7 @@ describe('GET /pet with invalid body', () => {
const parsed = JSON.parse(response.payload);
expect(parsed).toMatchObject({
type: 'https://stoplight.io/prism/errors#UNPROCESSABLE_ENTITY',
validation: [{ location: ['body'], severity: 'Error', code: 'type', message: 'should be object' }],
validation: [{ location: ['body', 'id'], severity: 'Error', code: 'type', message: 'should be integer' }],
});
await server.fastify.close();
});
Expand Down
48 changes: 36 additions & 12 deletions packages/http/src/validator/__tests__/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ describe('HttpValidator', () => {

describe('validateInput()', () => {
describe('body validation in enabled', () => {
const test = (extendResource?: Partial<IHttpOperation>) => () => {
const test = (extendResource: Partial<IHttpOperation> | undefined, expectedError: IPrismDiagnostic[]) => () => {
jest
.spyOn(resolveValidationConfigModule, 'resolveRequestValidationConfig')
.mockReturnValueOnce({ body: true, headers: false, hijack: false, query: false });
Expand All @@ -53,30 +53,54 @@ describe('HttpValidator', () => {
input: { method: 'get', url: { path: '/' } },
config: { mock: { dynamic: false }, validate: { request: { body: true } } },
}),
).toEqual([mockError]);
).toEqual(expectedError);

expect(resolveValidationConfigModule.resolveRequestValidationConfig).toHaveBeenCalled();
expect(httpBodyValidator.validate).toHaveBeenCalledWith(undefined, [], undefined);
expect(httpBodyValidator.validate).not.toHaveBeenCalled();
expect(httpHeadersValidator.validate).not.toHaveBeenCalled();
expect(httpQueryValidator.validate).not.toHaveBeenCalled();
};

describe('request is not set', () => {
it('validates body', test());
it('does not try to validate the body', test(undefined, []));
});

describe('request is set', () => {
describe('request.body is not set', () => {
it('validates body', test({ request: { path: [], headers: [], query: [], cookie: [] } }));
it(
'does not try to validate the body',
test({ request: { path: [], headers: [], query: [], cookie: [] } }, []),
);
});

describe('request.body is set', () => {
it(
'validates body',
test({
request: { body: { contents: [] }, path: [], query: [], headers: [], cookie: [] },
}),
);
describe('request body is not required', () => {
XVincentX marked this conversation as resolved.
Show resolved Hide resolved
it(
'does not try to validate the body',
test(
{
request: { body: { contents: [] }, path: [], query: [], headers: [], cookie: [] },
},
[],
),
);
});

describe('request body is required', () => {
it(
'tries to validate the body',
test(
{
method: 'get',
path: '/',
id: '1',
request: { body: { contents: [], required: true } },
responses: [{ code: '200' }],
},
[{ message: 'Body parameter is required', code: 'required', severity: DiagnosticSeverity.Error }],
),
);
});
});
});
});
Expand Down Expand Up @@ -196,7 +220,7 @@ describe('HttpValidator', () => {

describe('output is set', () => {
describe('body validation is enabled', () => {
it('validates body', async () => {
it('validates the body', async () => {
jest
.spyOn(resolveValidationConfigModule, 'resolveResponseValidationConfig')
.mockReturnValueOnce({ headers: false, body: true });
Expand Down
15 changes: 10 additions & 5 deletions packages/http/src/validator/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { IPrismDiagnostic, IValidator } from '@stoplight/prism-core';
import { IHttpContent, IHttpHeaderParam, IHttpOperation, IHttpQueryParam } from '@stoplight/types';
import { DiagnosticSeverity, IHttpContent, IHttpHeaderParam, IHttpOperation, IHttpQueryParam } from '@stoplight/types';
import * as caseless from 'caseless';

import { IHttpConfig, IHttpNameValue, IHttpNameValues, IHttpRequest, IHttpResponse } from '../types';
Expand Down Expand Up @@ -33,10 +33,15 @@ export class HttpValidator implements IValidator<IHttpOperation, IHttpRequest, I

if (config.body) {
const { body } = input;

this.bodyValidator
.validate(body, (request && request.body && request.body.contents) || [], mediaType)
.forEach(validationResult => results.push(validationResult));
if (request && request.body) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be way easier to read when we'll introduce Option<T> in fp-ts.

if (!body && request.body.required) {
results.push({ code: 'required', message: 'Body parameter is required', severity: DiagnosticSeverity.Error });
} else if (body) {
this.bodyValidator
.validate(body, (request && request.body && request.body.contents) || [], mediaType)
.forEach(validationResult => results.push(validationResult));
}
}
}

if (config.headers) {
Expand Down
1 change: 1 addition & 0 deletions test-harness/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ Connection: keep-alive
* The 4 keywords are `test,spec,server,command,expect,expect-loose`, nothing else at the moment
* You can run all the tests on the same port `4010`, but you can also choose another one
* The `curl` command does not support piping stuff into other tools; so if you're trying to be cool and do `curl | grep`, well maybe next time.
* All the `curl` commands **must** have the `-i` flag, otherwise the trace parser won't understand the output

## Technical details

Expand Down
39 changes: 39 additions & 0 deletions test-harness/specs/body-required-parameter.oas2.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
====test====
When I send a request to an operation that has a body parameter
and such body parameter is not required
I shuold get 200 response
====spec====
swagger: '2.0'
paths:
/pet:
post:
produces:
- application/json
consumes:
- application/json
parameters:
- in: body
name: body
required: false
schema:
type: object
properties:
id:
type: integer
format: int64
responses:
'200':
schema:
type: string
====server====
mock -p 4010
====command====
curl -i -X POST http://127.0.0.1:4010/pet
====expect====
HTTP/1.1 200 OK
content-type: application/json; charset=utf-8
content-length: 7
Date: Fri, 21 Jun 2019 09:25:34 GMT
Connection: keep-alive

"string"