diff --git a/CHANGES b/CHANGES index 63a5861dd3..d234fc607a 100644 --- a/CHANGES +++ b/CHANGES @@ -4,6 +4,11 @@ Release 1.15.3 [PENDING] * Improvement: the Cleaner will preserve the source position of cleaned elements, if source tracking is enabled in the original parse. + * Improvement: the error messages output from Validate are more descriptive. Exceptions are now ValidationExceptions + (extending IllegalArgumentException). Stack traces do not include the Validate class, to make it simpler to see + where the exception originated. Common validation errors including malformed URLs and empty selector results have + more explicit error messages. + * Bugfix: the DataUtil would incorrectly read from InputStreams that emitted reads less than the requested size. This lead to incorrect results when parsing from chunked server responses, for e.g. diff --git a/src/main/java/org/jsoup/helper/HttpConnection.java b/src/main/java/org/jsoup/helper/HttpConnection.java index b99807302d..9cc6bc1414 100644 --- a/src/main/java/org/jsoup/helper/HttpConnection.java +++ b/src/main/java/org/jsoup/helper/HttpConnection.java @@ -179,11 +179,11 @@ public Connection url(URL url) { } public Connection url(String url) { - Validate.notEmpty(url, "Must supply a valid URL"); + Validate.notEmptyParam(url, "url"); try { req.url(new URL(encodeUrl(url))); } catch (MalformedURLException e) { - throw new IllegalArgumentException("Malformed URL: " + url, e); + throw new IllegalArgumentException(String.format("The supplied URL, '%s', is malformed. Make sure it is an absolute URL, and starts with 'http://' or 'https://'.", url), e); } return this; } @@ -199,7 +199,7 @@ public Connection proxy(String host, int port) { } public Connection userAgent(String userAgent) { - Validate.notNull(userAgent, "User agent must not be null"); + Validate.notNullParam(userAgent, "userAgent"); req.header(USER_AGENT, userAgent); return this; } @@ -220,7 +220,7 @@ public Connection followRedirects(boolean followRedirects) { } public Connection referrer(String referrer) { - Validate.notNull(referrer, "Referrer must not be null"); + Validate.notNullParam(referrer, "referrer"); req.header("Referer", referrer); return this; } @@ -263,7 +263,7 @@ public Connection data(String key, String filename, InputStream inputStream, Str } public Connection data(Map data) { - Validate.notNull(data, "Data map must not be null"); + Validate.notNullParam(data, "data"); for (Map.Entry entry : data.entrySet()) { req.data(KeyVal.create(entry.getKey(), entry.getValue())); } @@ -271,7 +271,7 @@ public Connection data(Map data) { } public Connection data(String... keyvals) { - Validate.notNull(keyvals, "Data key value pairs must not be null"); + Validate.notNullParam(keyvals, "keyvals"); Validate.isTrue(keyvals.length %2 == 0, "Must supply an even number of key value pairs"); for (int i = 0; i < keyvals.length; i += 2) { String key = keyvals[i]; @@ -284,7 +284,7 @@ public Connection data(String... keyvals) { } public Connection data(Collection data) { - Validate.notNull(data, "Data collection must not be null"); + Validate.notNullParam(data, "data"); for (Connection.KeyVal entry: data) { req.data(entry); } @@ -292,7 +292,7 @@ public Connection data(Collection data) { } public Connection.KeyVal data(String key) { - Validate.notEmpty(key, "Data key must not be empty"); + Validate.notEmptyParam(key, "key"); for (Connection.KeyVal keyVal : request().data()) { if (keyVal.key().equals(key)) return keyVal; @@ -311,7 +311,7 @@ public Connection header(String name, String value) { } public Connection headers(Map headers) { - Validate.notNull(headers, "Header map must not be null"); + Validate.notNullParam(headers, "headers"); for (Map.Entry entry : headers.entrySet()) { req.header(entry.getKey(),entry.getValue()); } @@ -324,7 +324,7 @@ public Connection cookie(String name, String value) { } public Connection cookies(Map cookies) { - Validate.notNull(cookies, "Cookie map must not be null"); + Validate.notNullParam(cookies, "cookies"); for (Map.Entry entry : cookies.entrySet()) { req.cookie(entry.getKey(), entry.getValue()); } @@ -432,7 +432,7 @@ public URL url() { } public T url(URL url) { - Validate.notNull(url, "URL must not be null"); + Validate.notNullParam(url, "url"); this.url = punyUrl(url); // if calling url(url) directly, does not go through encodeUrl, so we punycode it explicitly. todo - should we encode here as well? return (T) this; } @@ -442,13 +442,13 @@ public Method method() { } public T method(Method method) { - Validate.notNull(method, "Method must not be null"); + Validate.notNullParam(method, "method"); this.method = method; return (T) this; } public String header(String name) { - Validate.notNull(name, "Header name must not be null"); + Validate.notNullParam(name, "name"); List vals = getHeadersCaseInsensitive(name); if (vals.size() > 0) { // https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2 @@ -460,7 +460,7 @@ public String header(String name) { @Override public T addHeader(String name, String value) { - Validate.notEmpty(name); + Validate.notEmptyParam(name, "name"); //noinspection ConstantConditions value = value == null ? "" : value; @@ -476,7 +476,7 @@ public T addHeader(String name, String value) { @Override public List headers(String name) { - Validate.notEmpty(name); + Validate.notEmptyParam(name, "name"); return getHeadersCaseInsensitive(name); } @@ -530,14 +530,14 @@ private static boolean looksLikeUtf8(byte[] input) { } public T header(String name, String value) { - Validate.notEmpty(name, "Header name must not be empty"); + Validate.notEmptyParam(name, "name"); removeHeader(name); // ensures we don't get an "accept-encoding" and a "Accept-Encoding" addHeader(name, value); return (T) this; } public boolean hasHeader(String name) { - Validate.notEmpty(name, "Header name must not be empty"); + Validate.notEmptyParam(name, "name"); return !getHeadersCaseInsensitive(name).isEmpty(); } @@ -556,8 +556,8 @@ public boolean hasHeaderWithValue(String name, String value) { } public T removeHeader(String name) { - Validate.notEmpty(name, "Header name must not be empty"); - Map.Entry> entry = scanHeaders(name); // remove is case insensitive too + Validate.notEmptyParam(name, "name"); + Map.Entry> entry = scanHeaders(name); // remove is case-insensitive too if (entry != null) headers.remove(entry.getKey()); // ensures correct case return (T) this; @@ -600,24 +600,24 @@ private List getHeadersCaseInsensitive(String name) { } public String cookie(String name) { - Validate.notEmpty(name, "Cookie name must not be empty"); + Validate.notEmptyParam(name, "name"); return cookies.get(name); } public T cookie(String name, String value) { - Validate.notEmpty(name, "Cookie name must not be empty"); - Validate.notNull(value, "Cookie value must not be null"); + Validate.notEmptyParam(name, "name"); + Validate.notNullParam(value, "value"); cookies.put(name, value); return (T) this; } public boolean hasCookie(String name) { - Validate.notEmpty(name, "Cookie name must not be empty"); + Validate.notEmptyParam(name, "name"); return cookies.containsKey(name); } public T removeCookie(String name) { - Validate.notEmpty(name, "Cookie name must not be empty"); + Validate.notEmptyParam(name, "name"); cookies.remove(name); return (T) this; } @@ -749,7 +749,7 @@ public Connection.Request ignoreContentType(boolean ignoreContentType) { } public Request data(Connection.KeyVal keyval) { - Validate.notNull(keyval, "Key val must not be null"); + Validate.notNullParam(keyval, "keyval"); data.add(keyval); return this; } @@ -778,7 +778,7 @@ public Parser parser() { } public Connection.Request postDataCharset(String charset) { - Validate.notNull(charset, "Charset must not be null"); + Validate.notNullParam(charset, "charset"); if (!Charset.isSupported(charset)) throw new IllegalCharsetNameException(charset); this.postDataCharset = charset; return this; @@ -834,7 +834,7 @@ static Response execute(HttpConnection.Request req, @Nullable Response previousR Validate.isFalse(req.executing, "Multiple threads were detected trying to execute the same request concurrently. Make sure to use Connection#newRequest() and do not share an executing request between threads."); req.executing = true; } - Validate.notNull(req, "Request must not be null"); + Validate.notNullParam(req, "req"); URL url = req.url(); Validate.notNull(url, "URL must be specified to connect"); String protocol = url.getProtocol(); @@ -1275,14 +1275,14 @@ public static KeyVal create(String key, String filename, InputStream stream) { } private KeyVal(String key, String value) { - Validate.notEmpty(key, "Data key must not be empty"); - Validate.notNull(value, "Data value must not be null"); + Validate.notEmptyParam(key, "key"); + Validate.notNullParam(value, "value"); this.key = key; this.value = value; } public KeyVal key(String key) { - Validate.notEmpty(key, "Data key must not be empty"); + Validate.notEmptyParam(key, "key"); this.key = key; return this; } @@ -1292,7 +1292,7 @@ public String key() { } public KeyVal value(String value) { - Validate.notNull(value, "Data value must not be null"); + Validate.notNullParam(value, "value"); this.value = value; return this; } @@ -1302,7 +1302,7 @@ public String value() { } public KeyVal inputStream(InputStream inputStream) { - Validate.notNull(value, "Data input stream must not be null"); + Validate.notNullParam(value, "inputStream"); this.stream = inputStream; return this; } diff --git a/src/main/java/org/jsoup/helper/Validate.java b/src/main/java/org/jsoup/helper/Validate.java index 3aaa3c802e..656c9114be 100644 --- a/src/main/java/org/jsoup/helper/Validate.java +++ b/src/main/java/org/jsoup/helper/Validate.java @@ -3,7 +3,7 @@ import javax.annotation.Nullable; /** - * Simple validation methods. Designed for jsoup internal use. + * Validators to check that method arguments meet expectations. */ public final class Validate { @@ -12,22 +12,34 @@ private Validate() {} /** * Validates that the object is not null * @param obj object to test - * @throws IllegalArgumentException if the object is null + * @throws ValidationException if the object is null */ public static void notNull(@Nullable Object obj) { if (obj == null) - throw new IllegalArgumentException("Object must not be null"); + throw new ValidationException("Object must not be null"); + } + + /** + Validates that the parameter is not null + + * @param obj the parameter to test + * @param param the name of the parameter, for presentation in the validation exception. + * @throws ValidationException if the object is null + */ + public static void notNullParam(@Nullable final Object obj, final String param) { + if (obj == null) + throw new ValidationException(String.format("The parameter '%s' must not be null.", param)); } /** * Validates that the object is not null * @param obj object to test * @param msg message to include in the Exception if validation fails - * @throws IllegalArgumentException if the object is null + * @throws ValidationException if the object is null */ public static void notNull(@Nullable Object obj, String msg) { if (obj == null) - throw new IllegalArgumentException(msg); + throw new ValidationException(msg); } /** @@ -35,60 +47,75 @@ public static void notNull(@Nullable Object obj, String msg) { null object. (Works around lack of Objects.requestNonNull in Android version.) * @param obj nullable object to case to not-null * @return the object, or throws an exception if it is null - * @throws IllegalArgumentException if the object is null + * @throws ValidationException if the object is null */ public static Object ensureNotNull(@Nullable Object obj) { if (obj == null) - throw new IllegalArgumentException("Object must not be null"); + throw new ValidationException("Object must not be null"); + else return obj; + } + + /** + Verifies the input object is not null, and returns that object. Effectively this casts a nullable object to a non- + null object. (Works around lack of Objects.requestNonNull in Android version.) + * @param obj nullable object to case to not-null + * @param msg the String format message to include in the validation exception when thrown + * @param args the arguments to the msg + * @return the object, or throws an exception if it is null + * @throws ValidationException if the object is null + */ + public static Object ensureNotNull(@Nullable Object obj, String msg, Object... args) { + if (obj == null) + throw new ValidationException(String.format(msg, args)); else return obj; } /** * Validates that the value is true * @param val object to test - * @throws IllegalArgumentException if the object is not true + * @throws ValidationException if the object is not true */ public static void isTrue(boolean val) { if (!val) - throw new IllegalArgumentException("Must be true"); + throw new ValidationException("Must be true"); } /** * Validates that the value is true * @param val object to test * @param msg message to include in the Exception if validation fails - * @throws IllegalArgumentException if the object is not true + * @throws ValidationException if the object is not true */ public static void isTrue(boolean val, String msg) { if (!val) - throw new IllegalArgumentException(msg); + throw new ValidationException(msg); } /** * Validates that the value is false * @param val object to test - * @throws IllegalArgumentException if the object is not false + * @throws ValidationException if the object is not false */ public static void isFalse(boolean val) { if (val) - throw new IllegalArgumentException("Must be false"); + throw new ValidationException("Must be false"); } /** * Validates that the value is false * @param val object to test * @param msg message to include in the Exception if validation fails - * @throws IllegalArgumentException if the object is not false + * @throws ValidationException if the object is not false */ public static void isFalse(boolean val, String msg) { if (val) - throw new IllegalArgumentException(msg); + throw new ValidationException(msg); } /** * Validates that the array contains no null elements * @param objects the array to test - * @throws IllegalArgumentException if the array contains a null element + * @throws ValidationException if the array contains a null element */ public static void noNullElements(Object[] objects) { noNullElements(objects, "Array must not contain any null objects"); @@ -98,33 +125,44 @@ public static void noNullElements(Object[] objects) { * Validates that the array contains no null elements * @param objects the array to test * @param msg message to include in the Exception if validation fails - * @throws IllegalArgumentException if the array contains a null element + * @throws ValidationException if the array contains a null element */ public static void noNullElements(Object[] objects, String msg) { for (Object obj : objects) if (obj == null) - throw new IllegalArgumentException(msg); + throw new ValidationException(msg); } /** * Validates that the string is not null and is not empty * @param string the string to test - * @throws IllegalArgumentException if the string is null or empty + * @throws ValidationException if the string is null or empty */ public static void notEmpty(@Nullable String string) { if (string == null || string.length() == 0) - throw new IllegalArgumentException("String must not be empty"); + throw new ValidationException("String must not be empty"); + } + + /** + Validates that the string parameter is not null and is not empty + * @param string the string to test + * @param param the name of the parameter, for presentation in the validation exception. + * @throws ValidationException if the string is null or empty + */ + public static void notEmptyParam(@Nullable final String string, final String param) { + if (string == null || string.length() == 0) + throw new ValidationException(String.format("The '%s' parameter must not be empty.", param)); } /** * Validates that the string is not null and is not empty * @param string the string to test * @param msg message to include in the Exception if validation fails - * @throws IllegalArgumentException if the string is null or empty + * @throws ValidationException if the string is null or empty */ public static void notEmpty(@Nullable String string, String msg) { if (string == null || string.length() == 0) - throw new IllegalArgumentException(msg); + throw new ValidationException(msg); } /** @@ -142,6 +180,6 @@ public static void wtf(String msg) { @throws IllegalStateException if we reach this state */ public static void fail(String msg) { - throw new IllegalArgumentException(msg); + throw new ValidationException(msg); } } diff --git a/src/main/java/org/jsoup/helper/ValidationException.java b/src/main/java/org/jsoup/helper/ValidationException.java new file mode 100644 index 0000000000..916cf12889 --- /dev/null +++ b/src/main/java/org/jsoup/helper/ValidationException.java @@ -0,0 +1,34 @@ +package org.jsoup.helper; + +import java.util.ArrayList; +import java.util.List; + +/** + Validation exceptions, as thrown by the methods in {@link Validate}. + */ +public class ValidationException extends IllegalArgumentException { + + public static final String Validator = Validate.class.getName(); + + public ValidationException(String msg) { + super(msg); + } + + @Override + public synchronized Throwable fillInStackTrace() { + // Filters out the Validate class from the stacktrace, to more clearly point at the root-cause. + + super.fillInStackTrace(); + + StackTraceElement[] stackTrace = getStackTrace(); + List filteredTrace = new ArrayList<>(); + for (StackTraceElement trace : stackTrace) { + if (trace.getClassName().equals(Validator)) continue; + filteredTrace.add(trace); + } + + setStackTrace(filteredTrace.toArray(new StackTraceElement[0])); + + return this; + } +} diff --git a/src/main/java/org/jsoup/helper/W3CDom.java b/src/main/java/org/jsoup/helper/W3CDom.java index f300b400c1..8caf31f525 100644 --- a/src/main/java/org/jsoup/helper/W3CDom.java +++ b/src/main/java/org/jsoup/helper/W3CDom.java @@ -269,8 +269,8 @@ public NodeList selectXpath(String xpath, Document doc) { @return the matches nodes */ public NodeList selectXpath(String xpath, Node contextNode) { - Validate.notEmpty(xpath); - Validate.notNull(contextNode); + Validate.notEmptyParam(xpath, "xpath"); + Validate.notNullParam(contextNode, "contextNode"); NodeList nodeList; try { diff --git a/src/main/java/org/jsoup/nodes/Element.java b/src/main/java/org/jsoup/nodes/Element.java index 016dac13bd..eed137ac42 100644 --- a/src/main/java/org/jsoup/nodes/Element.java +++ b/src/main/java/org/jsoup/nodes/Element.java @@ -172,7 +172,7 @@ public String normalName() { * @see Elements#tagName(String) */ public Element tagName(String tagName) { - Validate.notEmpty(tagName, "Tag name must not be empty."); + Validate.notEmptyParam(tagName, "tagName"); tag = Tag.valueOf(tagName, NodeUtils.parser(this).settings()); // maintains the case option of the original parse return this; } @@ -468,7 +468,13 @@ public Elements select(Evaluator evaluator) { @since 1.15.2 */ public Element expectFirst(String cssQuery) { - return (Element) Validate.ensureNotNull(Selector.selectFirst(cssQuery, this)); + return (Element) Validate.ensureNotNull( + Selector.selectFirst(cssQuery, this), + parent() != null ? + "No elements matched the query '%s' on element '%s'.": + "No elements matched the query '%s' in the document." + , cssQuery, this.tagName() + ); } /** diff --git a/src/main/java/org/jsoup/parser/TreeBuilder.java b/src/main/java/org/jsoup/parser/TreeBuilder.java index 902cbdf0ae..77083b2ea0 100644 --- a/src/main/java/org/jsoup/parser/TreeBuilder.java +++ b/src/main/java/org/jsoup/parser/TreeBuilder.java @@ -37,8 +37,8 @@ abstract class TreeBuilder { @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.notNullParam(input, "input"); + Validate.notNullParam(baseUri, "baseUri"); Validate.notNull(parser); doc = new Document(baseUri); diff --git a/src/main/java/org/jsoup/select/QueryParser.java b/src/main/java/org/jsoup/select/QueryParser.java index 3b2b735696..bed380db7b 100644 --- a/src/main/java/org/jsoup/select/QueryParser.java +++ b/src/main/java/org/jsoup/select/QueryParser.java @@ -360,7 +360,7 @@ private int consumeIndex() { private void has() { tq.consume(":has"); String subQuery = tq.chompBalanced('(', ')'); - Validate.notEmpty(subQuery, ":has(selector) subselect must not be empty"); + Validate.notEmpty(subQuery, ":has(selector) sub-select must not be empty"); evals.add(new StructuralEvaluator.Has(parse(subQuery))); } diff --git a/src/test/java/org/jsoup/helper/HttpConnectionTest.java b/src/test/java/org/jsoup/helper/HttpConnectionTest.java index 3ed9cf9f6c..4bd7dfb502 100644 --- a/src/test/java/org/jsoup/helper/HttpConnectionTest.java +++ b/src/test/java/org/jsoup/helper/HttpConnectionTest.java @@ -9,7 +9,13 @@ import java.io.IOException; import java.net.MalformedURLException; import java.net.URL; -import java.util.*; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; import static org.junit.jupiter.api.Assertions.*; @@ -293,4 +299,15 @@ public void caseInsensitiveHeaders(Locale locale) { } assertTrue(urlThrew); } + + @Test void testMalformedException() { + boolean threw = false; + try { + Jsoup.connect("jsoup.org/test"); + } catch (IllegalArgumentException e) { + threw = true; + assertEquals("The supplied URL, 'jsoup.org/test', is malformed. Make sure it is an absolute URL, and starts with 'http://' or 'https://'.", e.getMessage()); + } + assertTrue(threw); + } } diff --git a/src/test/java/org/jsoup/helper/ValidateTest.java b/src/test/java/org/jsoup/helper/ValidateTest.java index 138e532420..c7a093c358 100644 --- a/src/test/java/org/jsoup/helper/ValidateTest.java +++ b/src/test/java/org/jsoup/helper/ValidateTest.java @@ -3,8 +3,9 @@ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; -public class ValidateTest { +import static org.junit.jupiter.api.Assertions.*; +public class ValidateTest { @Test public void testNotNull() { Validate.notNull("foo"); @@ -16,4 +17,30 @@ public void testNotNull() { } Assertions.assertTrue(threw); } + + @Test void stacktraceFiltersOutValidateClass() { + boolean threw = false; + try { + Validate.notNull(null); + } catch (ValidationException e) { + threw = true; + assertEquals("Object must not be null", e.getMessage()); + StackTraceElement[] stackTrace = e.getStackTrace(); + for (StackTraceElement trace : stackTrace) { + assertNotEquals(trace.getClassName(), Validate.class.getName()); + } + assertTrue(stackTrace.length >= 1); + } + Assertions.assertTrue(threw); + } + + @Test void nonnullParam() { + boolean threw = true; + try { + Validate.notNullParam(null, "foo"); + } catch (ValidationException e) { + assertEquals("The parameter 'foo' must not be null.", e.getMessage()); + } + assertTrue(threw); + } } diff --git a/src/test/java/org/jsoup/nodes/ElementTest.java b/src/test/java/org/jsoup/nodes/ElementTest.java index 1f92eb11d1..0f51dc3ff8 100644 --- a/src/test/java/org/jsoup/nodes/ElementTest.java +++ b/src/test/java/org/jsoup/nodes/ElementTest.java @@ -2,6 +2,7 @@ import org.jsoup.Jsoup; import org.jsoup.TextUtil; +import org.jsoup.helper.ValidationException; import org.jsoup.internal.StringUtil; import org.jsoup.parser.ParseSettings; import org.jsoup.parser.Parser; @@ -2272,6 +2273,32 @@ void prettySerializationRoundTrips(Document.OutputSettings settings) { assertTrue(threw); } + @Test void testExpectFirstMessage() { + Document doc = Jsoup.parse("

One

Two Three Four"); + boolean threw = false; + Element p = doc.expectFirst("P"); + try { + Element span = p.expectFirst("span.doesNotExist"); + } catch (ValidationException e) { + threw = true; + assertEquals("No elements matched the query 'span.doesNotExist' on element 'p'.", e.getMessage()); + } + assertTrue(threw); + } + + @Test void testExpectFirstMessageDoc() { + Document doc = Jsoup.parse("

One

Two Three Four"); + boolean threw = false; + Element p = doc.expectFirst("P"); + try { + Element span = doc.expectFirst("span.doesNotExist"); + } catch (ValidationException e) { + threw = true; + assertEquals("No elements matched the query 'span.doesNotExist' in the document.", e.getMessage()); + } + assertTrue(threw); + } + @Test void spanRunsMaintainSpace() { // https://github.com/jhy/jsoup/issues/1787 Document doc = Jsoup.parse("

One\nTwo\nThree

");