Allow nulls for write elements in MXSerializer by garydgregory · Pull Request #29 · codehaus-plexus/plexus-xml

garydgregory · 2024-05-20T13:07:52Z

https://issues.apache.org/jira/browse/MJAVADOC-793

Allow nulls for write elements in MXSerializer.

slawekjaranowski · 2024-05-20T13:31:01Z

@garydgregory thanks ... I afraid thet we will have a conflict with my PR #28

If you let me I first merge my PR and that you can rebase your

garydgregory · 2024-05-20T14:34:51Z

@slawekjaranowski
OK, done.

slawekjaranowski · 2024-05-20T20:31:20Z

@michael-o @elharo can you also look at it 😄

michael-o · 2024-05-20T20:32:55Z

I'd like to see this rebased first...

michael-o · 2024-05-20T20:34:04Z

After the rebase is done let's try MJAVADOC again with a snapshot of Plexus XML and see what happens.

garydgregory · 2024-05-20T21:27:31Z

I rebased on master again.

slawekjaranowski · 2024-05-20T21:47:58Z

src/test/java/org/codehaus/plexus/util/xml/pull/MXSerializerTest.java

+        new MXSerializer().writeElementContent(null, stringWriter);
+        new MXSerializer().writeAttributeValue(null, stringWriter);


should support ?

new MXSerializer().writeElementContent("value", null); new MXSerializer().writeAttributeValue("value", null);

Also add some of assertions for what content of stringWriter should be.

src/main/java/org/codehaus/plexus/util/xml/pull/MXSerializer.java

slawekjaranowski · 2024-05-20T21:53:45Z

Please also change of PR / commit message to show what really is changed, what is root cause eg

Allow nulls for write elements in MXSerializer

michael-o · 2024-05-21T09:18:20Z

Test:

	 public static void main(String[] args) throws ParserConfigurationException, TransformerException {
			Document doc = DocumentBuilderFactory.newInstance().newDocumentBuilder().newDocument();
			Element root = doc.createElement("root");
			root.setTextContent(null);
			root.setAttribute("foo", null);
			doc.appendChild(root);

			DOMSource domSource = new DOMSource(doc);
			StreamResult result = new StreamResult(System.out);
	        TransformerFactory tf = TransformerFactory.newInstance();
	        Transformer transformer = tf.newTransformer();
	        transformer.setOutputProperty(OutputKeys.INDENT, "yes");
	        transformer.setOutputProperty("{http://xml.apache.org/xslt}indent-amount", "2");
	        transformer.transform(domSource, result);
		}

output:

<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<root foo=""/>

michael-o · 2024-05-21T09:18:52Z

What about the pretty printer?

elharo

some suggestion. I don't see anything actually incorrect here.

elharo · 2024-05-21T11:13:42Z

src/main/java/org/codehaus/plexus/util/xml/pull/MXSerializer.java

