Skip to content

Feature : Allow setting custom linefeed on DefaultXmlPrettyPrinter #568

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.

Sign up for GitHub

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 22 commits into from
Feb 17, 2023
Merged

Feature : Allow setting custom linefeed on DefaultXmlPrettyPrinter #568

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
374e125
feature allow custom line feed
JooHyukKim Feb 5, 2023
eb23bbd
Update XmlFactory.java
JooHyukKim Feb 5, 2023
c0b3e41
Correct method naming
JooHyukKim Feb 5, 2023
3b066cb
Set default and static default separately
JooHyukKim Feb 5, 2023
8595328
Make _lineFeed non static
JooHyukKim Feb 5, 2023
79564a4
Add dot to end documentation sentences.
JooHyukKim Feb 5, 2023
f515eba
Implment linefeed in XmlPrettyPrinter
JooHyukKim Feb 5, 2023
4cc4394
Implement linefeed only in DefaultXmlPrettyPrinter
JooHyukKim Feb 5, 2023
6974014
Update DefaultXmlPrettyPrinter.java
JooHyukKim Feb 5, 2023
b75bfdd
XML declaration not part of linefeed?
JooHyukKim Feb 5, 2023
18abcc2
Refactor : change names to newLine
JooHyukKim Feb 6, 2023
c764189
withCustomNewLine() method...
JooHyukKim Feb 10, 2023
af70ece
Apply review
JooHyukKim Feb 10, 2023
7deeb84
Fix : make FixedSpaceIndenter static
JooHyukKim Feb 10, 2023
8360276
Refactor : rename from linefeed to newline
JooHyukKim Feb 14, 2023
6187f3d
move SPACES char[] to outer DefaultXmlPrettyPrinter
JooHyukKim Feb 14, 2023
d0aaf64
Add default empty constructor
JooHyukKim Feb 14, 2023
29a2bbf
withCustomNewLine revert back to SYSTEM_DEFAULT_NEW_LINE if null is p…
JooHyukKim Feb 16, 2023
fc4fb17
replace \n with system linefeed to make platform independent test
JooHyukKim Feb 16, 2023
dd198f2
Update DefaultXmlPrettyPrinter copy constructor
JooHyukKim Feb 16, 2023
b43811c
Remove unrelated changes
JooHyukKim Feb 16, 2023
05b1f04
Remove unrelated formatting changes
JooHyukKim Feb 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 37 additions & 20 deletions src/main/java/com/fasterxml/jackson/dataformat/xml/util/DefaultXmlPrettyPrinter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,27 @@ public interface Indenter
*/
protected boolean _spacesInObjectEntries = true;

/**
* By default, will try to set as System.getProperty("line.separator").
* Can later set custom new line with withCustomNewLine method.
* @since 2.15
*/
private static final String SYSTEM_DEFAULT_NEW_LINE;
static {
String lf = null;
try {
lf = System.getProperty("line.separator");
} catch (Exception t) { } // access exception?
SYSTEM_DEFAULT_NEW_LINE = lf;
}
protected String _newLine = SYSTEM_DEFAULT_NEW_LINE;

static final int SPACE_COUNT = 64;
static final char[] SPACES = new char[SPACE_COUNT];
static {
Arrays.fill(SPACES, ' ');
}

/*
/**********************************************************
/* State
Expand Down Expand Up @@ -126,6 +147,17 @@ public void indentObjectsWith(Indenter i)

public void spacesInObjectEntries(boolean b) { _spacesInObjectEntries = b; }

/**
* Sets custom new-line
* @since 2.15
*/
public DefaultXmlPrettyPrinter withCustomNewLine(String newLine) {
if (newLine != null) {
_newLine = newLine;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

}
return this;
}

/*
/**********************************************************
/* Instantiatable impl
Expand Down Expand Up @@ -443,7 +475,7 @@ public void writeLeafXsiNilElement(XMLStreamWriter2 sw,
public void writePrologLinefeed(XMLStreamWriter2 sw) throws XMLStreamException
{
// 06-Dec-2015, tatu: Alternatively could try calling `writeSpace()`...
sw.writeRaw(Lf2SpacesIndenter.SYSTEM_LINE_SEPARATOR);
sw.writeRaw(_newLine);
}

/*
Expand Down Expand Up @@ -486,7 +518,7 @@ public void writeIndentation(XMLStreamWriter2 sw, int level)
{
sw.writeRaw(" ");
}

@Override
public void writeIndentation(JsonGenerator g, int level) throws IOException
{
Expand All @@ -501,26 +533,11 @@ public void writeIndentation(JsonGenerator g, int level) throws IOException
* Default linefeed-based indenter uses system-specific linefeeds and
* 2 spaces for indentation per level.
*/
protected static class Lf2SpacesIndenter
protected class Lf2SpacesIndenter
implements Indenter, java.io.Serializable
{
private static final long serialVersionUID = 1L;

final static String SYSTEM_LINE_SEPARATOR;
static {
String lf = null;
try {
lf = System.getProperty("line.separator");
} catch (Throwable t) { } // access exception?
SYSTEM_LINE_SEPARATOR = (lf == null) ? "\n" : lf;
}

final static int SPACE_COUNT = 64;
final static char[] SPACES = new char[SPACE_COUNT];
static {
Arrays.fill(SPACES, ' ');
}

public Lf2SpacesIndenter() { }

@Override
Expand All @@ -529,7 +546,7 @@ public Lf2SpacesIndenter() { }
@Override
public void writeIndentation(XMLStreamWriter2 sw, int level) throws XMLStreamException
{
sw.writeRaw(SYSTEM_LINE_SEPARATOR);
sw.writeRaw(_newLine);
level += level; // 2 spaces per level
while (level > SPACE_COUNT) { // should never happen but...
sw.writeRaw(SPACES, 0, SPACE_COUNT);
Expand All @@ -541,7 +558,7 @@ public void writeIndentation(XMLStreamWriter2 sw, int level) throws XMLStreamExc
@Override
public void writeIndentation(JsonGenerator jg, int level) throws IOException
{
jg.writeRaw(SYSTEM_LINE_SEPARATOR);
jg.writeRaw(_newLine);
level += level; // 2 spaces per level
while (level > SPACE_COUNT) { // should never happen but...
jg.writeRaw(SPACES, 0, SPACE_COUNT);
Expand Down
74 changes: 73 additions & 1 deletion src/test/java/com/fasterxml/jackson/dataformat/xml/ser/XmlPrettyPrinterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import com.fasterxml.jackson.dataformat.xml.XmlTestBase;
import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlElementWrapper;
import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlProperty;
import com.fasterxml.jackson.dataformat.xml.util.DefaultXmlPrettyPrinter;

public class XmlPrettyPrinterTest extends XmlTestBase
{
Expand Down Expand Up @@ -176,7 +177,7 @@ public void testMultiLevel172() throws Exception
.writeValueAsString(root);
// unify possible apostrophes to quotes
xml = a2q(xml);
// with indentation, should get linefeeds in prolog/epilog too

assertEquals("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
+"<Company>\n"
+" <e>\n"
Expand All @@ -188,4 +189,75 @@ public void testMultiLevel172() throws Exception
+"</Company>\n",
xml);
}

public void testNewLine_withCustomNewLine() throws Exception {
Company root = new Company();
root.employee.add(new Employee("abc"));

String xml = _xmlMapper.writer()
.with(new DefaultXmlPrettyPrinter().withCustomNewLine("\n\rLF\n\r"))
.with(ToXmlGenerator.Feature.WRITE_XML_DECLARATION)
.writeValueAsString(root);
// unify possible apostrophes to quotes
xml = a2q(xml);

// with indentation, should get newLines in prolog/epilog too
assertEquals("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
Copy link
Member Author

@JooHyukKim JooHyukKim Feb 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

see the answer to your follow up question

+ "<Company>\n\rLF\n\r"
+ " <e>\n\rLF\n\r"
+ " <employee>\n\rLF\n\r"
+ " <id>abc</id>\n\rLF\n\r"
+ " <type>FULL_TIME</type>\n\rLF\n\r"
+ " </employee>\n\rLF\n\r"
+ " </e>\n\rLF\n\r"
+ "</Company>\n",
Copy link
Member Author

@JooHyukKim JooHyukKim Feb 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@JooHyukKim JooHyukKim Feb 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@JooHyukKim JooHyukKim Feb 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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,

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

@JooHyukKim JooHyukKim Feb 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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 🙏🏼

xml);
}

public void testNewLine_systemDefault() throws Exception {
Company root = new Company();
root.employee.add(new Employee("abc"));

String xml = _xmlMapper.writer()
.with(new DefaultXmlPrettyPrinter())
.with(ToXmlGenerator.Feature.WRITE_XML_DECLARATION)
.writeValueAsString(root);
// unify possible apostrophes to quotes
xml = a2q(xml);

// with indentation, should get newLines in prolog/epilog too
assertEquals("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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");"?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah sounds good wrt hard-coded linefeeds

+ "<Company>\n"
+ " <e>\n"
+ " <employee>\n"
+ " <id>abc</id>\n"
+ " <type>FULL_TIME</type>\n"
+ " </employee>\n"
+ " </e>\n"
+ "</Company>\n",
xml);
}

public void testNewLine_UseSystemDefaultLineSeperatorOnNullCustomNewLine() throws Exception {
Company root = new Company();
root.employee.add(new Employee("abc"));

String xml = _xmlMapper.writer()
.with(new DefaultXmlPrettyPrinter().withCustomNewLine(null))
.with(ToXmlGenerator.Feature.WRITE_XML_DECLARATION)
.writeValueAsString(root);
// unify possible apostrophes to quotes
xml = a2q(xml);

assertEquals("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
+ "<Company>\n"
+ " <e>\n"
+ " <employee>\n"
+ " <id>abc</id>\n"
+ " <type>FULL_TIME</type>\n"
+ " </employee>\n"
+ " </e>\n"
+ "</Company>\n",
xml);
}
}