From f0afa42c539ce5f50a8fc949d52e80c0a5b73e6f Mon Sep 17 00:00:00 2001 From: Jonathan Hedley Date: Fri, 18 Dec 2020 15:20:53 +1100 Subject: [PATCH 1/4] Added Kotlin for lang interrop testing --- pom.xml | 82 +++++++++++++++++-- .../kotlin/org/jsoup/integration/jsoupTest.kt | 14 ++++ 2 files changed, 91 insertions(+), 5 deletions(-) create mode 100644 src/test/kotlin/org/jsoup/integration/jsoupTest.kt diff --git a/pom.xml b/pom.xml index c7328e0519..315e73fbf0 100644 --- a/pom.xml +++ b/pom.xml @@ -31,8 +31,42 @@ https://jhy.io/ + + UTF-8 + + 1.4.21 + true + + + + + org.jetbrains.kotlin + kotlin-maven-plugin + ${kotlin.version} + + + test-compile + test-compile + + test-compile + + + + src/test/java + src/test/kotlin + + + + + + 1.8 + + -Xjsr305=strict + + + org.apache.maven.plugins maven-compiler-plugin @@ -48,6 +82,35 @@ false + + + + default-compile + none + + + + default-testCompile + none + + + java-compile + compile + + compile + + + + java-test-compile + test-compile + + testCompile + + + ${maven.test.skip} + + + @@ -311,7 +374,6 @@ test - org.eclipse.jetty @@ -320,6 +382,20 @@ test + + + org.jetbrains.kotlin + kotlin-stdlib-jdk8 + ${kotlin.version} + test + + + org.jetbrains.kotlin + kotlin-test + ${kotlin.version} + test + + @@ -327,10 +403,6 @@ - - UTF-8 - - jhy diff --git a/src/test/kotlin/org/jsoup/integration/jsoupTest.kt b/src/test/kotlin/org/jsoup/integration/jsoupTest.kt new file mode 100644 index 0000000000..29a678e3cd --- /dev/null +++ b/src/test/kotlin/org/jsoup/integration/jsoupTest.kt @@ -0,0 +1,14 @@ +package org.jsoup.integration + +import org.jsoup.Jsoup +import org.junit.jupiter.api.Assertions +import org.junit.jupiter.api.Test + +class JsoupTest { + @Test fun jsoupTest() { + val doc = Jsoup.parse("jsoup") + doc.body() // Odd to me that the Kotlin compiler doesn't NPE warn on this, as .parse() is !, which includes ?. + // And compiler / Intellij doesn't have a code inspect for it either. At least have it be an option + Assertions.assertEquals("jsoup", doc.body().html()) + } +} From 0d3f8be588890e68f229cc27f03a51d2e8735317 Mon Sep 17 00:00:00 2001 From: Jonathan Hedley Date: Sun, 20 Dec 2020 12:04:44 +1100 Subject: [PATCH 2/4] Added nullable annotation types, and annotated Jsoup.class --- pom.xml | 16 ++++++++++ src/main/java/org/jsoup/Jsoup.java | 13 +++++--- .../internal/ReturnsAreNonnullByDefault.java | 20 ++++++++++++ .../org/jsoup/parser/HtmlTreeBuilder.java | 3 +- .../java/org/jsoup/parser/TreeBuilder.java | 4 +++ .../java/org/jsoup/parser/XmlTreeBuilder.java | 3 +- .../java/org/jsoup/helper/ValidateTest.java | 19 ++++++++++++ .../org/jsoup/parser/HtmlTreeBuilderTest.java | 31 ++++++++++++++++++- .../kotlin/org/jsoup/integration/jsoupTest.kt | 6 ++-- 9 files changed, 106 insertions(+), 9 deletions(-) create mode 100644 src/main/java/org/jsoup/internal/ReturnsAreNonnullByDefault.java create mode 100644 src/test/java/org/jsoup/helper/ValidateTest.java diff --git a/pom.xml b/pom.xml index 315e73fbf0..4982aa5038 100644 --- a/pom.xml +++ b/pom.xml @@ -395,6 +395,22 @@ ${kotlin.version} test + + org.jetbrains.kotlin + kotlin-reflect + ${kotlin.version} + test + + + + + com.google.code.findbugs + jsr305 + 3.0.2 + compile + + + diff --git a/src/main/java/org/jsoup/Jsoup.java b/src/main/java/org/jsoup/Jsoup.java index 5fe628614f..066978a60c 100644 --- a/src/main/java/org/jsoup/Jsoup.java +++ b/src/main/java/org/jsoup/Jsoup.java @@ -2,12 +2,15 @@ import org.jsoup.helper.DataUtil; import org.jsoup.helper.HttpConnection; +import org.jsoup.internal.ReturnsAreNonnullByDefault; import org.jsoup.nodes.Document; import org.jsoup.parser.Parser; import org.jsoup.safety.Cleaner; import org.jsoup.safety.Safelist; import org.jsoup.safety.Whitelist; +import javax.annotation.Nullable; +import javax.annotation.ParametersAreNonnullByDefault; import java.io.File; import java.io.IOException; import java.io.InputStream; @@ -17,6 +20,8 @@ The core public access point to the jsoup functionality. @author Jonathan Hedley */ + +@ParametersAreNonnullByDefault @ReturnsAreNonnullByDefault public class Jsoup { private Jsoup() {} @@ -85,7 +90,7 @@ public static Connection connect(String url) { @throws IOException if the file could not be found, or read, or if the charsetName is invalid. */ - public static Document parse(File in, String charsetName, String baseUri) throws IOException { + public static Document parse(File in, @Nullable String charsetName, String baseUri) throws IOException { return DataUtil.load(in, charsetName, baseUri); } @@ -100,7 +105,7 @@ public static Document parse(File in, String charsetName, String baseUri) throws @throws IOException if the file could not be found, or read, or if the charsetName is invalid. @see #parse(File, String, String) */ - public static Document parse(File in, String charsetName) throws IOException { + public static Document parse(File in, @Nullable String charsetName) throws IOException { return DataUtil.load(in, charsetName, in.getAbsolutePath()); } @@ -115,7 +120,7 @@ public static Document parse(File in, String charsetName) throws IOException { @throws IOException if the file could not be found, or read, or if the charsetName is invalid. */ - public static Document parse(InputStream in, String charsetName, String baseUri) throws IOException { + public static Document parse(InputStream in, @Nullable String charsetName, String baseUri) throws IOException { return DataUtil.load(in, charsetName, baseUri); } @@ -132,7 +137,7 @@ public static Document parse(InputStream in, String charsetName, String baseUri) @throws IOException if the file could not be found, or read, or if the charsetName is invalid. */ - public static Document parse(InputStream in, String charsetName, String baseUri, Parser parser) throws IOException { + public static Document parse(InputStream in, @Nullable String charsetName, String baseUri, Parser parser) throws IOException { return DataUtil.load(in, charsetName, baseUri, parser); } diff --git a/src/main/java/org/jsoup/internal/ReturnsAreNonnullByDefault.java b/src/main/java/org/jsoup/internal/ReturnsAreNonnullByDefault.java new file mode 100644 index 0000000000..3f004a68a2 --- /dev/null +++ b/src/main/java/org/jsoup/internal/ReturnsAreNonnullByDefault.java @@ -0,0 +1,20 @@ +package org.jsoup.internal; + +import javax.annotation.Nonnull; +import javax.annotation.meta.TypeQualifierDefault; +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; + +@Documented +@Nonnull +@TypeQualifierDefault(ElementType.METHOD) +@Retention(value = RetentionPolicy.RUNTIME) + +/** + Indicates return types are not nullable, unless otherwise specified by @Nullable. + @see javax.annotation.ParametersAreNonnullByDefault + */ +public @interface ReturnsAreNonnullByDefault { +} diff --git a/src/main/java/org/jsoup/parser/HtmlTreeBuilder.java b/src/main/java/org/jsoup/parser/HtmlTreeBuilder.java index 39520ed5f7..cb398a2cb1 100644 --- a/src/main/java/org/jsoup/parser/HtmlTreeBuilder.java +++ b/src/main/java/org/jsoup/parser/HtmlTreeBuilder.java @@ -12,6 +12,7 @@ import org.jsoup.nodes.TextNode; import org.jsoup.select.Elements; +import javax.annotation.ParametersAreNonnullByDefault; import java.io.Reader; import java.io.StringReader; import java.util.ArrayList; @@ -60,7 +61,7 @@ ParseSettings defaultSettings() { return ParseSettings.htmlDefault; } - @Override + @Override @ParametersAreNonnullByDefault protected void initialiseParse(Reader input, String baseUri, Parser parser) { super.initialiseParse(input, baseUri, parser); diff --git a/src/main/java/org/jsoup/parser/TreeBuilder.java b/src/main/java/org/jsoup/parser/TreeBuilder.java index 5292705c17..5c1ecfbfba 100644 --- a/src/main/java/org/jsoup/parser/TreeBuilder.java +++ b/src/main/java/org/jsoup/parser/TreeBuilder.java @@ -6,6 +6,7 @@ import org.jsoup.nodes.Element; import org.jsoup.nodes.Node; +import javax.annotation.ParametersAreNonnullByDefault; import java.io.Reader; import java.util.ArrayList; import java.util.List; @@ -27,9 +28,11 @@ abstract class TreeBuilder { private Token.EndTag end = new Token.EndTag(); abstract ParseSettings defaultSettings(); + @ParametersAreNonnullByDefault protected void initialiseParse(Reader input, String baseUri, Parser parser) { Validate.notNull(input, "String input must not be null"); Validate.notNull(baseUri, "BaseURI must not be null"); + Validate.notNull(parser); doc = new Document(baseUri); doc.parser(parser); @@ -42,6 +45,7 @@ protected void initialiseParse(Reader input, String baseUri, Parser parser) { this.baseUri = baseUri; } + @ParametersAreNonnullByDefault Document parse(Reader input, String baseUri, Parser parser) { initialiseParse(input, baseUri, parser); runParser(); diff --git a/src/main/java/org/jsoup/parser/XmlTreeBuilder.java b/src/main/java/org/jsoup/parser/XmlTreeBuilder.java index d68156dc38..1be8bf1e42 100644 --- a/src/main/java/org/jsoup/parser/XmlTreeBuilder.java +++ b/src/main/java/org/jsoup/parser/XmlTreeBuilder.java @@ -10,6 +10,7 @@ import org.jsoup.nodes.TextNode; import org.jsoup.nodes.XmlDeclaration; +import javax.annotation.ParametersAreNonnullByDefault; import java.io.Reader; import java.io.StringReader; import java.util.List; @@ -26,7 +27,7 @@ ParseSettings defaultSettings() { return ParseSettings.preserveCase; } - @Override + @Override @ParametersAreNonnullByDefault protected void initialiseParse(Reader input, String baseUri, Parser parser) { super.initialiseParse(input, baseUri, parser); stack.add(doc); // place the document onto the stack. differs from HtmlTreeBuilder (not on stack) diff --git a/src/test/java/org/jsoup/helper/ValidateTest.java b/src/test/java/org/jsoup/helper/ValidateTest.java new file mode 100644 index 0000000000..138e532420 --- /dev/null +++ b/src/test/java/org/jsoup/helper/ValidateTest.java @@ -0,0 +1,19 @@ +package org.jsoup.helper; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class ValidateTest { + + @Test + public void testNotNull() { + Validate.notNull("foo"); + boolean threw = false; + try { + Validate.notNull(null); + } catch (IllegalArgumentException e) { + threw = true; + } + Assertions.assertTrue(threw); + } +} diff --git a/src/test/java/org/jsoup/parser/HtmlTreeBuilderTest.java b/src/test/java/org/jsoup/parser/HtmlTreeBuilderTest.java index 04630dc990..086a1c2eb6 100644 --- a/src/test/java/org/jsoup/parser/HtmlTreeBuilderTest.java +++ b/src/test/java/org/jsoup/parser/HtmlTreeBuilderTest.java @@ -3,9 +3,14 @@ import org.junit.jupiter.api.Test; +import javax.annotation.Nonnull; +import javax.annotation.ParametersAreNonnullByDefault; +import java.io.Reader; +import java.lang.annotation.Annotation; +import java.lang.reflect.Method; import java.util.Arrays; -import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.*; public class HtmlTreeBuilderTest { @Test @@ -26,4 +31,28 @@ public void ensureSearchArraysAreSorted() { assertArrayEquals(array, copy); } } + + @Test + public void nonnull() { + assertThrows(IllegalArgumentException.class, () -> { + HtmlTreeBuilder treeBuilder = new HtmlTreeBuilder(); + treeBuilder.parse(null, null, null); // not sure how to test that these visual warnings actually appear! - test below checks for method annotation + } + ); // I'm not convinced that this lambda is easier to read than the old Junit 4 @Test(expected=IEA.class)... + } + + @Test public void nonnullAssertions() throws NoSuchMethodException { + Method parseMethod = TreeBuilder.class.getDeclaredMethod("parse", Reader.class, String.class, Parser.class); + assertNotNull(parseMethod); + Annotation[] declaredAnnotations = parseMethod.getDeclaredAnnotations(); + boolean seen = false; + for (Annotation annotation : declaredAnnotations) { + if (annotation.annotationType().isAssignableFrom(ParametersAreNonnullByDefault.class)) + seen = true; + } + + // would need to rework this if/when that annotation moves from the method to the class / package. + assertTrue(seen); + + } } diff --git a/src/test/kotlin/org/jsoup/integration/jsoupTest.kt b/src/test/kotlin/org/jsoup/integration/jsoupTest.kt index 29a678e3cd..e944f40d08 100644 --- a/src/test/kotlin/org/jsoup/integration/jsoupTest.kt +++ b/src/test/kotlin/org/jsoup/integration/jsoupTest.kt @@ -5,9 +5,11 @@ import org.junit.jupiter.api.Assertions import org.junit.jupiter.api.Test class JsoupTest { - @Test fun jsoupTest() { + @Test + fun jsoupTest() { val doc = Jsoup.parse("jsoup") - doc.body() // Odd to me that the Kotlin compiler doesn't NPE warn on this, as .parse() is !, which includes ?. + doc.body() + // It's odd to me that (prior to adding notnulll annotations), the Kotlin compiler doesn't NPE warn on this, as .parse() is !, which includes ?. // And compiler / Intellij doesn't have a code inspect for it either. At least have it be an option Assertions.assertEquals("jsoup", doc.body().html()) } From 4fcb5cfc838923e91cda741fc431f91ffdfe289d Mon Sep 17 00:00:00 2001 From: Jonathan Hedley Date: Sun, 20 Dec 2020 12:08:55 +1100 Subject: [PATCH 3/4] Scrubbed out the Kotlin tests Wasn't getting value commesurate to the extra dependencies. --- pom.xml | 81 ------------------- .../kotlin/org/jsoup/integration/jsoupTest.kt | 16 ---- 2 files changed, 97 deletions(-) delete mode 100644 src/test/kotlin/org/jsoup/integration/jsoupTest.kt diff --git a/pom.xml b/pom.xml index 4982aa5038..7de140e3e1 100644 --- a/pom.xml +++ b/pom.xml @@ -33,40 +33,10 @@ UTF-8 - - 1.4.21 - true - - - org.jetbrains.kotlin - kotlin-maven-plugin - ${kotlin.version} - - - test-compile - test-compile - - test-compile - - - - src/test/java - src/test/kotlin - - - - - - 1.8 - - -Xjsr305=strict - - - org.apache.maven.plugins maven-compiler-plugin @@ -82,35 +52,6 @@ false - - - - default-compile - none - - - - default-testCompile - none - - - java-compile - compile - - compile - - - - java-test-compile - test-compile - - testCompile - - - ${maven.test.skip} - - - @@ -382,26 +323,6 @@ test - - - org.jetbrains.kotlin - kotlin-stdlib-jdk8 - ${kotlin.version} - test - - - org.jetbrains.kotlin - kotlin-test - ${kotlin.version} - test - - - org.jetbrains.kotlin - kotlin-reflect - ${kotlin.version} - test - - com.google.code.findbugs @@ -410,8 +331,6 @@ compile - - diff --git a/src/test/kotlin/org/jsoup/integration/jsoupTest.kt b/src/test/kotlin/org/jsoup/integration/jsoupTest.kt deleted file mode 100644 index e944f40d08..0000000000 --- a/src/test/kotlin/org/jsoup/integration/jsoupTest.kt +++ /dev/null @@ -1,16 +0,0 @@ -package org.jsoup.integration - -import org.jsoup.Jsoup -import org.junit.jupiter.api.Assertions -import org.junit.jupiter.api.Test - -class JsoupTest { - @Test - fun jsoupTest() { - val doc = Jsoup.parse("jsoup") - doc.body() - // It's odd to me that (prior to adding notnulll annotations), the Kotlin compiler doesn't NPE warn on this, as .parse() is !, which includes ?. - // And compiler / Intellij doesn't have a code inspect for it either. At least have it be an option - Assertions.assertEquals("jsoup", doc.body().html()) - } -} From 3c1b7fb35b14edfc795b2c0df4b983d67988843c Mon Sep 17 00:00:00 2001 From: Jonathan Hedley Date: Sun, 20 Dec 2020 12:52:29 +1100 Subject: [PATCH 4/4] Changelog for PR --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES b/CHANGES index 491fda12a2..610a268267 100644 --- a/CHANGES +++ b/CHANGES @@ -22,6 +22,9 @@ jsoup changelog * Build Improvement: updated Jetty (used for integration tests; not bundled) to 9.4.35.v2020112. + * Build Improvement: added nullability annotations and initial settings. + + * Bugfix: when parsing HTML, could throw NPEs on some tags (isindex or table>input).