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

FIX #180: YAML Generator now quotes strings containing special chars #181

Merged
merged 2 commits into from
Mar 16, 2020

Conversation

trohrberg
Copy link
Contributor

@trohrberg trohrberg commented Mar 12, 2020

(fix for #180)

When using the YAMLGenerator in the MINIMIZE_QUOTES mode, the generator serializes strings in plain style, i.e. without quoting them. However, as stated in the YAML Spec [1] such plain style strings cannot contain some special characters such as ':', ',', and others.

[1] https://yaml.org/spec/1.2/spec.html#id2788859

As with the serialization of boolean-like strings, the YAMLGenerator should be adjusted to always quote strings containing those special characters.

I will shortly try to provide a pull request to fix this issue.

…al chars

When using the YAMLGenerator in the MINIMIZE_QUOTES mode, the generator serializes strings in plain style, i.e. without quoting them. However, as stated in the YAML Spec [1] such plain style strings cannot contain some special characters such as ':', ',', and others.

[1] https://yaml.org/spec/1.2/spec.html#id2788859

As with the serialization of boolean-like strings, the YAMLGenerator should be adjusted to always quote strings containing those special characters.

I will shortly try to provide a pull request to fix this issue.
@trohrberg
Copy link
Contributor Author

Hello everybody,

unfortunately, the build is breaking for my PR due to test failues. However, I had exactly the same test failues locally already when just checking out the 2.11 branch. Does anyone know if that is expected behavior, i.e. that the 2.11 branch is currently broken?
At least for the testEmptyStream(com.fasterxml.jackson.dataformat.csv.NullReader122Test) the failure makes totally sense to me as it expects a certain exception message which is not throw with that message in the jackson-databind module version 2.11.0-SNAPSHOT.

I would very much appreciate any help or hint.

Thank you everyone!

@cowtowncoder
Copy link
Member

First of all: thank you for reporting the issue, proposing a fix (and basing it on 2.11 which is the right branch).

My only concern is that the implementation os bit inefficient (although I don't have a benchmark to show it, and granted YAML handling is not super efficient to begin with).
But I think I can change that easily after merge, to either scan over characters, or maybe even use a java.util.Pattern for scanning.

But just one more thing before merging: unless I have asked and received CLA (apologies if so), I'd need one from:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

(or, Corporate CLA which is in same repo).
Only on CLA needed for all contributions, so this is a one-time thing.
Usual method is to print it, fill, sign & scan, email to info at fasterxml dot com.

Looking forward to merging!

@cowtowncoder cowtowncoder added the yaml Issue related to YAML format backend label Mar 13, 2020
@trohrberg
Copy link
Contributor Author

Hey cowtowncoder,

thank you so much for your response. I signed the CLA and sent it to the given address.

Concerning my PR: I can understand that you have doubts about the performance of my contribution as it scans the strings to be serialized with the String.contains() method. Unfortunately, I do not completely understand how the use of Java.util.Pattern could make that more performant. If you tell me a bit more about your idea, I can of course adjust my PR so that you don't have additional work with it.

I am absolutely fine with further discussing my solution and improve it before you merge it.

I am looking forward to contribute the well-known FasterXML/jackson libraries.

Best regards
Timo

@cowtowncoder
Copy link
Member

@trohrberg No problem wrt PR; I can play with that aspect after merging -- I just mentioned it as a sidenote (I often make small tweaks but want to let author know). :)

I'll see when the CLA comes to the account; if not (not sure if gmail account has issues), I can give you my personal address. But will wait for a bit.

Thank you again for quick follow up & contribution!

@cowtowncoder
Copy link
Member

@trohrberg For some reason, I don't see your email to info@fasterxml.com. Could you try to send it again? You can also cc tsaloranta at gmail.com (my personal address), just to make sure it gets to somewhere where I can access it.

@cowtowncoder cowtowncoder merged commit c41a621 into FasterXML:2.11 Mar 16, 2020
@trohrberg
Copy link
Contributor Author

@cowtowncoder - No problem, I sent the CLA again to info@fasterxml.com and also to your personal address. Please check if you received it this time. I sometimes have problems delivering eMails to some mail servers from my self-hosted mail server.

And thank you for merging my PR already. I am looking forward to a new release of fasterxml/jackson-dataformats-text.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
yaml Issue related to YAML format backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants