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

Set request Content-Type to application/json #8

Merged
merged 3 commits into from
Feb 28, 2019

Conversation

richie3366
Copy link
Contributor

In order to be compliant with the version 2 of the API (see https://api.random.org/json-rpc/2/fundamentals).
It is still compatible with the version 1 (see https://api.random.org/json-rpc/1/introduction).

In order to be compliant with the version 2 of the API.
It is still compatible with the version 1.
@willfrew
Copy link
Owner

Hey @richie3366 thanks for the PR! The change itself LGTM but we'll need to fix up the rpc tests.

Having had a quick look at the output, I think nock is probably pre-parsing the body as json given the changed content-type, resulting in requestBody being an object rather than the plain string. Could you try telling nock not to parse the body, or getting the raw request body another way?

@richie3366
Copy link
Contributor Author

richie3366 commented Feb 22, 2019

The only workaround I found (even after searching for a way to get the raw body with nock) was to use JSON.stringify in the Content-Length test, and to remove the JSON.parse calls in the next failing tests.

      var contentLength = Buffer.byteLength(JSON.stringify(requestBody), 'utf8');
      expect(request.headers['content-length']).to.equal(contentLength);

It works well, but I don't know if it is a legit & safe way to test it.
Please tell me what you think.

EDIT
Another way would be to only JSON.stringify here ; less changes to make.

    .reply(200, function(uri, reqBody) {
      // Store the request object & body to assert about later.
      request = this.req;
      requestBody = JSON.stringify(reqBody)
      return jsonRpcResponse;
    });

@willfrew
Copy link
Owner

@richie3366 Sorry for the delay in getting back to you - I just took a look through the nock codebase and, yeah, it seems that we can't easily get access to the raw body.

I think in this instance let's go with your first suggestion to JSON.stringify only in the Content-Length test then remove the other parse calls. It's a bit hacky since it's not really checking the length of what gets sent over the wire but at least it's only limited to that test.

Thanks for your help with this!

@richie3366
Copy link
Contributor Author

No worries about the delay, I'm using my fork until PR is merged.

I hope that it's all good to go.
Let me know if you have any question &/or suggestion.

test/rpc.test.js Outdated Show resolved Hide resolved
Copy link
Owner

@willfrew willfrew left a comment

Choose a reason for hiding this comment

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

Minor change but ready to merge after that!

Copy link
Owner

@willfrew willfrew left a comment

Choose a reason for hiding this comment

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

Legend, thanks very much for the change & iterations!

@willfrew willfrew merged commit 6488133 into willfrew:master Feb 28, 2019
@willfrew
Copy link
Owner

Pushed this out as v1.1.0

@richie3366 richie3366 deleted the patch-1 branch March 1, 2019 08:49
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.

2 participants