New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Console JSON ignore comments #7964

Open
mbniebergall opened this Issue Oct 28, 2015 · 12 comments

Comments

Projects
None yet
3 participants
@mbniebergall

mbniebergall commented Oct 28, 2015

The Console JSON tab does not ignore comments (ex: /* comment here */). One or many comments before or after valid JSON causes the JSON tab to not show. This appears to have worked prior to commit 1569514.

Firebug 2.0.13 on Windows 10

For example, if the response is:

{"edible":["apple","banana","carrot"]}

Then the JSON tab shows correctly.

If the response is:

/* comment */{"edible":["apple","banana","carrot"]}

Or:

/* comment A */{"edible":["apple","banana","carrot"]}/* comment B */

Or:

/* comment A *//* Comment B */{"edible":["apple","banana","carrot"]}

Then the JSON tab does not show.

I would expect one or many comment(s) to be ignored by the JSON parsing.

Comment in response causing JSON tab to not show:

jsonresponseinvalid-response

No comment response:

jsonresponsevalid-response

No comment JSON:

jsonresponsevalid-json

@fflorent

This comment has been minimized.

Show comment
Hide comment
@fflorent

fflorent Nov 1, 2015

Member

Hello and thanks for the report!

I would say these two lines are the culprit, as they don't handle the case when the first character is a comment:

if (first !== "[" && first !== "{" && isNaN(first) && first !== '"')
return null;

Florent

Member

fflorent commented Nov 1, 2015

Hello and thanks for the report!

I would say these two lines are the culprit, as they don't handle the case when the first character is a comment:

if (first !== "[" && first !== "{" && isNaN(first) && first !== '"')
return null;

Florent

@fflorent fflorent self-assigned this Nov 1, 2015

@fflorent fflorent added the net label Nov 1, 2015

@fflorent

This comment has been minimized.

Show comment
Hide comment
@fflorent

fflorent Nov 2, 2015

Member

I wonder if we couldn't be more generic with this piece of code (strip any comment, not just comment of the form described in the regexp):

regex = /^\s*\/\*-secure-([\s\S]*)\*\/\s*$/;
matches = regex.exec(jsonString);
if (matches)
{
jsonString = matches[1];
if (jsonString[0] === "\\" && jsonString[1] === "n")
jsonString = jsonString.substr(2);
if (jsonString[jsonString.length-2] === "\\" && jsonString[jsonString.length-1] === "n")
jsonString = jsonString.substr(0, jsonString.length-2);
}

But I don't know if we want to change the code now (as the tests don't run anymore and we may release firebug.next soon). Ping @janodvarko and @simonlindholm

Florent

Member

fflorent commented Nov 2, 2015

I wonder if we couldn't be more generic with this piece of code (strip any comment, not just comment of the form described in the regexp):

regex = /^\s*\/\*-secure-([\s\S]*)\*\/\s*$/;
matches = regex.exec(jsonString);
if (matches)
{
jsonString = matches[1];
if (jsonString[0] === "\\" && jsonString[1] === "n")
jsonString = jsonString.substr(2);
if (jsonString[jsonString.length-2] === "\\" && jsonString[jsonString.length-1] === "n")
jsonString = jsonString.substr(0, jsonString.length-2);
}

But I don't know if we want to change the code now (as the tests don't run anymore and we may release firebug.next soon). Ping @janodvarko and @simonlindholm

Florent

@fflorent fflorent removed their assignment Nov 2, 2015

@simonlindholm

This comment has been minimized.

Show comment
Hide comment
@simonlindholm

simonlindholm Nov 2, 2015

Member

Nah, that's for JSON contained in comments. Your first comment sounds spot on - just adding && first !== "/" to the conditional should fix it and be very very safe. (It's just an optimization, I was worried that calling pseudoJsonToJson too often would be slow.)

Member

simonlindholm commented Nov 2, 2015

Nah, that's for JSON contained in comments. Your first comment sounds spot on - just adding && first !== "/" to the conditional should fix it and be very very safe. (It's just an optimization, I was worried that calling pseudoJsonToJson too often would be slow.)

@fflorent

This comment has been minimized.

Show comment
Hide comment
@fflorent

fflorent Nov 2, 2015