-            } else {
+        if (value != null) {
+            // .[apostrophe and <, & escaped],
+            final char quot = attributeUseApostrophe ? '\'' : '"';


Not sure why we even make a choice here. Would be simpler to pick one and stick with it.

elharo · 2024-05-21T11:14:20Z

src/main/java/org/codehaus/plexus/util/xml/pull/MXSerializer.java

+                    out.write(quotEntity);
                    pos = i + 1;
                } else if (ch < 32) {
                    // in XML 1.0 only legal character are #x9 | #xA | #xD


legal C0 characters

elharo · 2024-05-21T11:14:34Z

src/main/java/org/codehaus/plexus/util/xml/pull/MXSerializer.java

-                        // pass through
-
-                        // } else if(ch == 13) { //escape
+                    // and they must be escaped otherwise in attribute value they are normalized to spaces


elharo · 2024-05-21T11:15:24Z

src/main/java/org/codehaus/plexus/util/xml/pull/MXSerializer.java

+                        pos = i + 1;
+                    } else {
+                        throw new IllegalStateException(
+                                "character " + Integer.toString(ch) + " is not allowed in output" + getLocation());


IllegalArgumentException

elharo · 2024-05-21T11:15:33Z

src/main/java/org/codehaus/plexus/util/xml/pull/MXSerializer.java

+                    } else {
+                        throw new IllegalStateException(
+                                "character " + Integer.toString(ch) + " is not allowed in output" + getLocation());
+                        // in XML 1.1 legal are [#x1-#xD7FF]


I'd delete all this commented code

elharo · 2024-05-21T11:16:30Z

src/main/java/org/codehaus/plexus/util/xml/pull/MXSerializer.java

+                        if (ch == 9 || ch == 10 || ch == 13) {
+                            // pass through
+
+                            // } else if(ch == 13) { //escape


delete commented code

garydgregory · 2024-05-21T12:45:28Z

@elharo Thank you for your comments. I'll let the Maven team address those since I feel they are out of scope for this PR.

michael-o · 2024-05-21T12:58:51Z

src/test/java/org/codehaus/plexus/util/xml/pull/MXSerializerTest.java

+     * Tests MJAVADOC-793.
+     */
+    @Test
+    public void testMJAVADOC793() throws IOException {


This should be renamed. This is not related to the Javadoc plugin. IT simply does not allow null values. testNullValues()?

Done: testWriteNullValues().

slawekjaranowski · 2024-05-21T15:56:55Z

@elharo Thank you for your comments. I'll let the Maven team address those since I feel they are out of scope for this PR.

you right if needed should be fixed in separate PR

slawekjaranowski

looks OK please chenge test name and commit PR title

https://issues.apache.org/jira/browse/MJAVADOC-793

slawekjaranowski · 2024-05-21T20:45:38Z

@michael-o I hope your doubts was resolved, so I will merge and process forward

garydgregory · 2024-05-21T21:49:28Z

TY @slawekjaranowski

michael-o · 2024-05-22T07:05:21Z

src/test/java/org/codehaus/plexus/util/xml/pull/MXSerializerTest.java

    }
+
+    /**
+     * Tests MJAVADOC-793.


Well, that comment is wrong here as well.

michael-o · 2024-05-22T07:05:50Z

@michael-o I hope your doubts was resolved, so I will merge and process forward

Yes, but my question about the PrettyPrinter remains open.

garydgregory force-pushed the MJAVADOC-793 branch 2 times, most recently from 8a472f5 to e03c11f Compare May 20, 2024 14:34

garydgregory force-pushed the MJAVADOC-793 branch from e03c11f to 5a5581a Compare May 20, 2024 21:27

slawekjaranowski reviewed May 20, 2024

View reviewed changes

src/main/java/org/codehaus/plexus/util/xml/pull/MXSerializer.java Outdated Show resolved Hide resolved

elharo approved these changes May 21, 2024

View reviewed changes

garydgregory force-pushed the MJAVADOC-793 branch from 5a5581a to b2263d5 Compare May 21, 2024 12:43

garydgregory requested a review from slawekjaranowski May 21, 2024 12:44

michael-o requested changes May 21, 2024

View reviewed changes

slawekjaranowski reviewed May 21, 2024

View reviewed changes

Allow nulls for write elements in MXSerializer

5a58475

https://issues.apache.org/jira/browse/MJAVADOC-793

garydgregory force-pushed the MJAVADOC-793 branch from b2263d5 to 5a58475 Compare May 21, 2024 17:07

garydgregory changed the title ~~Fix for MJAVADOC-793~~ Allow nulls for write elements in MXSerializer May 21, 2024

slawekjaranowski approved these changes May 21, 2024

View reviewed changes

garydgregory requested a review from michael-o May 21, 2024 20:02

slawekjaranowski added the bug Something isn't working label May 21, 2024

slawekjaranowski merged commit 43dbdca into codehaus-plexus:master May 21, 2024

garydgregory deleted the MJAVADOC-793 branch May 21, 2024 21:49

michael-o reviewed May 22, 2024

View reviewed changes

		new MXSerializer().writeElementContent(null, stringWriter);
		new MXSerializer().writeAttributeValue(null, stringWriter);

Conversation

garydgregory commented May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slawekjaranowski commented May 20, 2024

Uh oh!

garydgregory commented May 20, 2024

Uh oh!

slawekjaranowski commented May 20, 2024

Uh oh!

michael-o commented May 20, 2024

Uh oh!

michael-o commented May 20, 2024

Uh oh!

garydgregory commented May 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

slawekjaranowski commented May 20, 2024

Uh oh!

michael-o commented May 21, 2024

Uh oh!

michael-o commented May 21, 2024

Uh oh!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garydgregory commented May 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slawekjaranowski commented May 21, 2024

Uh oh!

slawekjaranowski left a comment

Choose a reason for hiding this comment

Uh oh!

slawekjaranowski commented May 21, 2024

Uh oh!

garydgregory commented May 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-o commented May 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

garydgregory commented May 20, 2024 •

edited

Loading