Skip to content

Skip RS CTRL-CHAR to support JSON Text Sequence (RFC7464) #1414

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

Merged
merged 11 commits into from
Mar 31, 2025

Conversation

iifawzi
Copy link
Contributor

@iifawzi iifawzi commented Mar 24, 2025

Hello, This PR tries to resolve #633

This is my first PR for the project, so I could be missing a point. Feel free to guide me through points that might need to be added.

I didn't implement it in the data input parser as it seems like we're skipping the validation of WS

} else {
// 06-May-2016, tatu: Could verify validity of WS, but for now why bother.
// ... but line number is useful thingy
if (i == INT_CR || i == INT_LF) {
++_currInputRow;
}

so not sure if we need to have it now, the same way we do for other parsers

iifawzi added 2 commits March 25, 2025 11:46
Signed-off-by: Fawzi Essam <[email protected]>
Signed-off-by: Fawzi Essam <[email protected]>
@iifawzi iifawzi requested a review from pjfanning March 28, 2025 19:59
@cowtowncoder cowtowncoder added the cla-received PR already covered by CLA (optional label) label Mar 30, 2025
@@ -193,6 +193,12 @@ public enum Feature {
@Deprecated
ALLOW_UNQUOTED_CONTROL_CHARS(false),

/**
* @deprecated Use {@link com.fasterxml.jackson.core.json.JsonReadFeature#ALLOW_RS_CONTROL_CHAR} instead
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May look funny but JsonParser.Feature is to be removed from 3.0 yet is needed internally (all StreamReadFeatures and JsonReadFeatures must map back to one entry).
But we don't want users to use JsonParser.Feature any more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good to know about that to help take care of it next time.
Thanks for your collaboration on the PRs!

@cowtowncoder
Copy link
Member

cowtowncoder commented Mar 31, 2025

@iifawzi Implementation looks good; refactored things slightly wrt tests but functionally it was fine.
Did not remember that DataInput-backed parser skips WS validation; agree that we'll go with it for now.

The only concern I have is merging to 3.0/master -- that will be some fun. But I'll take care of it.

Thank you again for contributing this!

EDIT: merging wasn't too bad after all. :)

@cowtowncoder cowtowncoder merged commit 297a66b into FasterXML:2.19 Mar 31, 2025
7 checks passed
@iifawzi
Copy link
Contributor Author

iifawzi commented Mar 31, 2025

@iifawzi Implementation looks good; refactored things slightly wrt tests but functionally it was fine. Did not remember that DataInput-backed parser skips WS validation; agree that we'll go with it for now.

The only concern I have is merging to 3.0/master -- that will be some fun. But I'll take care of it.

Thank you again for contributing this!

EDIT: merging wasn't too bad after all. :)

Thanks for your collaboration @cowtowncoder, it helps and encourages contributing to the project.
Do you think the next move for this is to allow it in Content as we do for tabs? Otherwise, I might try to take a look on 1144

@cowtowncoder
Copy link
Member

@iifawzi thank you! It is great to hear that! And thank you for your contributions so far.

On RS in content: unless there's strong use case for allowing it (that is, some actually needing it for specific use), I'd probably not want to add support for in-content. #1144 would be good to try to figure out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-received PR already covered by CLA (optional label)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow skipping RS CTRL-CHAR to support JSON Text Sequences
3 participants