gstroup / apimocker

node.js module to run a simple http server for mock service responses.
MIT License
280 stars 81 forks source link

Incorrect documentation around templating #84

Open MarqueIV opened 6 years ago

MarqueIV commented 6 years ago

Hey! Really great tool. We depend on this heavily here at Citi for our mobile apps.

Just wanted to call out an issue we had that unfortunately was caused by incorrect documentation here in your 'readme.md' file. Specifically, it's around templating.

In your example, you stated that for this configuration...

 "template/:Name/:Number" :{
   "mockFile": "templateSample.json",
   "verbs":["get"],
   "enableTemplate": true
   "contentType":"application/json"
 }

With this template...

{
  "Name": "@Name",
  "Number": "@Number"
}

When you call /John/12345 you claim it will return this:

{
    "Name": "John"
    "Number": 12345
}

But we're actually getting this...

{
    "Name": "John"
    "Number": "12345"  <-- Note the quotes
}

For one of our service calls, we needed the non-quote-wrapped number.

The confusion came in when we opened up our template in a JSON editor and tried removing the quotes around '@number' but it started barking at us that we now had invalid JSON, so thinking that wasn't the correct approach, we abandoned it.

The problem is that actually is the correct way to format the template to have it return the placeholder without quotes. It just means that your template pre-filled-in is not valid JSON, and thus has to be edited with a text file. That's what tripped us up.

If you put a note/caveat/warning/whatever stating that if you need the non-quoted version, your template will no longer be valid JSON, but is still valid for Mocker (since it will become valid once merged) it would've gone a long way to helping people like us avoiding going down a rabbit hole trying to debug something we didn't actually have to debug.

That said, have you considered doing something like this for the tokens you don't want to have to wrap?

"#Number" or
"@@Number" or
"@Number@"
etc...

By implementing something like that (I'm personally partial to the '@@' myself) we can still keep our templates editable with a JSON editor without sacrificing output which is essential considering we run validation scripts against our JSON response files and now we have to explicitly exclude one.

It shouldn't be too difficult to implement with RegEx either. Just search for that version first, consuming the quotes as part of the search, then run the parsing again with the second flavor just as you do now. (You may even be able to format a single RegEx that handles both!)

Anyway, hope this helps. Again, keep up the really, really great work!

Mark

ferrerod commented 6 years ago

Quick solution is to implement the fix and issue a PR against the project with tests to show it works. I like your idea and so I will be issuing a PR with your proposal as well as some README fixes. Stay tuned.

MarqueIV commented 6 years ago

Yeah, I know what you mean about creating PR’s, and would have done exactly that, but for some reason our corporate network blocks github pushes. My guess is a security thing as not to let our code out in the wild. We are a bank after all! :)

Anyway, thanks for addressing this. I’m sure it will help someone else who was in my shoes earlier.

[Sent from my... wait... does anyone really read this part?!?]

On Dec 4, 2017, at 6:56 PM, David Ferrero notifications@github.com wrote:

Quick solution is to implement the fix and issue a PR against the project with tests to show it works. I like your idea and so I will be issuing a PR with your proposal as well as some README fixes. Stay tuned.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

gstroup commented 6 years ago

Thanks for pointing this out. And I’m glad to hear someone is using this thing! 😄 Is it a new problem? Was this ever working? Thanks.

ferrerod commented 6 years ago

Greg, Mark points out two main issues which I'll try to summarize:

Yes the template and templateSwitch features are working, but in order for a templated JSON file returned to contain a number value (not quoted), the template JSON file would also be unquoted and thus, not be a valid JSON file. The template mock file being invalid JSON is not an error per se, since the template mock could be another type of file such as HTML.

MarqueIV commented 6 years ago

Hi guys! Shared the update with the team here at Citi. Lots of happy people including management!

Quick thought... originally I was thinking @@tag was the best solution, but I've sort of had a change of heart and now think @#tag is clearer. That keeps the single '@' representing a normal replacement, wherever it's found, and then allows the edge case of a number where that's the only thing between the quotes. It also has the side-benefit of making the replacements no longer depending on searching for the numeric-only version first.

What do you guys think?

I've also added a comment to the PR about the text on the help page.

Finally, sorry again about not being able to create PR's here to GitHub. I'm trying to get an exception for things like this, but even though our team is agile, Citi's security is like trying to turn three oil tankers welded together.

MarqueIV commented 6 years ago

Crap! Damn iPhone! Didn’t mean to close that. Sorry. Disregard.

MarqueIV commented 6 years ago

Hey David, saw you updated the PR. I like the changes! Question though... did you see my comment above about using "@#tag" instead of "@@tag"? I think it reads more clear and separates intent. Now '@' simply means 'a replacement tag' whereas '#' means '...with the number format rules'

ferrerod commented 6 years ago

I could go either way. My only concern with using @#Number instead of @@Numbe is just another syntax to remember. @gstroup, what are your thoughts?

gstroup commented 6 years ago

Probably either syntax is OK. @# or @@. The only question I have is - what about a boolean value? If we use something like "@@isActive", then it would return an actual boolean value, right? Not the string "true"? For this reason, I think the @@ syntax might be better. @@ could apply to numbers, booleans, etc. What do you guys think?

MarqueIV commented 6 years ago

@gstroup, good point about the boolean. I hadn't considered that, but you are right, booleans aren't numbers but don't appear in quotes either.

Even more to the point, that's technically a json rule, not a 'response' rule. What about xml responses? HTML? CSV? I keep forgetting this tool is for mocking any APIs from a server, not just microservices returning json as we're using it.

To that effect, I think the rule should simply be this: '@' means 'replace with value' and '@@' means 'replace with value while also consuming any surrounding quotes'. Makes no mention about formats, etc. In other words, how that's used is completely up to the developer. (i.e. it can help with things like JSON, but technically isn't required if you don't care if your source template is still valid.)

Consider my mind changed. Let's stick with @@ as it is now.