Verify whether Dredd handles binary data #1094

honzajavorek · 2018-07-20T17:07:12Z

🚀 Why this change?

This is to close the Epic: empty responses and binary files epic. Since the empty responses have been fixed, it is (probably!) already possible to test binary data with Dredd or to easily skip the testing. However, this isn't verified in Dredd's test suite and there are no examples in the docs.

🚧 To Do

test Dredd with "higher bytes", images, etc. Support setting Request encoding for response #836 and with non-JS hooks Find a way how to work with binary files in hooks #617
implement encoding of bodies in case they're binary (or a different solution)
refactor and unit test the implementation
unit test the performRequest itself
move the API Blueprint multipart fix to Dredd Transactions Work around an APIB parser bug (multipart/form-data) dredd-transactions#177
move multipart unit tests to Dredd Transactions
get all tests passing
write docs with examples

📝 Related issues and Pull Requests

Closes Find a way how to work with binary files in hooks #617
Closes Dredd testing of APIary calls returning an image / PNG #87
Deprecates Support setting Request encoding for response #836

✅ What didn't I forget?

To write docs
To write tests
To put Conventional Changelog prefixes in front of all my commits and run npm run lint

Addressing #836?

https://github.com/apiaryio/fury-adapter-swagger/issues/193

Uses Base64 encoding as the serialization, which allows also non-JS hooks to set request/response bodies to a binary content. Close #617 Close #87 Close #836

Normalize 'multipart/form-data' bodies for both API Blueprint and Swagger, avoid ignoring falsy values at multiple places (URI parameters, x-example)

michalholasek · 2018-08-01T09:42:45Z

src/performRequest.js

+
+  try {
+    httpOptions.body = getBodyAsBuffer(transactionReq.body, transactionReq.bodyEncoding);
+    httpOptions.headers = setContentLength(transactionReq.headers, httpOptions.body);


I think that setContentLength is a little bit misleading here, normalizeHeaders or something like that would be more fitting in this case.

It doesn't interfere with any other headers. It detects the name of the content-length header if it exists, and sets it to the correct value. In case it doesn't exists, it adds the header. IMHO the name is right.

normalizeContentLengthHeader then Q___(-_-)Q

michalholasek · 2018-08-01T09:52:05Z

src/performRequest.js

+    return 'base64';
+  }
+  throw new Error(`Unsupported encoding: '${encoding}' (only UTF-8 and `
+    + 'Base64 are supported)');


Nit: I would personally use switch instead if(s) wherever possible, something like:

switch (lcEncoding) { case 'utf-8': case 'utf8': return 'utf-8' case 'base64'; return 'base64'; default: throw new Error(`Unsupported encoding: '${encoding}' (only UTF-8 and ` + 'Base64 are supported)'); }

But it is not an issue, just an idea.

I'll trust you this is more javascriptonic 😄 Python doesn't have switch so I never think of using it, even in situations where it probably makes sense.

michalholasek · 2018-08-01T10:01:09Z

src/performRequest.js

+performRequest.getBodyAsBuffer = getBodyAsBuffer;
+performRequest.setContentLength = setContentLength;
+performRequest.createTransactionRes = createTransactionRes;
+performRequest.detectBodyEncoding = detectBodyEncoding;


When we are exposing private/internal APIs for testing purposes, I think it would be good idea to prefix the names with leading underscores __ - see this for inspiration 🙂

lol (inspiration). I did some research and dropped some thoughts on this to apiaryio/dredd-transactions#179 (comment). Let's continue there.

michalholasek

Great work 👍. Just a few comments, I'll leave it up to you whether you decide to make changes or not.

honzajavorek · 2018-08-02T13:39:44Z

@michalholasek Addressed your comments. Would you mind looking at the additional commits and saying 👍 or 👎 ?

michalholasek

👍

test: test whether Dredd handles binary responses

7d76fae

honzajavorek added Context: docs Epic: empty responses and binary files labels Jul 20, 2018

honzajavorek changed the title ~~Verify whether Dredd handles binary responses~~ Verify whether Dredd handles binary data Jul 20, 2018

honzajavorek added the in progress label Jul 20, 2018

honzajavorek added 2 commits July 23, 2018 12:45

test: test non-UTF-8 bytes

3307b02

Addressing #836?

test: test binary requests