Member

Ah right!

Your first comment sounds spot on - just adding && first !== "/" to the conditional should fix it and be very very safe.

That sounds wise :)

Florent

Member

fflorent commented Nov 2, 2015

Ah right!

Your first comment sounds spot on - just adding && first !== "/" to the conditional should fix it and be very very safe.

That sounds wise :)

Florent

@simonlindholm

This comment has been minimized.

Show comment
Hide comment
@simonlindholm

simonlindholm Nov 2, 2015

Member

Oh, I should add: another reason we don't want to modify that code to handle general comments is that we absolutely don't want to touch comments within strings in the JSON.

And I don't mind patching this even with firebug.next coming; it's a nice self-contained library function, and we or someone might want to reuse it for something later, so I'd say that all bug fixes are welcome. For larger feature additions, I would agree.

Member

simonlindholm commented Nov 2, 2015

Oh, I should add: another reason we don't want to modify that code to handle general comments is that we absolutely don't want to touch comments within strings in the JSON.

And I don't mind patching this even with firebug.next coming; it's a nice self-contained library function, and we or someone might want to reuse it for something later, so I'd say that all bug fixes are welcome. For larger feature additions, I would agree.

@fflorent

This comment has been minimized.

Show comment
Hide comment
@fflorent

fflorent Nov 2, 2015

Member

so I'd say that all bug fixes are welcome.

I was thinking the same (would be nice to have in the DevTools if we can call it at request details visualizations ).

Florent

Member

fflorent commented Nov 2, 2015

so I'd say that all bug fixes are welcome.

I was thinking the same (would be nice to have in the DevTools if we can call it at request details visualizations ).

Florent

@fflorent

This comment has been minimized.

Show comment
Hide comment
@fflorent

fflorent Nov 2, 2015

Member

Commit above. Is that OK for you @simonlindholm?

Florent

Member

fflorent commented Nov 2, 2015

Commit above. Is that OK for you @simonlindholm?

Florent

@simonlindholm

This comment has been minimized.

Show comment
Hide comment
@simonlindholm

simonlindholm Nov 2, 2015

Member

I'd say you don't need to change the comment (or change "or" -> "and" if you do). But LGTM assuming it works.

Member

simonlindholm commented Nov 2, 2015

I'd say you don't need to change the comment (or change "or" -> "and" if you do). But LGTM assuming it works.

@fflorent

This comment has been minimized.

Show comment
Hide comment
@fflorent

fflorent Nov 2, 2015

Member

I prefer keeping the comment (as it would be wrong otherwise). Otherwise, I made the change.

Florent

Member

fflorent commented Nov 2, 2015

I prefer keeping the comment (as it would be wrong otherwise). Otherwise, I made the change.

Florent

@fflorent

This comment has been minimized.

Show comment
Hide comment
@mbniebergall

This comment has been minimized.

Show comment
Hide comment
@mbniebergall

mbniebergall Nov 3, 2015

Confirmed the fix. Seeing JSON tab now with comments before and/or after.

For example, the following now works with the JSON tab showing:

/* comment */{"edible":["apple","banana","carrot"]}
/* comment *//* other comment */{"edible":["apple","banana","carrot"]}
/* comment *//* other comment */{"edible":["apple","banana","carrot"]}/* end comments */
{"edible":["apple","banana","carrot"]}/* end comments */

Thank you @fflorent and @simonlindholm !

mbniebergall commented Nov 3, 2015

Confirmed the fix. Seeing JSON tab now with comments before and/or after.

For example, the following now works with the JSON tab showing:

/* comment */{"edible":["apple","banana","carrot"]}
/* comment *//* other comment */{"edible":["apple","banana","carrot"]}
/* comment *//* other comment */{"edible":["apple","banana","carrot"]}/* end comments */
{"edible":["apple","banana","carrot"]}/* end comments */

Thank you @fflorent and @simonlindholm !

@fflorent

This comment has been minimized.

Show comment
Hide comment
@fflorent

fflorent Nov 3, 2015

Member

Merged, thanks for the feedback @mbniebergall !

Florent

Member

fflorent commented Nov 3, 2015

Merged, thanks for the feedback @mbniebergall !

Florent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment