-
Notifications
You must be signed in to change notification settings - Fork 218
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.
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
Undo JDK6 hack #311
Undo JDK6 hack #311
Conversation
e68d009
to
e484104
Compare
This basically reverts commit f7e0924 (and commit 74c6dd0). Signed-off-by: Sven Strickroth <[email protected]>
Signed-off-by: Sven Strickroth <[email protected]>
e484104
to
7a2659e
Compare
} | ||
|
||
@Test | ||
public static void testThatAutoCloseablesAreClosed() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any other tests that closing an autocloseable closes the underlying appendable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original code (to which I reverted), also had no tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this supposed to be used?
package org.owasp.html;
import java.io.Closeable;
import java.io.IOException;
import junit.framework.TestCase;
@SuppressWarnings("javadoc")
public class AutoCloseableHtmlStreamRendererTest extends TestCase {
public static void testBlup() throws Exception {
PolicyFactory policyBuilder = new HtmlPolicyBuilder()
.allowAttributes("href").onElements("a")
.allowStandardUrlProtocols()
.allowElements("a")
.toFactory();
AutoClosableAppender autoClosableAppender = new AutoClosableAppender();
assertFalse(autoClosableAppender.closed);
try (AutoCloseable renderer = (AutoCloseable)HtmlStreamRenderer.create(autoClosableAppender,
Handler.DO_NOTHING,
Handler.DO_NOTHING)) {
// Use the policy defined above to sanitize the HTML.
HtmlSanitizer.sanitize("<a href=http://example.com>A link</a>", policyBuilder.apply((HtmlStreamRenderer)renderer));
}
assertTrue(autoClosableAppender.closed);
}
private static class AutoClosableAppender implements Appendable, AutoCloseable {
public boolean closed = false;
@Override
public void close() throws Exception {
closed = true;
}
@Override
public Appendable append(CharSequence csq) throws IOException {
return this;
}
@Override
public Appendable append(CharSequence csq, int start, int end) throws IOException {
return this;
}
@Override
public Appendable append(char c) throws IOException {
return this;
}
}
}
try (AutoCloseableHtmlStreamRenderer renderer = (AutoCloseableHtmlStreamRenderer)HtmlStreamRenderer.create(autoClosableAppender, ..)) {
cannot be used by users, as AutoCloseableHtmlStreamRenderer
is package scoped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Java has intersection types but doesn't allow mentioning them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the intended usage, maybe we need another factory method and make the inner-classes public
Undo AutoClosable hack introduced for JDK6 compatibility.
This is safe to do, as it was added in JDK7 and the Java-HTML-Sanitizer now requires JDK9 (even requiring 8 would still be ok)
This basically reverts commit f7e0924 (and commit 74c6dd0).