5264ce6

This was referenced Jul 23, 2018

Gavel doesn't seem to be able to validate JSON Schema primitive types apiaryio/gavel.js#107

Closed

Dredd testing of APIary calls returning an image / PNG #87

Closed

honzajavorek added 2 commits July 24, 2018 17:28

test: mitigate an issue with Swagger schemas of binary format

b0106d1

https://github.com/apiaryio/fury-adapter-swagger/issues/193

docs: add docs on binary bodies

514d2ff

honzajavorek force-pushed the honzajavorek/binary branch 2 times, most recently from 7e6109b to 7e5bf31 Compare July 25, 2018 15:45

feat: support binary req/res bodies

d5414b3

Uses Base64 encoding as the serialization, which allows also non-JS hooks to set request/response bodies to a binary content. Close #617 Close #87 Close #836

honzajavorek force-pushed the honzajavorek/binary branch from 1b096f0 to d5414b3 Compare July 26, 2018 12:23

refactor: modularize and unit test the code

1965d9f

honzajavorek force-pushed the honzajavorek/binary branch from 73f458f to 1965d9f Compare July 27, 2018 17:05

honzajavorek mentioned this pull request Jul 30, 2018

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

Merged

honzajavorek added 2 commits July 30, 2018 16:50

test: unit test the performRequest itself

a281db9

docs: update docs with bodyEncoding and binary request bodies

c2ca0e0

This was referenced Jul 31, 2018

Find a way how to work with binary files in hooks #617

Closed

Support setting Request encoding for response #836

Closed

honzajavorek added 2 commits July 31, 2018 11:27

fix: upgrade dredd-transactions

32aa815

Normalize 'multipart/form-data' bodies for both API Blueprint and Swagger, avoid ignoring falsy values at multiple places (URI parameters, x-example)

test: update unit tests to new architecture

3d396f6

honzajavorek force-pushed the honzajavorek/binary branch from a62adda to 3d396f6 Compare July 31, 2018 13:49

honzajavorek requested a review from michalholasek July 31, 2018 14:10

honzajavorek removed the in progress label Aug 1, 2018

michalholasek reviewed Aug 1, 2018

View reviewed changes

michalholasek approved these changes Aug 1, 2018

View reviewed changes

michalholasek mentioned this pull request Aug 1, 2018

Add multipart/form-data unit tests apiaryio/dredd-transactions#179

Merged

honzajavorek mentioned this pull request Aug 1, 2018

Document why no-underscore-dangle is off #1100

Merged

3 tasks

honzajavorek added 4 commits August 2, 2018 14:57

chore: ignore docs build when linting JS code

be53b7d

refactor: make the function name clearer

1f6c251

style: use switch

ae58682

style: use underscores to declare private stuff in public API

59a383b

michalholasek approved these changes Aug 2, 2018

View reviewed changes

honzajavorek merged commit 36b4f3b into master Aug 2, 2018

honzajavorek deleted the honzajavorek/binary branch August 2, 2018 14:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify whether Dredd handles binary data #1094

Verify whether Dredd handles binary data #1094

honzajavorek commented Jul 20, 2018 •

edited

Loading

michalholasek Aug 1, 2018

honzajavorek Aug 1, 2018

michalholasek Aug 1, 2018 •

edited

Loading

honzajavorek Aug 1, 2018

honzajavorek Aug 2, 2018

michalholasek Aug 1, 2018

honzajavorek Aug 2, 2018

honzajavorek Aug 2, 2018

michalholasek Aug 1, 2018

honzajavorek Aug 1, 2018

michalholasek left a comment

honzajavorek commented Aug 2, 2018

michalholasek left a comment

Verify whether Dredd handles binary data #1094

Verify whether Dredd handles binary data #1094

Conversation

honzajavorek commented Jul 20, 2018 • edited Loading

🚀 Why this change?

🚧 To Do

📝 Related issues and Pull Requests

✅ What didn't I forget?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalholasek Aug 1, 2018 • edited Loading

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

michalholasek left a comment

Choose a reason for hiding this comment

honzajavorek commented Aug 2, 2018

michalholasek left a comment

Choose a reason for hiding this comment

honzajavorek commented Jul 20, 2018 •

edited

Loading

michalholasek Aug 1, 2018 •

edited

Loading