Feature : Allow setting custom linefeed on DefaultXmlPrettyPrinter #568

JooHyukKim · 2023-02-05T21:42:19Z

for Add DefaultXmlPrettyPrinter.withCustomNewLine() to configure linefeed for XML pretty-printing #560

BEFORE

Linefeed is set when static { } block from Lf2SpacesIndenter is run.
both DefaultXmlPrettyPrinterand Indenters both use static Lf2SpacesIndenter.SYSTEM_LINE_SEPARATOR

AFTER

_lineFeed is set when static { } block from DefaultXmlPrettyPrinter is run.
Indenters ONLY customize indentations and linefeed value is given by DefaultXmlPrettyPrinter

src/main/java/com/fasterxml/jackson/dataformat/xml/util/DefaultXmlPrettyPrinter.java

pjfanning · 2023-02-05T22:08:59Z

src/main/java/com/fasterxml/jackson/dataformat/xml/util/DefaultXmlPrettyPrinter.java

+        }
+    }
+
+    protected static String _lineFeed = DEFAULT_LINE_FEED;


protected String _lineFeed = DEFAULT_LINE_FEED; -- should not be static

@pjfanning I have a question.
Even if we make _lineFeed field non-static, we have static line-feeding Indenters like Lf2SpacesIndenter. Linefeed happens inside Lf2SpacesIndenter.writeIndentation().
So Indenters can't access non-static field of outer class. 🥲

How can we solve this?

My thoughts, line-feed outside of Lf2SpacesIndenter?

Lf2SpacesIndenter could have a constructor that takes an instance of DefaultXmlPrettyPrinter in its constructor. Or we could make Lf2SpacesIndenter non-static.

ic

pjfanning · 2023-02-05T22:37:41Z

I have to ask why you are not ensuring the code compiles?

The @Override should not have been added to the new lineFeed() method. In fact, it should not even be necessary. It is likely that Lf2SpacesIndenter can access the defaultXmlPrettyPrinter._lineFeed.

JooHyukKim · 2023-02-05T22:41:36Z

I have to ask why you are not ensuring the code compiles?

My mistake, won't happen again 👍🏻👍🏻

The @Override should not have been added to the new lineFeed() method. In fact, it should not even be necessary. It is likely that Lf2SpacesIndenter can access the defaultXmlPrettyPrinter._lineFeed.

I making Lf2SpacesIndenter non-static at the moment. Sorry for the confusion

JooHyukKim · 2023-02-05T22:53:40Z

src/test/java/com/fasterxml/jackson/dataformat/xml/ser/XmlPrettyPrinterTest.java

+        xml = a2q(xml);
+
+        // with indentation, should get linefeeds in prolog/epilog too
+        assertEquals("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"


I am not sure if this line should end with customLineFeed instead of `\n?

see the answer to your follow up question

JooHyukKim · 2023-02-05T22:53:52Z

src/test/java/com/fasterxml/jackson/dataformat/xml/ser/XmlPrettyPrinterTest.java

+                + "      <type>FULL_TIME</type>\n\rLF\n\r"
+                + "    </employee>\n\rLF\n\r"
+                + "  </e>\n\rLF\n\r"
+                + "</Company>\n",


I am not sure if this line should end with customLineFeed instead of `\n?

yes - all the line feeds should be the same

actually, could we rename the method as withCustomNewLine? - I don't think 'line feed' is a good name for this property at all - 'new line' is better

yes - all the line feeds should be the same

noted will make sure tests are applied there?

actually, could we rename the method as withCustomNewLine? - I don't think 'line feed' is a good name for this property at all - 'new line' is better

Agreed and I will change names as below!🙏🏼

_lineFeed -> _newLine

DEFAULT_LINE_FEED -> SYSTEM_DEFAULT_NEW_LINE

withCustomLineFeed -> withCustomNewLine

@pjfanning So here is an update.

actually, could we rename the method as withCustomNewLine? - I don't think 'line feed' is a good name for this property at all - 'new line' is better

Done.
All names are changed from 'line feed' to 'new line'

yes - all the line feeds should be the same

May I get your opinion here? It seems like writing custom 'new lines' before and after root element might be out of this PR's scope. The 'new lines' before and after the root element are written by method writePrologLinefeed() of another instance of DefaultXmlPrettyPrinter.

This instance invoking writePrologLinefeed() instance seems to be initialized and used inside ToXmlGenerator which we cannot hook into from neither XmlMapper().setDefaultPrettyPrinter() nor ObjectWriter.with(PrettyPrinter).

For references,

Similar issues I found doing research. Removed prolog line feed in XMLPrettyPrinter as the current version m… #554

Check 👉🏼 the code here writes BEFORE root element and the code here writes AFTER root element

It makes little sense to me not to use the same new lines throughout the XML doc. I'm now -1 on this PR.

@JooHyukKim Ok that sounds like a problem indeed! I wonder if it's the case of Indenter instance not being created yet (with createInstance()) or what is happening.

Not the indenter, actually. There is a separate instance of DefaultXmlPrettyPrinter initialized together with ToXmlGenerator that invokes _xmlPrettyPrinter.writePrologLinefeed to write prolog and epilog.

You can refer to below to code lines.

ToXmlGenerator. initGenerator method for prolog and

ToXmlGenerator._handleObject for epilog

My first (probable) observation is that writing prolog and epilog with instance of DefaultXmlPrettyPrinter I mentioned above seems NOT to be overridden by ObjectMapper.setDefaultPrettyPrinter(PrettyPrinter) nor ObjectWriter.with(PrettyPrinter).

Second observation is ToXmlGenerator writes prolog during the process of initialization within ToXmlGenerator.initGenerator method.

It will be great, @cowtowncoder if you can help out a little here? To override DefaultXmlPrettyPrinter of ToXmlGenerator._xmlPrettyPrinter before it writes prolog and epilog?

Right, pretty printer instance. I will try to see if I can figure it out -- the problem is that XML module extends basic PrettyPrinter API to its needs but databind still tries to orchestrate handling. So I will try to see if I can figure out how to avoid there being different instance being used.

@JooHyukKim Ah. Part of the problem is that you haven't updated copy constructor:

protected DefaultXmlPrettyPrinter(DefaultXmlPrettyPrinter base) { }

to copy _newLine property -- so when method

@Override public DefaultXmlPrettyPrinter createInstance() {

is called the new instance "forgets" _newLine. So you need to add

_newLine = base._newLine;

in that constructor.

is called the new instance "forgets" _newLine. So you need to add

Woooooow thank you for helping out 🥹 @cowtowncoder
How did I miss that!
Done 🙏🏼

pjfanning · 2023-02-06T21:14:14Z

src/main/java/com/fasterxml/jackson/dataformat/xml/util/DefaultXmlPrettyPrinter.java

+     * @since 2.15
+     */
+    public DefaultXmlPrettyPrinter withCustomNewLine(String newLine) {
+        // 06-Feb-2023, joohyukkim: when JacksonException extends RuntimeExceptions, throw it?


can you remove this comment?

pjfanning · 2023-02-06T21:15:56Z

src/test/java/com/fasterxml/jackson/dataformat/xml/ser/XmlPrettyPrinterTest.java

+                + "      <type>FULL_TIME</type>\n\rLF\n\r"
+                + "    </employee>\n\rLF\n\r"
+                + "  </e>\n\rLF\n\r"
+                + "</Company>\n",


It makes little sense to me not to use the same new lines throughout the XML doc. I'm now -1 on this PR.

src/main/java/com/fasterxml/jackson/dataformat/xml/util/DefaultXmlPrettyPrinter.java

cowtowncoder · 2023-02-10T04:35:52Z

src/main/java/com/fasterxml/jackson/dataformat/xml/util/DefaultXmlPrettyPrinter.java

+        String lf = null;
+        try {
+            lf = System.getProperty("line.separator");
+        } catch (Throwable t) { } // access exception?


Let's just catch Exception, not Throwable (ok unlike we'd get OOME or such but as a general good practice)

late reply, but done 👍🏻

src/main/java/com/fasterxml/jackson/dataformat/xml/util/DefaultXmlPrettyPrinter.java

cowtowncoder · 2023-02-10T04:37:26Z

src/main/java/com/fasterxml/jackson/dataformat/xml/util/DefaultXmlPrettyPrinter.java

@@ -473,7 +501,7 @@ public NopIndenter() { }
     * single space for indentation. It is used as the default
     * indenter for array values.
     */
-    protected static class FixedSpaceIndenter
+    protected class FixedSpaceIndenter


Same here, keep static (no need to refer to enclosing class)

src/main/java/com/fasterxml/jackson/dataformat/xml/util/DefaultXmlPrettyPrinter.java

JooHyukKim · 2023-02-10T09:54:42Z

Sorry it took me long to react! 🙏🏼
I just moved house 🥲🥲🥲 and it's taking so much time to pack and unpack stuff

I just got internet back, I will try to apply your feed back asap.

JooHyukKim · 2023-02-14T23:48:05Z

All reviews made up to this point are applied to the changes.

The only thing left is being discussed in the thread of comments above. Simply put, about how to(or whether) to apply custom new line while writing XML declaration with ToXmlGenerator.Feature.WRITE_XML_DECLARATION or not.

cowtowncoder · 2023-02-16T04:16:01Z

src/test/java/com/fasterxml/jackson/dataformat/xml/ser/XmlPrettyPrinterTest.java

+        xml = a2q(xml);
+
+        // with indentation, should get newLines in prolog/epilog too
+        assertEquals("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"


Need to replace \n with system linefeed so test passes on Windows systems too.

Right, done! 🙏🏼

And plus I checked other similar test cases' assertions and some hard coded \n as system linefeed.
@cowtowncoder if you agree, I will make a separate PR on "replacing all hard coded \n to the value read from System.getProperty("line.separator");"?

Yeah sounds good wrt hard-coded linefeeds

cowtowncoder · 2023-02-16T04:17:49Z

src/main/java/com/fasterxml/jackson/dataformat/xml/util/DefaultXmlPrettyPrinter.java

+     */
+    public DefaultXmlPrettyPrinter withCustomNewLine(String newLine) {
+        if (newLine != null) {
+            _newLine = newLine;


Why not set _newLine to SYSTEM_DEFAULT_NEW_LINE if null is passed? This would allow reverting back to system newline.

Agreed, I don't see any problem doing so.
done!

…assed

cowtowncoder · 2023-02-17T03:54:11Z

Thank you again @JooHyukKim ! Merged for inclusion in 2.15.0

JooHyukKim added 2 commits February 6, 2023 06:29

feature allow custom line feed

374e125

Update XmlFactory.java

eb23bbd

pjfanning reviewed Feb 5, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/dataformat/xml/util/DefaultXmlPrettyPrinter.java Show resolved Hide resolved

pjfanning reviewed Feb 5, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/dataformat/xml/util/DefaultXmlPrettyPrinter.java Outdated Show resolved Hide resolved

JooHyukKim added 2 commits February 6, 2023 06:55

Correct method naming

c0b3e41

Set default and static default separately

3b066cb

pjfanning reviewed Feb 5, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/dataformat/xml/util/DefaultXmlPrettyPrinter.java Outdated Show resolved Hide resolved

pjfanning reviewed Feb 5, 2023

View reviewed changes

JooHyukKim added 5 commits February 6, 2023 07:12

Make _lineFeed non static

8595328

ic

Add dot to end documentation sentences.

79564a4

Implment linefeed in XmlPrettyPrinter

f515eba

Implement linefeed only in DefaultXmlPrettyPrinter

4cc4394

Update DefaultXmlPrettyPrinter.java

6974014

XML declaration not part of linefeed?

b75bfdd

JooHyukKim commented Feb 5, 2023

View reviewed changes

JooHyukKim requested a review from pjfanning February 5, 2023 22:56

Refactor : change names to newLine

18abcc2

pjfanning reviewed Feb 6, 2023

View reviewed changes

pjfanning requested changes Feb 6, 2023

View reviewed changes

cowtowncoder reviewed Feb 10, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/dataformat/xml/util/DefaultXmlPrettyPrinter.java Outdated Show resolved Hide resolved

cowtowncoder reviewed Feb 10, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/dataformat/xml/util/DefaultXmlPrettyPrinter.java Outdated Show resolved Hide resolved

cowtowncoder reviewed Feb 10, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/dataformat/xml/util/DefaultXmlPrettyPrinter.java Outdated Show resolved Hide resolved

JooHyukKim added 2 commits February 10, 2023 20:24

withCustomNewLine() method...

c764189

Apply review

af70ece

JooHyukKim added 4 commits February 10, 2023 20:28

Fix : make FixedSpaceIndenter static

7deeb84

Refactor : rename from linefeed to newline

8360276

move SPACES char[] to outer DefaultXmlPrettyPrinter

6187f3d

Add default empty constructor

d0aaf64

cowtowncoder reviewed Feb 16, 2023

View reviewed changes

JooHyukKim added 5 commits February 17, 2023 00:13

withCustomNewLine revert back to SYSTEM_DEFAULT_NEW_LINE if null is p…

29a2bbf

…assed

replace \n with system linefeed to make platform independent test

fc4fb17

Update DefaultXmlPrettyPrinter copy constructor

dd198f2

Remove unrelated changes

b43811c

Remove unrelated formatting changes

05b1f04

cowtowncoder approved these changes Feb 17, 2023

View reviewed changes

cowtowncoder merged commit 74ac5ac into FasterXML:2.15 Feb 17, 2023

JooHyukKim mentioned this pull request Feb 18, 2023

Test : Remove hardcoded new line "\n" from tests #572

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature : Allow setting custom linefeed on DefaultXmlPrettyPrinter #568

Feature : Allow setting custom linefeed on DefaultXmlPrettyPrinter #568

JooHyukKim commented Feb 5, 2023

pjfanning Feb 5, 2023

JooHyukKim Feb 5, 2023

JooHyukKim Feb 5, 2023

pjfanning Feb 5, 2023

pjfanning commented Feb 5, 2023

JooHyukKim commented Feb 5, 2023

JooHyukKim Feb 5, 2023 •

edited

Loading

pjfanning Feb 5, 2023

JooHyukKim Feb 5, 2023 •

edited

Loading

pjfanning Feb 5, 2023

JooHyukKim Feb 5, 2023 •

edited

Loading

JooHyukKim Feb 6, 2023 •

edited

Loading

pjfanning Feb 6, 2023

cowtowncoder Feb 14, 2023

JooHyukKim Feb 14, 2023 •

edited

Loading

cowtowncoder Feb 16, 2023

cowtowncoder Feb 16, 2023

JooHyukKim Feb 16, 2023

pjfanning Feb 6, 2023

JooHyukKim Feb 14, 2023

pjfanning Feb 6, 2023

cowtowncoder Feb 10, 2023

JooHyukKim Feb 16, 2023

cowtowncoder Feb 10, 2023

JooHyukKim commented Feb 10, 2023

JooHyukKim commented Feb 14, 2023 •

edited

Loading

cowtowncoder Feb 16, 2023

JooHyukKim Feb 16, 2023

cowtowncoder Feb 17, 2023

cowtowncoder Feb 16, 2023

JooHyukKim Feb 16, 2023

cowtowncoder commented Feb 17, 2023

Feature : Allow setting custom linefeed on DefaultXmlPrettyPrinter #568

Feature : Allow setting custom linefeed on DefaultXmlPrettyPrinter #568

Conversation

JooHyukKim commented Feb 5, 2023

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

pjfanning commented Feb 5, 2023

JooHyukKim commented Feb 5, 2023

JooHyukKim Feb 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JooHyukKim Feb 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JooHyukKim Feb 5, 2023 • edited Loading

Choose a reason for hiding this comment

JooHyukKim Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JooHyukKim Feb 14, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JooHyukKim commented Feb 10, 2023

JooHyukKim commented Feb 14, 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

Choose a reason for hiding this comment

cowtowncoder commented Feb 17, 2023

JooHyukKim Feb 5, 2023 •

edited

Loading

JooHyukKim Feb 5, 2023 •

edited

Loading

JooHyukKim Feb 5, 2023 •

edited

Loading

JooHyukKim Feb 6, 2023 •

edited

Loading

JooHyukKim Feb 14, 2023 •

edited

Loading

JooHyukKim commented Feb 14, 2023 •

edited

Loading