Skip to content
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

Add Feature for generating JSONP compliant output #136

Closed
wants to merge 2 commits into from
Closed

Add Feature for generating JSONP compliant output #136

wants to merge 2 commits into from

Conversation

malaporte
Copy link

I recently got bitten by the fact that JSON in some (rare) cases cannot be parsed directly as JavaScript, typically when using JSONP to perform cross domain requests. Since I found no straightforward option to have Jackson generate JSON escaped in a way that would also work for JSONP, I've added a Feature that, when enabled, causes some special CharacterEscapes to be used to escape the Unicode newline characters \u2028 and \u2029. The output remains perfectly valid and equivalent JSON.

I tried to code that in a way that seems to agree with how the lib is built; do not hesitate to ask if you'd prefer it to be done differently (or none at all).

@cowtowncoder
Copy link
Member

Thank you for the contribution! I think this could be a useful feature, but I will have to think little bit more about what is the best way to expose it.

@GuiSim
Copy link

GuiSim commented May 19, 2016

@cowtowncoder Did you have time to think about this? We'd love this see this pull request merged.

@cowtowncoder
Copy link
Member

@GuiSim thank you for the ping. I think I would like to see just the JSONP escapes implementation, included under com.fasterxml.core.util, and leave out other parts of the handling. Javadoc link from CharacterEscapes should mention its existence.
I think that adding a separator JsonGenerator.Feature and handling adds complexity here (mostly since JsonGenerator unfortunately is base for all generators for all formats), whereas plugging in CharacterEscapes works robustly either via JsonGenerator, JsonFactory, ObjectMapper or ObjectWriter.

So while it may be slightly more cumbersome for users it seems like a reasonable compromise.

I was also trying to think of an idea of SimpleCharacterEscapes that could be configured for other uses, but also had a factory method for JSONP variant, but not quite sure how that'd work so that it'd be both general enough and performant.

I can actually merge this manually I think, and it would be in Jackson 2.8.

cowtowncoder added a commit that referenced this pull request May 29, 2016
@cowtowncoder
Copy link
Member

Ok, I added JsonpCharacterEscapes (under ..../util), but did not add feature quite yet.
The main reason is that I am starting to worry about interaction between other related features, such as allowing alternate quote character (mainly just single quote) and how to ensure that escapes needed are a superset.
But I hope that inclusion of the new escapes implementation simplifies use of this feature.

And I whoever idiot added these formatting characters in Javascript specification suffers a multi-year episode of explosive diarrhea for his/her sins -- what a messed-up misguided stupid no-good all-around pathetically sordid idea it was. Very much like fiasco with XML 1.1 and IBM-mandated linefeed additions. But I digress.

Thank you for the contribution!

@cowtowncoder
Copy link
Member

@malaporte @GuiSim One thing I started thinking about this one: are there other possibly problematic character that would make sense to add?

@malaporte
Copy link
Author

Thanks for merging JsonpCharacterEscapes, I'll be able to remove the dupe implementation from my own code 😁

I remember researching the issue a bit back then, and I didn't find any other exceptions like this. Also everywhere I've seen the JSONP & Unicode Newlines issue mentioned made no reference to other characters.

Also I've been using an in-house version of this very code for a while on a few moderately high traffic sites without noticing any other similar breakage (we only use JSONP with older versions of IE but hey). That's how we initially came across the unicode newline issue, so I feel pretty safe about the fix.

@cowtowncoder
Copy link
Member

@malaporte Ok sounds good! I hope to figure out what to do wrt possible need to combine aspects of character escaping with configurability of quote character (different pull request), but other than that my main concern isn't that code wouldn't work, but that maybe there'd be more exotic content. But based on your experiences it seems less likely to be the case.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants