WIP: Support appendSeedData argument to be an array or array of arrays

[andreas-marschke]

Uses csv-stringify to render from one and two dimensional arrays to csv and can post that to the SeedData service.

Also fixes a bug in api-json.js where the responseBody would come down bare CSV data and crashes the request as api-json.js expects JSON only and results in a SyntaxError with Unexpected token , which is also fixed with the PR.

please review: @msolnit @nicjansma

[nicjansma]

Looks great!

[msolnit]

Is there a case where api-json.js is being used for a request that doesn't get back JSON?

[andreas-marschke]

Yes. CSV data for example when we appendSeedData or readSeedData

[msolnit]

Actually, I think that's a bug in SeedData.readSeedData(). It should be using apiStream instead of apiJson.

There's no concern for SeedData.appendSeedData(), because the response is always empty (204 No Content).

[andreas-marschke]

Well okay so we are wrapping appendSeedData into the stream and take end and the wrapper for the callback. That's just fine. Remember that you have 2 versions of append/read one stream one json. And one possibly doesn't work currently as it uses the api-json.js instead of api-stream.js.

Additionally using JSON.parse() and hoping to always retrieve JSON feels presumptious. What if the URL we are requesting this from is not an API endpoint, keep in mind the user may point it to another server not from us. What if the Server returns HTML because an Error happend internally and the Servlet was inaccessible? We should guard against these issues better.

[andreas-marschke]

But then readSeedData and ReadSeedDataStream are redundant?
Am 22.09.2015 07:35 schrieb "Matt Solnit" [email protected]:

In lib/util/api-json.js
#18 (comment):

@@ -63,7 +63,19 @@ function api(token, endpoint, method, requestBody, callback) {
if (responseBody.trim().length === 0) {
responseBody = null;
} else {

•                responseBody = JSON.parse(responseBody);
  

•                try {
  

Actually, I think that's a bug in SeedData.readSeedData(). It should be
using apiStream instead of apiJson.

There's no concern for SeedData.appendSeedData(), because the response is
always empty (204 No Content).

—
Reply to this email directly or view it on GitHub
https://github.com/SOASTA/repository.js/pull/18/files#r40054154.

[nicjansma]

I like switching to a single readSeedData that just reads a stream, instead of expecting it in JSON.

[msolnit]

OK, so I have a few thoughts on this.

1. SeedData.readSeedData definitely has a bug, and just as you found, if the response has multiple lines (which I never tested), you'll get SyntaxError: Unexpected string. This is bad.
2. Removing this method, and requiring the caller to provide a Stream.Writable, is more "pure" but inconvenient IMO.
3. Having to deal with special edge cases in api-json.js is also bad. We should let JSON.parse() do its thing without having to worry about "expected non-JSON" responses.

I propose that we keep SeedData.readSeedData, but instead of using apiJson(), it instead creates a temporary stream, calls apiStream(), and then "un-wraps" the content, returning it to the caller as a regular string. In fact, it can even use this.readSeedDataStream() with an in-between callback to do the un-wrapping.

Finally, we can add a note to the JSDoc, warning against doing this unless you know the response is small.

[andreas-marschke]

Sounds fair.