cnguy / kayn

superagent-inspired Node.js lib (w/ **some** TypeScript support) for accessing Riot's League of Legend's API (discord: cnguy#3614)
MIT License
134 stars 34 forks source link

JSON.parse() throws errors when response contains an array #45

Closed srcpack closed 6 years ago

srcpack commented 6 years ago

I haven't done much testing but some endpoints seem to respond with arrays which cause JSON.parse() to fail. For example: ChampionMastery.list returns a stringified array that parses with no issue. TournamentStub.create returns a normal array that throws errors in JSON.parse().

I'm not sure if it's Riots fault or something in the lib causing it but I'm using this workaround for now. I haven't tested it with every endpoint but checking for array and calling stringify on it seems to be a working bandaid until a permanent solution is found.

https://github.com/cnguy/kayn/blob/c7fb8edb2f41cb3709fba7baa7f9d74e3455bc43/lib/RequestClient/Request.js#L101

let res = await this.limiter.executing({
...
try { 
  if (Array.isArray(res)) {
    res = JSON.stringify(res)
   }
   const blob = JSON.parse(res)
...
}
cnguy commented 6 years ago

Thank you! This is a good catch -- I'll look into it later. Maybe it specifically relates to the POST/PUT endpoints (which all happen to be in Tournament/TournamentStub), which means that I caused some bugs in the forked rate limiter I'm using (kayn uses a temporary, forked version of RiotRateLimiter-node that has PUT/POST functionality).

I remember TournamentStub.create wasn't working for me (Riot API returned 500s no matter what) -- hopefully I can test it tomorrow.

srcpack commented 6 years ago

I remember TournamentStub.create wasn't working for me (Riot API returned 500s no matter what) -- hopefully I can test it tomorrow.

Ya I ran into that same problem. The docs for TournamentStub claim the TournamentCodeParameters are optional but they are required and the server returns 500 if they are missing for some reason. I posted a topic: https://discussion.developer.riotgames.com/questions/4719/error-in-tournament-stubv3codes-docs.html and Tuxedo seemed to confirm it's a bug on Discord.

cnguy commented 6 years ago

@srcpack

Pretty sure I've found the issue.

https://github.com/cnguy/RiotRateLimiter-node/blob/c431f19cff94e6c70ba928b71386628ef67dfb83/dist/RiotRateLimiter/index.js#L117

Basically, I (accidentally) set the request options to include json: true, which converts the body property of the response to json automatically [see bottom], and so in kayn we attempt to parse an already parsed object and so an error naturally occurs.

Removing json: true from that module would be an easy fix (and proper, too, as the GET requests return unparsed body's as well so the user can do whatever they want with it).

=== https://github.com/request/request json - sets body to JSON representation of value and adds Content-type: application/json header. Additionally, parses the response body as JSON.

cnguy commented 6 years ago

@srcpack

Since this project is using a temporary, forked version of RiotRateLimiter-node, you won't be able to npm upgrade blahblahblah as of now.

Here's the commit though: https://github.com/cnguy/RiotRateLimiter-node/commit/ce96b5d9342ae1cb7d181e6bde78f2f6a67cf746

Try refreshing the module (rm -rf node_modules && {npm install | yarn}). Let me know if it works!

srcpack commented 6 years ago

Seems to be throwing errors on tournament-stub outgoing requests. This could be on my end though, I'm at work I'll test more when I'm home on my machine.

creating sync rate limiter for  americas /lol/tournament-stub/v3/providers
(STDERR) _http_outgoing.js:645
(STDERR)     throw new TypeError('First argument must be a string or Buffer');
(STDERR)     ^
(STDERR)
(STDERR) TypeError: First argument must be a string or Buffer
(STDERR)     at write_ (_http_outgoing.js:645:11)
(STDERR)     at ClientRequest.write (_http_outgoing.js:620:10)
(STDERR)     at Request.write (D:\Dev\Projects\riot_test\node_modules\request\request.js:1501:27)
(STDERR)     at end (D:\Dev\Projects\riot_test\node_modules\request\request.js:546:18)
(STDERR)     at Immediate._onImmediate (D:\Dev\Projects\riot_test\node_modules\request\request.js:575:7)
(STDERR)     at runCallback (timers.js:789:20)
(STDERR)     at tryOnImmediate (timers.js:751:5)
(STDERR)     at processImmediate [as _immediateCallback] (timers.js:722:5)
const masterylist = await kayn.ChampionMastery.list(25130483); // Good
const provider = await kayn.TournamentStub.registerProviderData(REGIONS.NORTH_AMERICA, 'https://localhost/cb'); // Error
const tournament = await kayn.TournamentStub.register(948, 'kappa'); // Error
const codes = await kayn.TournamentStub.create(5428, {
      "mapType": "SUMMONERS_RIFT",
      "metadata": "Kappa",
      "pickType": "TOURNAMENT_DRAFT",
      "spectatorType": "NONE",
      "teamSize": 5
}).query({count: 5}); // Error
cnguy commented 6 years ago

No problem. I'll test it manually when I'm at home later, too. :) I don't remember having to deal with that specific error before.

srcpack commented 6 years ago

I think you are correct about removing json: true to avoid the double parse. However when you remove it options['body'] = body isn't serialized by request as a json object and "Must be a Buffer, String or ReadStream". So you either need to pass in body as a string into execute or stringify the json body yourself (the simpler option imo) in if (method === 'POST' || method === 'PUT') {...}. Doing this has solved all my parsing errors.

=== https://github.com/request/request#requestoptions-callback body - entity body for PATCH, POST and PUT requests. Must be a Buffer, String or ReadStream. If json is true, then body must be a JSON-serializable object.

cnguy commented 6 years ago

It should work now. :) https://github.com/cnguy/kayn/commit/380f5db67dca8ff93335c8b2aa3bf2483f21aa2e

The fork dependency from the git master branch URL has been moved to the npm registry, so I won't be accidentally breaking any projects now haha.

This fix is in kayn@v0.8.4 so make sure to upgrade! Let me know if it works, and if it does, feel free to close the issue.

edit: Actually, it might be better to leave this issue up for a while just in case anyone else stumbles upon problems with the old versions or problems with .create like I did before you told me about it.

The code below should work cleanly (and the ChampionList example above should work with the right API key too).


 const provider = await kayn.TournamentStub.registerProviderData(
    REGIONS.NORTH_AMERICA,
    'https://localhost/cb',
)
const tournament = await kayn.TournamentStub.register(provider, 'kappa')
const codes = await kayn.TournamentStub.create(tournament, {
    mapType: 'SUMMONERS_RIFT',
    metadata: 'Kappa',
    pickType: 'TOURNAMENT_DRAFT',
    spectatorType: 'NONE',
    teamSize: 5,
}).query({ count: 5 })
``
srcpack commented 6 years ago

It works. I'll leave the issue open and leave it you to close when you're ready. Thanks.