[CAMEL-21199] Camel-jackson not properly marshalling 4-byte characters #15515

rnetuka · 2024-09-11T10:32:39Z

Issue: https://issues.apache.org/jira/browse/CAMEL-21199

Jackson doesn't handle 4-byte characters well. Marshalling a 4-byte Japanese kanji character results in two UTF-16 escapes to be written instead of the character itself. While this is ok for emoji an such, it's not for natural languages.

Jackson issue: FasterXML/jackson-core#223

Reproducer:

from("file:data?file-name=input.txt&noop=true")
    .log("${body}")
    .unmarshal().json(JsonLibrary.Jackson)
    .log("${body[0]['name']}")
    .marshal().json(JsonLibrary.Jackson, true)
    .log("${body}");

with the file input.txt containing:
[{"name": "システム𩸽"}]

Solution:
While it's an error in Jackson-core, it could be avoided by forcing Jackson to use WriterBasedJsonGenerator instead of UTF8JsonGenerator (see the original Jackson issue reproducer in the topmost comment)

github-actions · 2024-09-11T10:32:50Z

🌟 Thank you for your contribution to the Apache Camel project! 🌟

🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

First-time contributors require MANUAL approval for the GitHub Actions to run
You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot.
You can label PRs using build-all, build-dependents, skip-tests and test-dependents to fine-tune the checks executed by this PR.
Build and test logs are available in the Summary page. Only Apache Camel committers have access to the summary.
⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

davsclaus · 2024-09-11T11:04:52Z

This should be fixed in jackson instead of workaround in camel that can affect others.

rnetuka · 2024-09-11T11:10:35Z

@davsclaus I'll try to provide a fix for Jackson, but if you see the history, it's open from 2015 and there's not much effort to fix it from their side. Several PRs were even rejected as too dangerous for Jackson-core :(

davsclaus · 2024-09-11T11:13:08Z

Yes and if its dangerous for jackson, then why is it not dangerous for camel ?

What you can/should do is to create a JIRA ticket, and add a new option to the dataformat, where you can configure this with a boolean (default false) and then only use this new code if enabled. Then existing is not affected at all.

rnetuka · 2024-09-11T11:17:09Z

Yes and if its dangerous for jackson, then why is it not dangerous for camel ?

It was claimed dangerous by Jackson authors to mess with UTF8JsonGenerator (which containts the error, but is also fragile). This solution forces Jackson to use WriterBasedJsonGenerator inside, which seems to work fine.

What you can/should do is to create a JIRA ticket, and add a new option to the dataformat, where you can configure this with a boolean (default false) and then only use this new code if enabled. Then existing is not affected at all.

Seems like an overkill to me

davsclaus · 2024-09-11T11:19:00Z

Yes and if its dangerous for jackson, then why is it not dangerous for camel ?

It was claimed dangerous for Jackson to dabble with UTF8JsonGenerator which is erronous. This solution forces Jackson to use WriterBasedJsonGenerator inside, which seems to work fine.

What you can/should do is to create a JIRA ticket, and add a new option to the dataformat, where you can configure this with a boolean (default false) and then only use this new code if enabled. Then existing is not affected at all.

Seems like an overkill to me, but if you like, I can surely add the option

If its so non dangerous why has jackson not fixed it already. Please respect the large existing user-base of Camel that dont want hacks in commonly used components.

Also we need unit tests to go with that thanks.

rnetuka · 2024-09-11T11:19:34Z

If its so non dangerous why has jackson not fixed it already

Don't ask me. Ask them :)

rnetuka · 2024-09-11T11:26:19Z

@davsclaus I agree that it should be fixed in Jackson. Sadly, it hasn't been fixed for a long time and they don't consider it much of an issue. This hack as you call it just tries to overcome the error by invoking different string-writing mechanism (on their side) and things should be same as soon as calling Jackson marshal method ends.

I see no point in providing an option. It's hardly an option for Camel users. It's either fixed, or not.

Also we need unit tests to go with that thanks.

It cannot be unit-tested. It's integration issue.

If you don't want this fix (which I can understand), feel free to close it. I'll then try to create a fix for Jackson itself. But I doubt they will accept it...

davsclaus · 2024-09-11T11:42:01Z

@davsclaus I agree that it should be fixed in Jackson. Sadly, it hasn't been fixed for a long time and they don't consider it much of an issue. This hack as you call it just tries to overcome the error by invoking different string-writing mechanism (on their side) and things should be same as soon as calling Jackson marshal method ends.

I see no point in providing an option. It's hardly an option for Camel users. It's either fixed, or not.

I am sorry but we cannot risk affecting everyone else with some code change that Jackson itself does have out of the box, and its been outstanding since 2015. Would such a change cause unforseen changes to existing users, performance degration or what else.

Are there other reported tickets at Jackson than this single instance?

Also we need unit tests to go with that thanks.

It cannot be unit-tested. It's integration issue.

What we ask is that there is a junit test class in the PR that tests this code change.

If you don't want this fix (which I can understand), feel free to close it. I'll then try to create a fix for Jackson itself. But I doubt they will accept it...

Maybe take more time to investigate if there are other reports on Jackson. But in the mean time there is only a solution you can do and that is on the Camel side. But that requires more work which you then must step up and do.

And it would require a JIRA ticket in the Apache Camel JIRA at
https://issues.apache.org/jira/projects/CAMEL/

rnetuka · 2024-09-11T11:58:15Z

I am sorry but we cannot risk affecting everyone else with some code change that Jackson itself does have out of the box, and its been outstanding since 2015. Would such a change cause unforseen changes to existing users, performance degration or what else.

I'm not sure if we talk about the same thing here. Jackson offers two string generators - UTF8JsonGenerator (which we are using at the moment and which contains the error) and WriterBasedJsonGenerator (which seems to handle 4-byte characters fine). I don't see switching from one to another risky for Camel users, since they should behave exactly the same (not speaking about the error itself).

rnetuka · 2024-09-11T12:23:44Z

@davsclaus I've added a test for the fix. Also, the Camel JIRA issue has been created.

orpiske

Likely need to close the OutputStreamWriter ... waiting for clarifications.

...amel-jackson/src/main/java/org/apache/camel/component/jackson/AbstractJacksonDataFormat.java

orpiske · 2024-09-11T13:28:54Z

BTW, I am only reviewing the technicalities here. @davsclaus points are a lot more important than my notes.

.../src/test/java/org/apache/camel/component/jackson/JacksonMarshalMultibyteCharactersTest.java

rnetuka · 2024-09-11T14:07:08Z

@orpiske That's fine. Thak you very much for your review!

davsclaus · 2024-09-11T14:22:37Z

We use unicode escape to have foregin symbols in the camel source code.

See for example
https://github.com/apache/camel/blob/main/core/camel-core/src/test/java/org/apache/camel/component/file/FileConvertBodyToUTF8Test.java

Can you see if you can replace those to escaped values in this PR unit test.

davsclaus · 2024-09-12T13:58:00Z

.../src/test/java/org/apache/camel/component/jackson/JacksonMarshalMultibyteCharactersTest.java

+    @Test
+    public void testMarshal4ByteCharacter() throws Exception {
+        Map<String, Object> in = new HashMap<>();
+        in.put("name", "システム\uD867\uDE3D");


We use unicode escape to have foregin symbols in the camel source code.

See for example
https://github.com/apache/camel/blob/main/core/camel-core/src/test/java/org/apache/camel/component/file/FileConvertBodyToUTF8Test.java

Can you see if you can replace those to escaped values in this PR unit test.

davsclaus · 2024-09-12T15:41:06Z

Added option useWriter in this PR
#15538

davsclaus · 2024-09-13T06:14:41Z

On main branch we have the new option useWriter that can be enabled for this use-cases.

davsclaus · 2024-09-14T09:16:02Z

@rnetuka what is the status of this ?

rnetuka · 2024-09-18T13:40:05Z

PR for Jackson issue 223 has been merged: FasterXML/jackson-core#1335. There will be a new Jackson option to combine utf-16 surrogate characters into a one unicode char in their UTF8JsonGenerator.

I'm closing this PR and will create a replacement with the new option being supported.

github-actions bot added components camel-4 labels Sep 11, 2024

davsclaus marked this pull request as draft September 11, 2024 11:04

rnetuka force-pushed the csb-4781 branch 4 times, most recently from 5ffcae6 to 0bc57cc Compare September 11, 2024 12:22

orpiske requested changes Sep 11, 2024

View reviewed changes

...amel-jackson/src/main/java/org/apache/camel/component/jackson/AbstractJacksonDataFormat.java Outdated Show resolved Hide resolved

rnetuka force-pushed the csb-4781 branch from 0bc57cc to 160d6af Compare September 11, 2024 12:54

rnetuka requested a review from orpiske September 11, 2024 13:12

orpiske requested changes Sep 11, 2024

View reviewed changes

...amel-jackson/src/main/java/org/apache/camel/component/jackson/AbstractJacksonDataFormat.java Outdated Show resolved Hide resolved

rnetuka changed the title ~~Fix for Jackson marshalling 4-byte characters~~ [CAMEL-21199] Camel-jackson not properly marshalling 4-byte characters Sep 11, 2024

rnetuka force-pushed the csb-4781 branch from 160d6af to 79e2a5b Compare September 11, 2024 13:41

rnetuka requested a review from orpiske September 11, 2024 13:44

davsclaus reviewed Sep 11, 2024

View reviewed changes

.../src/test/java/org/apache/camel/component/jackson/JacksonMarshalMultibyteCharactersTest.java Outdated Show resolved Hide resolved

rnetuka force-pushed the csb-4781 branch from 79e2a5b to 04d3717 Compare September 11, 2024 14:02

Fix for Jackson marshalling 4-byte characters

eab5139

rnetuka force-pushed the csb-4781 branch from 04d3717 to eab5139 Compare September 11, 2024 14:03

rnetuka requested a review from davsclaus September 11, 2024 14:05

oscerd approved these changes Sep 11, 2024

View reviewed changes

davsclaus requested changes Sep 12, 2024

View reviewed changes

oscerd approved these changes Sep 14, 2024

View reviewed changes

rnetuka closed this Sep 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CAMEL-21199] Camel-jackson not properly marshalling 4-byte characters #15515

[CAMEL-21199] Camel-jackson not properly marshalling 4-byte characters #15515

rnetuka commented Sep 11, 2024 •

edited

Loading

github-actions bot commented Sep 11, 2024

davsclaus commented Sep 11, 2024

rnetuka commented Sep 11, 2024 •

edited

Loading

davsclaus commented Sep 11, 2024

rnetuka commented Sep 11, 2024 •

edited

Loading

davsclaus commented Sep 11, 2024

rnetuka commented Sep 11, 2024

rnetuka commented Sep 11, 2024 •

edited

Loading

davsclaus commented Sep 11, 2024

rnetuka commented Sep 11, 2024

rnetuka commented Sep 11, 2024

orpiske left a comment

orpiske commented Sep 11, 2024

rnetuka commented Sep 11, 2024

davsclaus commented Sep 11, 2024

davsclaus Sep 12, 2024

davsclaus commented Sep 12, 2024

davsclaus commented Sep 13, 2024

davsclaus commented Sep 14, 2024

rnetuka commented Sep 18, 2024

[CAMEL-21199] Camel-jackson not properly marshalling 4-byte characters #15515

[CAMEL-21199] Camel-jackson not properly marshalling 4-byte characters #15515

Conversation

rnetuka commented Sep 11, 2024 • edited Loading

github-actions bot commented Sep 11, 2024

davsclaus commented Sep 11, 2024

rnetuka commented Sep 11, 2024 • edited Loading

davsclaus commented Sep 11, 2024

rnetuka commented Sep 11, 2024 • edited Loading

davsclaus commented Sep 11, 2024

rnetuka commented Sep 11, 2024

rnetuka commented Sep 11, 2024 • edited Loading

davsclaus commented Sep 11, 2024

rnetuka commented Sep 11, 2024

rnetuka commented Sep 11, 2024

orpiske left a comment

Choose a reason for hiding this comment

orpiske commented Sep 11, 2024

rnetuka commented Sep 11, 2024

davsclaus commented Sep 11, 2024

davsclaus Sep 12, 2024

Choose a reason for hiding this comment

davsclaus commented Sep 12, 2024

davsclaus commented Sep 13, 2024

davsclaus commented Sep 14, 2024

rnetuka commented Sep 18, 2024

rnetuka commented Sep 11, 2024 •

edited

Loading

rnetuka commented Sep 11, 2024 •

edited

Loading

rnetuka commented Sep 11, 2024 •

edited

Loading

rnetuka commented Sep 11, 2024 •

edited

Loading