fix($http): only parse as JSON when opening/closing brackets match

[gkalpak]

Previously, due to weak JSON-detecting RegExp, string like [...} and
{...] would be considered JSON (even if they obviously aren't) and an
expection would be thrown while trying to parse them.

This commit makes sure the opening and closing brackets match. This
doesn't completely eliminate false positives (e.g. []{}[]), but does
help reduce them.

Closes #10349

Note:
I don't consider this to be a breaking change, but if someone expects their app to throw when receiving a response like '[...}' or '{...]', then this PR will break their app.

[googlebot]

CLAs look good, thanks!

[pkozlowski-opensource]

Just another small remark regarding the commit message: if we want to close #10349 with this commit (we could because it is technically fixing the reported issue) we should open a separate one to fix this once and fall all properly in 1.4

[gkalpak]

Good point on the function name (updated).

If you prefer leaving #10349 open, let me know so I update the commit message.
I thought we might want to add the once-and-forall fix (respecting content-type header) in this doc, under $http improvements.

[pkozlowski-opensource]

@gkalpak cool, thnx for updating the function name. Regarding the issue - let's leave the commit message as-is and let's add a note to the 1.4 planning doc. Could you do it?

[gkalpak]

I added it to the doc.

[jbedard]

Is there a reason for not using a single regex? Something such as /^(\[[\s\S]*\]|\{([^\{][\s\S]*)?\})$/ seems to work...

The two regex thing appears to have been added a long long time ago in c7913a4.

[lgalfaso]

Even when I am ok with the check itself, we should do better here, as we are first removing the security prefix (and not keeping the original) and then checking if the []{} match. The issue is that if they do not match and for whatever reason the original response had the security prefix, then we end up with something that is not the original response nor the JSON

[gkalpak]

@jbedard: You probably have to ask the one who introduced it. I just tried to stick as much as possible to the current approach.
For one, it's much more readable with the 2 smaller RegExps.
On the other hand, I am not familiar with JS RegExp internals, so not sure which approach would perform better (especially for longer strings).

@lgalfaso: Not removing the prefix, when no deserialization takes place, sounds reasonable. I will update the PR shortly.

[jbedard]

@gkalpak it's been that way since introduced. I assume because it read easier then combining them. But now that we have 4 REs and a map I'd say one RE is simpler...

[gkalpak]

@lgalfaso: Updated so that the original data is returned, when the response doesn't look like JSON.

@jbedard: It's 3 RegExp (and 2 of them are tiny); so if you ask me, I find the current approach simpler to read/understand. But if people think it is preferable to have one larger RegExp, it's fine by me :)

[jbedard]

Right, 3 RE not 4. But its the extra json mapping logic that makes it more complicated and error prone imo.

[pkozlowski-opensource]

So, I think we should merge it even if it is not perfect in the current form (no of RegExps etc.). It looks like we are going to drop JSON auto-detection in 1.4 so this will be a temporary change that improves the existing situation.

[gkalpak]

@pkozlowski-opensource: Like I said, I find the 3 small RegExps more readable than 1 bigger RegExp, but if you'd rather go with the 1 RegExp, let me know; it should be a quick change.

[pkozlowski-opensource]

Nah, it is fine - this code will go away in 1.4 anyway...

[pkozlowski-opensource]

@gkalpak one last thing - what do you think about adding a test showing that a JSON protection prefix is not removed when a response is not deserialized as JSON? This is one of the things that changes with the PR, right? Maybe it would be good to be explicit about it?

[gkalpak]

@pkozlowski-opensource: Good idea :) UPDATED
(I also rebased on master.)