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: improved serializer selection #620

Merged
merged 12 commits into from
Sep 18, 2019
Merged

Conversation

karol-maciaszek
Copy link
Contributor

@karol-maciaszek karol-maciaszek commented Sep 16, 2019

Closes #589

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?

Bugfix

What is the current behavior? What is the new behavior?

Current behavior:

Accept header negotiation is done in two places: in negotiator and in http-server.

  • Negotiatior is selecting matching content based on Accept header and available content types.
  • http-server serializes content based on Accept header and available serializers.

To reproduce:

  1. spec file has content for application/json
  2. request has Accept: application/xml, application/json

Results in:
Negotiator selects application/json as it matches available contents.
http-server serializes content as xml because Accept header has xml and xml serializer exists.

New behavior:

http-server serialzes the content according to what negotiator negotiated.

Does this PR introduce a breaking change?

No

dependabot-preview bot and others added 2 commits September 10, 2019 11:29
* chore(deps): bump @stoplight/http-spec from 2.1.4 to 2.2.1

Bumps [@stoplight/http-spec](https://github.com/stoplightio/http-spec) from 2.1.4 to 2.2.1.
- [Release notes](https://github.com/stoplightio/http-spec/releases)
- [Commits](stoplightio/http-spec@v2.1.4...v2.2.1)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

* deps: upgrade again

* refactor: write test text and done type
@karol-maciaszek karol-maciaszek added the WIP Work In Progress label Sep 16, 2019
@karol-maciaszek karol-maciaszek changed the title [WIP] fix: improved serializer selection fix: improved serializer selection Sep 17, 2019
@karol-maciaszek karol-maciaszek removed the WIP Work In Progress label Sep 17, 2019
@lag-of-death
Copy link
Contributor

hi @StefanDywersant, please take a look at #604 (review)

@lag-of-death
Copy link
Contributor

@StefanDywersant, I'll take a look when the conflict is resolved 👍

Copy link
Contributor

@XVincentX XVincentX left a comment

Choose a reason for hiding this comment

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

Some minor comments but it looks good.

  • The fixtures should not be modified to return the charset because the OAS document does not specify it. If you apply my suggestions, it works correctly; I guess the default serialiser is still being kicked off for some reason.

  • It would be better to write this test as an E2E test; you can find more informations in the README of the test-harness directory.

CHANGELOG.md Outdated Show resolved Hide resolved
packages/http-server/src/serialize.ts Show resolved Hide resolved
packages/http-server/src/serialize.ts Show resolved Hide resolved
packages/http-server/src/server.ts Show resolved Hide resolved
packages/http-server/src/server.ts Show resolved Hide resolved
packages/http-server/src/serialize.ts Outdated Show resolved Hide resolved
packages/http-server/src/__tests__/server.oas.spec.ts Outdated Show resolved Hide resolved
packages/http-server/src/server.ts Outdated Show resolved Hide resolved
packages/http-server/src/server.ts Outdated Show resolved Hide resolved
@lag-of-death
Copy link
Contributor

@XVincentX, I understand your reasoning in #620 (comment)

Personally, I would only use the imperative style if something is not possible to do in FP. We don't really need the mayhem of mutations and exceptions being thrown throughout the code. I like your distinction here, it's like Elm/Haskell purity of the code vs side effects taken care of by the compilers, but would try to make that imperative world as small as possible still.

@XVincentX XVincentX merged commit c53030d into master Sep 18, 2019
@XVincentX XVincentX deleted the fix/serializer-selection branch September 18, 2019 16:32
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.

Mocked dynamic response content type not defined in OAS spec
3 participants