Skip to content
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

Jump to bottom

[SUREFIRE-1556] fail fast on empty element names #11

Merged
merged 4 commits into from
Apr 21, 2021
Merged

[SUREFIRE-1556] fail fast on empty element names #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fee97f9
fail fast on empty element names
elharo Feb 23, 2020
e448a88
merge master
elharo Apr 16, 2021
2c65109
format
elharo Apr 16, 2021
b16a973
Merge branch 'master' into xml3
Apr 21, 2021
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
52 changes: 29 additions & 23 deletions src/main/java/org/apache/maven/shared/utils/xml/PrettyPrintXMLWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public class PrettyPrintXMLWriter

/**
* @param writer not null
* @param lineIndent could be null, but the normal way is some spaces.
* @param lineIndent can be null, but the normal way is some spaces
*/
public PrettyPrintXMLWriter( PrintWriter writer, String lineIndent )
{
Expand All @@ -69,7 +69,7 @@ public PrettyPrintXMLWriter( PrintWriter writer, String lineIndent )

/**
* @param writer not null
* @param lineIndent could be null, but the normal way is some spaces.
* @param lineIndent can be null, but the normal way is some spaces.
*/
public PrettyPrintXMLWriter( Writer writer, String lineIndent )
{
Expand All @@ -94,9 +94,9 @@ public PrettyPrintXMLWriter( Writer writer )

/**
* @param writer not null
* @param lineIndent could be null, but the normal way is some spaces.
* @param encoding could be null or invalid.
* @param doctype could be null.
* @param lineIndent can be null, but the normal way is some spaces
* @param encoding can be null or invalid
* @param doctype can be null
*/
public PrettyPrintXMLWriter( PrintWriter writer, String lineIndent, String encoding, String doctype )
{
Expand All @@ -105,9 +105,9 @@ public PrettyPrintXMLWriter( PrintWriter writer, String lineIndent, String encod

/**
* @param writer not null
* @param lineIndent could be null, but the normal way is some spaces.
* @param encoding could be null or invalid.
* @param doctype could be null.
* @param lineIndent can be null, but the normal way is some spaces
* @param encoding can be null or invalid
* @param doctype can be null
*/
public PrettyPrintXMLWriter( Writer writer, String lineIndent, String encoding, String doctype )
{
Expand All @@ -116,8 +116,8 @@ public PrettyPrintXMLWriter( Writer writer, String lineIndent, String encoding,

/**
* @param writer not null
* @param encoding could be null or invalid.
* @param doctype could be null.
* @param encoding can be null or invalid
* @param doctype can be null
*/
public PrettyPrintXMLWriter( PrintWriter writer, String encoding, String doctype )
{
Expand All @@ -126,8 +126,8 @@ public PrettyPrintXMLWriter( PrintWriter writer, String encoding, String doctype

/**
* @param writer not null
* @param encoding could be null or invalid.
* @param doctype could be null.
* @param encoding can be null or invalid
* @param doctype can be null
*/
public PrettyPrintXMLWriter( Writer writer, String encoding, String doctype )
{
Expand All @@ -136,10 +136,10 @@ public PrettyPrintXMLWriter( Writer writer, String encoding, String doctype )

/**
* @param writer not null
* @param lineIndent could be null, but the normal way is some spaces.
* @param lineSeparator could be null, but the normal way is valid line separator
* @param encoding could be null or the encoding to use.
* @param doctype could be null.
* @param lineIndent can be null, but the normal way is some spaces.
* @param lineSeparator can be null, but the normal way is valid line separator
* @param encoding can be null or the encoding to use.
* @param doctype can be null
*/
public PrettyPrintXMLWriter( PrintWriter writer, String lineIndent, String lineSeparator, String encoding,
String doctype )
Expand All @@ -149,10 +149,10 @@ public PrettyPrintXMLWriter( PrintWriter writer, String lineIndent, String lineS

/**
* @param writer not null
* @param lineIndent could be null, but the normal way is some spaces.
* @param lineSeparator could be null, but the normal way is valid line separator
* @param encoding could be null or the encoding to use.
* @param doctype could be null.
* @param lineIndent can be null, but the normal way is some spaces
* @param lineSeparator can be null, but the normal way is valid line separator
* @param encoding can be null or the encoding to use
* @param doctype can be null
*/
private PrettyPrintXMLWriter( PrintWriter writer, char[] lineIndent, char[] lineSeparator, String encoding,
String doctype )
Expand Down Expand Up @@ -211,7 +211,7 @@ public void setDocType( String docType )
}

/**
* @param lineSeparator The line separator to be used.
* @param lineSeparator the line separator to be output
*/
public void setLineSeparator( String lineSeparator )
{
Expand All @@ -224,7 +224,7 @@ public void setLineSeparator( String lineSeparator )
}

/**
* @param lineIndentParameter The line indent parameter.
* @param lineIndentParameter the line indent parameter
*/
public void setLineIndenter( String lineIndentParameter )
{
Expand All @@ -239,6 +239,12 @@ public void setLineIndenter( String lineIndentParameter )
/** {@inheritDoc} */
public void startElement( String elementName ) throws IOException
{

if ( elementName.isEmpty() )
{
throw new IllegalArgumentException( "Element name cannot be empty" );
}

boolean firstLine = ensureDocumentStarted();

completePreviouslyOpenedElement();
Expand Down Expand Up @@ -325,7 +331,7 @@ public void endElement() throws IOException
}

/**
* Write the documents if not already done.
* Write the document if not already done.
*
* @return <code>true</code> if the document headers have freshly been written.
*/
Expand Down
26 changes: 15 additions & 11 deletions src/main/java/org/apache/maven/shared/utils/xml/XMLWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,15 @@ public interface XMLWriter

/**
* Sets the encoding of the document.
* If not set, UTF-8 is being used
* If not set, UTF-8 is used.
*
* @param encoding the encoding
* @throws IllegalStateException if the generation of the document has already started
*/
void setEncoding( String encoding );

/**
* Sets the docType of the document.
* Sets the DOCTYPE of the document.
*
* @param docType the docType
* @throws IllegalStateException if the generation of the document has already started
Expand All @@ -48,15 +48,17 @@ public interface XMLWriter

/**
* Start an XML Element tag.
* @param name The name of the tag.
* @throws IOException if starting the element fails.
*
* @param name the name of the tag
* @throws IOException if starting the element fails
*/
void startElement( String name ) throws IOException;


/**
* Add a XML attribute to the current XML Element.
* This method must get called immediately after {@link #startElement(String)}
* This method must get called immediately after {@link #startElement(String)}.
*
* @param key The key of the attribute.
* @param value The value of the attribute.
* @throws IllegalStateException if no element tag is currently in process
Expand All @@ -65,19 +67,21 @@ public interface XMLWriter
void addAttribute( String key, String value ) throws IOException;

/**
* Add a value text to the current element tag
* This will perform XML escaping to guarantee valid content
* Add text to the current element tag.
* This performs XML escaping to guarantee well-formed content.
*
* @param text The text which should be written.
* @throws IllegalStateException if no element tag got started yet
* @throws IOException if writing the text fails.
*/
void writeText( String text ) throws IOException;

/**
* Add a preformatted markup to the current element tag
* @param text The text which should be written.
* @throws IllegalStateException if no element tag got started yet
* @throws IOException if writing the markup fails.
* Add preformatted markup to the current element tag.
*
* @param text the text which should be written
* @throws IllegalStateException if no element tag is started yet
* @throws IOException if writing the markup fails
*/
void writeMarkup( String text ) throws IOException;

Expand Down
19 changes: 15 additions & 4 deletions src/test/java/org/apache/maven/shared/utils/xml/PrettyPrintXmlWriterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,8 @@
*/

import java.io.IOException;

import javax.swing.text.html.HTML;
import java.io.StringWriter;
import javax.swing.text.html.HTML;

import org.apache.maven.shared.utils.StringUtils;
import org.junit.Assert;
Expand All @@ -32,13 +31,25 @@
* Test of {@link PrettyPrintXMLWriter}
*
* @author <a href="mailto:[email protected]">Vincent Siveton</a>
*
*/
public class PrettyPrintXmlWriterTest
{
private StringWriter w = new StringWriter();
private StringWriter w = new StringWriter();;
private PrettyPrintXMLWriter writer = new PrettyPrintXMLWriter( w );

@Test
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 use the expected property in the annotation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Expected was a bad design now replaced in JUnit 4.13 with assertThrows. The problem is that @Expected captures unexpected exceptions thrown from different parts of the method body and thus hides test failures.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use ExpectedException which is the Rule in JUnit4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same issue. This is not a full replacement for the pattern below and risks false negatives. See https://github.com/junit-team/junit4/wiki/Exception-testing

Copy link
Contributor

Choose a reason for hiding this comment

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

@elharo
I am a practicle man. You can of course use [1] with anonymous class instead of Lambda but you can use a traditional ExpectedException with the old version of JUnit.
[1]: https://junit.org/junit4/javadoc/4.13/org/junit/Assert.html#assertThrows(java.lang.String,%20java.lang.Class,%20org.junit.function.ThrowingRunnable)

import org.junit.rules.ExpectedException;

import static org.hamcrest.Matchers.equalTo;

    @Rule
    public final ExpectedException e = ExpectedException.none();

    @Test
    public void testNoStartTag() throws IOException
    {
        e.expect( IllegalArgumentException.class );
        e.expectMessage( equalTo( "Element name cannot be empty" ) );
        writer.startElement( "" );
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That has the same problem. It does not guarantee the expected exception was thrown by the right method. JUnit has rightly backed away from this style of testing exceptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

@elharo
In practice the last line with main code must be followed by e.expect. So yes there is guarantee. It is clear that only that one line has to throw the exception. If it does not, then the ExpectedException fails the build. That's why it was designed in JUnit4. You can use the new assertThrows but it has another meaning - wrapping the exception and checking a new errors or exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This works and it follows the recommendation of the JUnit Wiki and the design of JUnit from 4.13 onward. I'd like to move ahead with fixing this bug. Can I get an approval for this?

public void testNoStartTag() throws IOException
{

try {
writer.startElement( "" );
Assert.fail( "allowed empty name" );
} catch ( IllegalArgumentException ex ) {
Assert.assertEquals( "Element name cannot be empty", ex.getMessage() );
}

}

@Test
public void testDefaultPrettyPrintXMLWriter() throws IOException
{
Expand Down