Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

Work around an APIB parser bug (multipart/form-data) #177

Merged
merged 3 commits into from
Jul 31, 2018

Conversation

honzajavorek
Copy link
Contributor

Work around apiaryio/api-blueprint#401. This was previously a core part of Dredd. It belongs to Dredd Transactions though as it's a format-specific bug fix. Ideally, Dredd shouldn't need to know anything about the specifics of formats, Dredd Transactions should provide a uniform interface.

This allows better refactoring of the HTTP client infrastructure in apiaryio/dredd#1094.

This was previously a core part of Dredd. It belongs to Dredd Transactions
though as it's a format-specific bug fix. Ideally, Dredd shouldn't need
to know anything about the specifics of formats, Dredd Transactions
should provide a uniform interface.

apiaryio/api-blueprint#401
Copy link
Member

@kylef kylef left a comment

Choose a reason for hiding this comment

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

This makes sense to me, but I am not sure it is API Blueprint specific. If a user was to manually specify a multipart example in Swagger 2 similar to in API Blueprint I bet it won't also contain \r unless the user was really careful about adding \r on this specific lines (providing YAML parser doesn't just eat them).

That said, most users will be leaving \r out so I expect if you define manual multipart response example you'd hit the same problem.

src/compile.js Outdated
if (mediaType === 'text/vnd.apiblueprint') {
// Fixing 'multipart/form-data' bodies coming from the API Blueprint
// parser: https://github.com/apiaryio/api-blueprint/issues/401
[].concat(...transactions.map(t => [t.request, t.response]))
Copy link
Member

Choose a reason for hiding this comment

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

Could be simpler using flat (or maybe even using reduce).

transactions
  .map(t => [t.request, t.response])
  .flat()
  .filter(isMultipart)
  .forEach...

Copy link
Contributor Author

@honzajavorek honzajavorek Jul 30, 2018

Choose a reason for hiding this comment

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

Yes! Looks much better. I forgot about flat(). In my new commit I removed this part completely though.

@honzajavorek
Copy link
Contributor Author

@kylef Those are great points! I reworked the solution. I used it for Swagger as well and I promoted it from "hack at the end of the file" to an integral part of the compilation process. Let me know what you think.

Copy link

@michalholasek michalholasek left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@honzajavorek honzajavorek merged commit eb8f356 into master Jul 31, 2018
@honzajavorek honzajavorek deleted the honzajavorek/apib-multipart branch July 31, 2018 07:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants