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

refactor: validation returns eithers #731

Merged
merged 52 commits into from
Oct 30, 2019
Merged

Conversation

XVincentX
Copy link
Contributor

@XVincentX XVincentX commented Oct 22, 2019

This PR is a start to a refactor of the validators approach.

As I mentioned somewhere, the idea is to have 3 PRs that will do the job

Here's what I did:

  1. The validator generic signature now does not return a IPrismDiagnostic[], but rather an Either<NonEmptyArray<IPrismDiagnostic>, T> — which means either a validated T or an array of validation errors. https://github.com/stoplightio/prism/pull/731/files#diff-2991521f751aa9cf68976ceef512e19a
  2. I have created a sequence for an Either with the applicative method modified to concat the errors, as the article we were reading about was suggesting: https://github.com/stoplightio/prism/pull/731/files#diff-d6f4104cbf0cdb51855c413bc0079241R43-R44
  3. All the validators have been modified to return the Either — https://github.com/stoplightio/prism/pull/731/files#diff-ec5f393de032d67a4f08a12ecde5456cR54, and then all sequenced together to get the final request/response validation https://github.com/stoplightio/prism/pull/731/files#diff-52fafd78c752aca126e4fd8c4ce567a6R98-R103
  4. All the improvements then finally converge in the factory.ts file where you can see that finally validation and mocking are two different TaskEither steps melted together with chain calls.

@XVincentX XVincentX mentioned this pull request Oct 24, 2019
@XVincentX
Copy link
Contributor Author

shouldn't the refactor address validators vs serializers in the first place? Is this something that will be addressed in the next PRs?

This will be a follow up PR. This is just the beginning of breaking it down so that we can easily at least compose the steps (route, validate input, mock, validate output, send response) that is required for Hosted Prism. We'll iterate and move on as we go.

@XVincentX XVincentX mentioned this pull request Oct 29, 2019
2 tasks
inputValidations: IPrismDiagnostic[];
};

const inputValidation = (
Copy link
Contributor

Choose a reason for hiding this comment

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

something more verb-ish, like:

Suggested change
const inputValidation = (
const getInputValidation = (

describe('request is not set', () => {
it('validates headers', validate(undefined, 3));
it('does not validate headers', validate(undefined, undefined, 0));
Copy link
Contributor

Choose a reason for hiding this comment

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

there's a couple of these, does it mean that there's a user-facing change in Prism behavior or is it just a part of the refactor?

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly, why the number of validation errors changed from 0 to 3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. So previously our validators were ran even if there was no need to run them. You can see it here: https://github.com/stoplightio/prism/pull/731/files#diff-52fafd78c752aca126e4fd8c4ce567a6L45-L47

You can see that the functions were called anyway even if there was nothing to validate. In the following lines instead you can see there's a check to make sure it's the case to call them or otherwise call directly Either.right

In this tests, the validators were mocked to return an error in any case, so no matter what was happening, all the validators were always called and returning an error. Body + Query + Headers = 3 validators — that explains the number.

Copy link
Contributor

@karol-maciaszek karol-maciaszek left a comment

Choose a reason for hiding this comment

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

I have really no improvement requests for this PR. Some parts were mindblowing. I took me double-espresso to understand validationSequence.

I'd like to discuss that comment in context of further refactoring: https://github.com/stoplightio/prism/pull/731/files#diff-2991521f751aa9cf68976ceef512e19aR23

azure-pipelines.yml Show resolved Hide resolved
packages/core/src/types.ts Show resolved Hide resolved
packages/http/src/mocker/__tests__/functional.spec.ts Outdated Show resolved Hide resolved
describe('request is not set', () => {
it('validates headers', validate(undefined, 3));
it('does not validate headers', validate(undefined, undefined, 0));
Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly, why the number of validation errors changed from 0 to 3?

Co-Authored-By: Karol Maciaszek <karol@maciaszek.pl>
@XVincentX XVincentX merged commit 533208b into master Oct 30, 2019
@XVincentX XVincentX deleted the feat/validator-either branch October 30, 2019 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants