Add support to optionally allow surrogate pair entities (#165) #174

Magmaruss · 2023-06-18T16:06:04Z

This feature is an answer for the issue #165. It's extending the XML reader functionality with a new configuration option (default disabled) to support reading XML with unicode characters written using surrogate pair (UTF-16 encoding). This feature is very important to correctly read XML from the legacy services which produces responses this way and there is no way to change the behavior of them.

Example:
The external service produces a response with a unicode character written using UCS2:

<response>
    <value>Merry Christmas &#55356;&#57221;</value>
</response>

Default behavior of XMLStreamReader is throwing an exception with message:

Illegal character entity: expansion character (code 0xd83c

After setting the P_RESOLVE_ENTITY_SURROGATE_PAIRS parameter to true it will be readed with no exception converting the surrogate pair entities to the appropriate unicode (🎅)

cowtowncoder · 2023-06-19T01:34:12Z

First of all: thank you for contributing this feature.

Second: could you add a description that explains why this is needed, what is the use case (ideally with a simple example) and so on?

Magmaruss · 2023-06-20T21:22:40Z

Description added.

src/main/java/com/ctc/wstx/api/ReaderConfig.java

src/main/java/com/ctc/wstx/api/WstxInputProperties.java

src/main/java/com/ctc/wstx/sr/StreamScanner.java

cowtowncoder · 2023-11-03T23:22:35Z

src/test/java/org/codehaus/stax/test/stream/TestEntityRead.java

+            try {
+                streamThrough(sr);
+                fail("Expected an exception for invalid surrogate pair");
+            } catch (Exception e) { }


Would be better to catch specific Exception subtype and check exception message that it fails for expected reason (just match a phrase, no need to match exact message).

cowtowncoder · 2023-11-04T00:10:40Z

First of all: apologies for ultra-slow follow up here. But I am ready to get this merged.

One minor legal thing first: if we haven't yet gotten a CLA, we'd need one from here (it's under Jackson project, that's fine):

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

and it's easiest to print, fill & sign, scan/photo, email to info at fasterxml dot com.
(for project you can add Woodstox or FasterXML/Woodstox).

This is only needed once for all contributions to Woodstox, and once I get it, I can proceed with merge.
I added some notes already; just need to re-read main logic and should be good to merge.

Thank you again for this contribution!

EDIT: I gave wrong email address -- CLA actually needs to be sent to info at fasterxml dot com instead; cla email alias did not yet exist unlike I thought.

cowtowncoder · 2023-11-04T00:15:55Z

src/main/java/com/ctc/wstx/sr/StreamScanner.java

+                     * If pair value is not in range of low surrogate values, then throw an error
+                     */
+                    if (pairValue < 0xDC00 || pairValue > 0xDFFF) {
+                        reportNoSurrogatePair(value);


Would be better if we reported both leading surrogate and second non-surrogate entity value?

cowtowncoder · 2023-11-04T00:20:25Z

src/main/java/com/ctc/wstx/sr/StreamScanner.java

-                    } else {
-                        mInputPtr = ptr; // so error points to correct char
-                        throwUnexpectedChar(c, "; expected a hex digit (0-9a-fA-F).");
+            boolean isValueHighSurrogate = false;


Ok, so, I assume this works, but is quite hard to follow. What I'd suggest is to split code in such a way that the original decoding for the first entity is within initial method; and the rest of processing in separate method, only called if:

Entity surrogate pair decoding is enabled

First entity is valid first surrogate

This probably leads to slight duplication of code (decoding loop) but allows removing the outer loop since each method only decodes one entity.
And if duplicate looks too ugly decoding loops can be extracted into separate method too.

I'll try to refactor this part in my free time (maybe on the weekend), but my first assumptions was to write functional code with minimal changes in the class architecture, to not conflict your rules and coding style. I haven't seen before any contributor guide, so I wanted to be careful and do this most safety way. Please give me a time, because I don't have much free time after hours

@Magmaruss understood! I could try my hand at change, but was worried I might get some parts wrong easily.
And my suggestion was not meant as critique; I much appreciate the contribution and it follows all reasonable expectations (esp. in absence of much documentation on what is expected of contributions).

cowtowncoder · 2023-11-11T01:10:15Z

@Magmaruss quick note: I gave a wrong email address to send CLA to -- CLA actually needs to be sent to info at fasterxml dot com instead; cla email alias did not yet exist unlike I thought.
So just in case you already sent one, could you please re-send it. Apologies for mix-up.

cowtowncoder · 2023-11-13T02:19:32Z

NOTE: CLA received; can now focus on finalizing PR.

src/main/java/com/ctc/wstx/api/WstxInputProperties.java

cowtowncoder · 2023-11-13T02:31:58Z

@Magmaruss I hope you don't mind my applying changes I suggested here, wrt naming and adding @since tags. I am open to different naming too if you strongly prefer yet different naming.

Beyond this, I think it'd be good to refactor decoding loop, possibly the way I suggested (but not necessarily, there may well be other ways to improve readability).

Also thank you for getting CLA; I am hoping we can get this PR merged soon!

src/main/java/com/ctc/wstx/sr/StreamScanner.java

cowtowncoder · 2023-12-07T04:59:04Z

src/test/java/org/codehaus/stax/test/stream/TestEntityRead.java

+     * This unit test checks that resolving of surrogate pairs works
+     * as expected, including different ways of writing entities
+     */
+    public void testValidSurrogatePairEntities()


Shouldn't there be a test or two for invalid ones too?

Test for invalid ones is already there - testInvalidSurrogatePairEntities method: https://github.com/FasterXML/woodstox/pull/174/files/eb5189d107d911709774c79e1ed445865567d7ff#diff-50c2abc037511dda94147cf808bde7666e630ad76d08151e92ff86fb56b6f17aR466

Right, I meant the case when new option was disabled, as per your other comment.

cowtowncoder · 2023-12-07T05:05:34Z

@Magmaruss Ok, I did some more tweaks and I think things look good.

About the only thing I think that is needed would be a test or two for invalid cases; one for catching surrogate pairs without enabling their use; and another one for just invalid pairings with handling enabled.
I hope to get this merged soon, and then soon after release 6.6.0.

Thank you again for contributing this!

Magmaruss · 2023-12-10T13:12:05Z

I can write simple test for disabled option, but for now when disabled it's throwing "Illegal character entity..." - default behavior before introducing the feature. Should I replace this exception with something special when first valid high surrogate detected or leave the default behavior and add test only?

cowtowncoder · 2023-12-11T01:38:38Z

I can write simple test for disabled option, but for now when disabled it's throwing "Illegal character entity..." - default behavior before introducing the feature. Should I replace this exception with something special when first valid high surrogate detected or leave the default behavior and add test only?

Either way works for me: obviously improved error messages are always a plus, but if you want to limit the scope of changes & extra work, verifying default behavior is fine also.

cowtowncoder · 2024-01-15T02:32:54Z

Hi @Magmaruss! I was wondering if you might be able to add basic unit test? No rush if you don't have time -- I just hope to be able to merge this in, get 6.6.0 released, and then be able to do #134 (increase min JDK baseline to 8 for Woodstox).

Magmaruss · 2024-01-15T09:43:22Z

Yes, sorry for that, but i am having a very difficult time. I will try to deliver it tonight or tomorrow at the latest

cowtowncoder · 2024-01-15T18:19:00Z

@Magmaruss Sorry did not mean to add pressure at all. I appreciate your help here and please take your time -- I don't want this to be another stress point.

Magmaruss · 2024-01-15T21:50:21Z

Test case for disabled option added. Please tell if you need something more.

cowtowncoder

Ok good, let's merge this!

Support for entity surrogate pairs (FasterXML#165)

0c71536

gnodet reviewed Jul 13, 2023

View reviewed changes

src/main/java/com/ctc/wstx/api/ReaderConfig.java Outdated Show resolved Hide resolved

cowtowncoder reviewed Nov 3, 2023

View reviewed changes

src/main/java/com/ctc/wstx/api/WstxInputProperties.java Show resolved Hide resolved

cowtowncoder reviewed Nov 3, 2023

View reviewed changes

src/main/java/com/ctc/wstx/sr/StreamScanner.java Outdated Show resolved Hide resolved

cowtowncoder reviewed Nov 3, 2023

View reviewed changes

cowtowncoder reviewed Nov 4, 2023

View reviewed changes

cowtowncoder mentioned this pull request Nov 11, 2023

Fix parser when not replacing entities and treating char references as entities #176

Merged

cowtowncoder reviewed Nov 13, 2023

View reviewed changes

src/main/java/com/ctc/wstx/api/WstxInputProperties.java Outdated Show resolved Hide resolved

Made changes I suggested in PR code review, so naming kept consistent.

d85a50d

Merge branch 'master' into entity-surrogate-pairs

46e9db3

cowtowncoder changed the title ~~Support for entity surrogate pairs (#165)~~ Add support to optionally allow surrogate pair entities (#165) Nov 13, 2023

cowtowncoder and others added 2 commits November 12, 2023 18:35

Update release notes wrt this PR

8ed199a

Changes after PR review

1dcb8be

Magmaruss force-pushed the entity-surrogate-pairs branch from 02607fe to 1dcb8be Compare December 4, 2023 23:21

cowtowncoder added 2 commits December 6, 2023 20:35

Minor simplification

3d34620

Fix one @since tag, trivial whitespace removal

d5c42c0

cowtowncoder reviewed Dec 7, 2023

View reviewed changes

src/main/java/com/ctc/wstx/sr/StreamScanner.java Outdated Show resolved Hide resolved

Further streamlining

eb5189d

cowtowncoder reviewed Dec 7, 2023

View reviewed changes

Further simplification

c57023d

cowtowncoder added 3 commits January 5, 2024 18:39

Merge branch 'master' into entity-surrogate-pairs

0bdd8e4

Merge branch 'master' into entity-surrogate-pairs

659aaf8

Merge branch 'master' into entity-surrogate-pairs

eecec96

Test case for allow surrogate pair entities option disabled

88c5358

cowtowncoder approved these changes Jan 16, 2024

View reviewed changes

cowtowncoder added this to the 6.6.0 milestone Jan 16, 2024

cowtowncoder merged commit 172371f into FasterXML:master Jan 16, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to optionally allow surrogate pair entities (#165) #174

Add support to optionally allow surrogate pair entities (#165) #174

Magmaruss commented Jun 18, 2023 •

edited

Loading

cowtowncoder commented Jun 19, 2023

Magmaruss commented Jun 20, 2023

cowtowncoder Nov 3, 2023

cowtowncoder commented Nov 4, 2023 •

edited

Loading

cowtowncoder Nov 4, 2023

cowtowncoder Nov 4, 2023

Magmaruss Nov 13, 2023

cowtowncoder Nov 14, 2023

cowtowncoder commented Nov 11, 2023

cowtowncoder commented Nov 13, 2023

cowtowncoder commented Nov 13, 2023

cowtowncoder Dec 7, 2023

Magmaruss Dec 10, 2023

cowtowncoder Dec 11, 2023

cowtowncoder commented Dec 7, 2023

Magmaruss commented Dec 10, 2023

cowtowncoder commented Dec 11, 2023

cowtowncoder commented Jan 15, 2024

Magmaruss commented Jan 15, 2024

cowtowncoder commented Jan 15, 2024

Magmaruss commented Jan 15, 2024

cowtowncoder left a comment

Add support to optionally allow surrogate pair entities (#165) #174

Add support to optionally allow surrogate pair entities (#165) #174

Conversation

Magmaruss commented Jun 18, 2023 • edited Loading

cowtowncoder commented Jun 19, 2023

Magmaruss commented Jun 20, 2023

Choose a reason for hiding this comment

cowtowncoder commented Nov 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cowtowncoder commented Nov 11, 2023

cowtowncoder commented Nov 13, 2023

cowtowncoder commented Nov 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cowtowncoder commented Dec 7, 2023

Magmaruss commented Dec 10, 2023

cowtowncoder commented Dec 11, 2023

cowtowncoder commented Jan 15, 2024

Magmaruss commented Jan 15, 2024

cowtowncoder commented Jan 15, 2024

Magmaruss commented Jan 15, 2024

cowtowncoder left a comment

Choose a reason for hiding this comment

Magmaruss commented Jun 18, 2023 •

edited

Loading

cowtowncoder commented Nov 4, 2023 •

edited

Loading