sideshowcoder / canned

Server to respond with fake API responses, by using a directory of files for finding out what to say!
213 stars 46 forks source link

Using more than one custom header does not work #89

Closed ratschance closed 7 years ago

ratschance commented 8 years ago

Having multiple custom headers, such as the one from the readme:

//! customHeaders: [{"MyCustomHeaderName": "MyCustomHeaderValue"}, {"SecondHeaderName": "SecondHeaderValue"}] 

returns the error:

Invalid file header format try //! statusCode: 201

I believe this is due to the code on line 130 of canned.js:

var content = line.split(',').map(function (s) {

which splits the line on commas but should not actually be splitting on the comma located in the JSON string for the custom headers.

sideshowcoder commented 8 years ago

Hi sorry for the late response, I was responding before from by phone but github didn't like it it seems so here it goes:

yes this is indeed right this is a bug and I think you hit the nail on the head that splitting at commas does not work in this case. The problem I see right now is that we can't simply not split, and trying to figure out if we are already in a JSON string starts to get into parsing no longer possible with a simple regex. Not sure how to resolve this right now I have to think about it, if you have an Idea I'm always happy to accept PRs or suggestions. Thanks again for the report!

wadtech commented 8 years ago

Could it instead be switched to accept a single custom header per line?

e.g.

//! statusCode 200
//! customHeader: {"My-Header": "My-Value"}
//! customHeader: {"My-Other-Header": "My-Other-Value"}

The parsing and splitting would then still work, but when you match a customHeader, the key value pair gets added to a customHeaders object.

sideshowcoder commented 8 years ago

@wadtech that makes sense, guess we need a testcase for that.

wadtech commented 8 years ago

Ok, I'll put one together when I get a chance :-)

mazoni commented 7 years ago

I'm having the same problem. Do we have a work around for this? Thank you in advance.

sideshowcoder commented 7 years ago

@mazoni sorry I currently am not actively working on this, but am very open to accepting PRs for getting this fixed and helping as much as I can to get this pushed through.

mazoni commented 7 years ago

There it goes @sideshowcoder, if you could take a look at it. I tested locally and wrote some specs too. Sorry if something don't match the used pattern.

I've wrote it the way @wadtech suggested.

sideshowcoder commented 7 years ago

Thansk so much @mazoni I left some comments, mainly some suggestions around the style awesome work!

mazoni commented 7 years ago

@sideshowcoder , I've made the style change you mentioned. It was really looking weird. Could you merge the pull request? I'm willing to use this as soon as we have it merged. After merge does it go directly to NPM?

sideshowcoder commented 7 years ago

Just merged thanks for that, I will release to NPM soon it does not automatically go there but you can install from github for now. I'll created an issue to track the release, will do probably tomorrow morning.