GNOME.org
 

[libsoup] soup-uri: revert some of the previously-added return-if-fails

commit 6a8814e232f4c1e8f1bc4bf31f6899c1968dfe5e
Author: Dan Winship <danw gnome org>
Date:   Thu Feb 9 09:35:00 2012 -0500

    soup-uri: revert some of the previously-added return-if-fails
    
    Although it has always been documented that a SoupURI must have a
    non-NULL path, nothing ever enforced this, and most methods checked
    whether it was NULL before looking at it anyway. So lots of existing
    code was getting this wrong, and is now breaking because of the
    "g_return_if_fail (SOUP_URI_IS_VALID (uri))" checks.
    
    So, change most of those to just g_warn_if_fail() (while adding back
    the old return-if-fail !NULL checks), but also fix soup_uri_set_path()
    and soup_uri_new_with_base() to handle NULL paths more sanely (after
    warning). Also, allow calling the getters on invalid URIs.
    
    Add a new test to uri-testing to make sure that URIs created with
    soup_uri_new(NULL) behave as expected at each step of the way...
    
    https://bugzilla.gnome.org/show_bug.cgi?id=667637

 libsoup/soup-uri.c  |   64 +++++++++++++++++++++-----------
 tests/uri-parsing.c |  102 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 144 insertions(+), 22 deletions(-)
---
diff --git a/libsoup/soup-uri.c b/libsoup/soup-uri.c
index 7fbea77..bda843d 100644
--- a/libsoup/soup-uri.c
+++ b/libsoup/soup-uri.c
@@ -157,15 +157,25 @@ soup_scheme_default_port (const char *scheme)
 SoupURI *
 soup_uri_new_with_base (SoupURI *base, const char *uri_string)
 {
-	SoupURI *uri;
+	SoupURI *uri, fixed_base;
 	const char *end, *hash, *colon, *at, *path, *question;
 	const char *p, *hostend;
 	gboolean remove_dot_segments = TRUE;
 	int len;
 
-	g_return_val_if_fail (base == NULL || SOUP_URI_IS_VALID (base), NULL);
 	g_return_val_if_fail (uri_string != NULL, NULL);
 
+	/* Allow a %NULL path in @base, for compatibility */
+	if (base && base->scheme && !base->path) {
+		g_warn_if_fail (SOUP_URI_IS_VALID (base));
+
+		memcpy (&fixed_base, base, sizeof (SoupURI));
+		fixed_base.path = "";
+		base = &fixed_base;
+	}
+
+	g_return_val_if_fail (base == NULL || SOUP_URI_IS_VALID (base), NULL);
+
 	/* First some cleanup steps (which are supposed to all be no-ops,
 	 * but...). Skip initial whitespace, strip out internal tabs and
 	 * line breaks, and ignore trailing whitespace.
@@ -447,7 +457,8 @@ soup_uri_to_string (SoupURI *uri, gboolean just_path_and_query)
 	GString *str;
 	char *return_result;
 
-	g_return_val_if_fail (SOUP_URI_IS_VALID (uri), NULL);
+	g_return_val_if_fail (uri != NULL, NULL);
+	g_warn_if_fail (SOUP_URI_IS_VALID (uri));
 
 	str = g_string_sized_new (20);
 
@@ -502,7 +513,8 @@ soup_uri_copy (SoupURI *uri)
 {
 	SoupURI *dup;
 
-	g_return_val_if_fail (SOUP_URI_IS_VALID (uri), NULL);
+	g_return_val_if_fail (uri != NULL, NULL);
+	g_warn_if_fail (SOUP_URI_IS_VALID (uri));
 
 	dup = g_slice_new0 (SoupURI);
 	dup->scheme   = uri->scheme;
@@ -539,8 +551,10 @@ parts_equal (const char *one, const char *two, gboolean insensitive)
 gboolean 
 soup_uri_equal (SoupURI *uri1, SoupURI *uri2)
 {
-	g_return_val_if_fail (SOUP_URI_IS_VALID (uri1), FALSE);
-	g_return_val_if_fail (SOUP_URI_IS_VALID (uri2), FALSE);
+	g_return_val_if_fail (uri1 != NULL, FALSE);
+	g_return_val_if_fail (uri2 != NULL, FALSE);
+	g_warn_if_fail (SOUP_URI_IS_VALID (uri1));
+	g_warn_if_fail (SOUP_URI_IS_VALID (uri2));
 
 	if (uri1->scheme != uri2->scheme                         ||
 	    uri1->port   != uri2->port                           ||
@@ -771,7 +785,7 @@ gboolean
 soup_uri_uses_default_port (SoupURI *uri)
 {
 	g_return_val_if_fail (uri != NULL, FALSE);
-	g_return_val_if_fail (SOUP_URI_IS_VALID (uri), FALSE);
+	g_warn_if_fail (SOUP_URI_IS_VALID (uri));
 
 	return uri->port == soup_scheme_default_port (uri->scheme);
 }
@@ -803,7 +817,7 @@ soup_uri_uses_default_port (SoupURI *uri)
 const char *
 soup_uri_get_scheme (SoupURI *uri)
 {
-	g_return_val_if_fail (SOUP_URI_IS_VALID (uri), NULL);
+	g_return_val_if_fail (uri != NULL, NULL);
 
 	return uri->scheme;
 }
@@ -839,7 +853,7 @@ soup_uri_set_scheme (SoupURI *uri, const char *scheme)
 const char *
 soup_uri_get_user (SoupURI *uri)
 {
-	g_return_val_if_fail (SOUP_URI_IS_VALID (uri), NULL);
+	g_return_val_if_fail (uri != NULL, NULL);
 
 	return uri->user;
 }
@@ -873,7 +887,7 @@ soup_uri_set_user (SoupURI *uri, const char *user)
 const char *
 soup_uri_get_password (SoupURI *uri)
 {
-	g_return_val_if_fail (SOUP_URI_IS_VALID (uri), NULL);
+	g_return_val_if_fail (uri != NULL, NULL);
 
 	return uri->password;
 }
@@ -907,7 +921,7 @@ soup_uri_set_password (SoupURI *uri, const char *password)
 const char *
 soup_uri_get_host (SoupURI *uri)
 {
-	g_return_val_if_fail (SOUP_URI_IS_VALID (uri), NULL);
+	g_return_val_if_fail (uri != NULL, NULL);
 
 	return uri->host;
 }
@@ -947,7 +961,7 @@ soup_uri_set_host (SoupURI *uri, const char *host)
 guint
 soup_uri_get_port (SoupURI *uri)
 {
-	g_return_val_if_fail (SOUP_URI_IS_VALID (uri), 0);
+	g_return_val_if_fail (uri != NULL, 0);
 
 	return uri->port;
 }
@@ -981,7 +995,7 @@ soup_uri_set_port (SoupURI *uri, guint port)
 const char *
 soup_uri_get_path (SoupURI *uri)
 {
-	g_return_val_if_fail (SOUP_URI_IS_VALID (uri), NULL);
+	g_return_val_if_fail (uri != NULL, NULL);
 
 	return uri->path;
 }
@@ -997,7 +1011,12 @@ void
 soup_uri_set_path (SoupURI *uri, const char *path)
 {
 	g_return_if_fail (uri != NULL);
-	g_return_if_fail (path != NULL);
+
+	/* We allow a NULL path for compatibility, but warn about it. */
+	if (!path) {
+		g_warn_if_fail (path != NULL);
+		path = "";
+	}
 
 	g_free (uri->path);
 	uri->path = g_strdup (path);
@@ -1016,7 +1035,7 @@ soup_uri_set_path (SoupURI *uri, const char *path)
 const char *
 soup_uri_get_query (SoupURI *uri)
 {
-	g_return_val_if_fail (SOUP_URI_IS_VALID (uri), NULL);
+	g_return_val_if_fail (uri != NULL, NULL);
 
 	return uri->query;
 }
@@ -1094,7 +1113,7 @@ soup_uri_set_query_from_fields (SoupURI    *uri,
 const char *
 soup_uri_get_fragment (SoupURI *uri)
 {
-	g_return_val_if_fail (SOUP_URI_IS_VALID (uri), NULL);
+	g_return_val_if_fail (uri != NULL, NULL);
 
 	return uri->fragment;
 }
@@ -1130,13 +1149,14 @@ soup_uri_copy_host (SoupURI *uri)
 {
 	SoupURI *dup;
 
-	g_return_val_if_fail (SOUP_URI_IS_VALID (uri), NULL);
+	g_return_val_if_fail (uri != NULL, NULL);
+	g_warn_if_fail (SOUP_URI_IS_VALID (uri));
 
 	dup = soup_uri_new (NULL);
 	dup->scheme = uri->scheme;
 	dup->host   = g_strdup (uri->host);
 	dup->port   = uri->port;
-	dup->path = g_strdup ("");
+	dup->path   = g_strdup ("");
 
 	return dup;
 }
@@ -1156,8 +1176,8 @@ soup_uri_host_hash (gconstpointer key)
 {
 	const SoupURI *uri = key;
 
-	g_return_val_if_fail (SOUP_URI_IS_VALID (uri), 0);
-	g_return_val_if_fail (uri->host != NULL, 0);
+	g_return_val_if_fail (uri != NULL && uri->host != NULL, 0);
+	g_warn_if_fail (SOUP_URI_IS_VALID (uri));
 
 	return GPOINTER_TO_UINT (uri->scheme) + uri->port +
 		soup_str_case_hash (uri->host);
@@ -1182,9 +1202,9 @@ soup_uri_host_equal (gconstpointer v1, gconstpointer v2)
 	const SoupURI *two = v2;
 
 	g_return_val_if_fail (one != NULL && two != NULL, one == two);
-	g_return_val_if_fail (SOUP_URI_IS_VALID (one), FALSE);
-	g_return_val_if_fail (SOUP_URI_IS_VALID (two), FALSE);
 	g_return_val_if_fail (one->host != NULL && two->host != NULL, one->host == two->host);
+	g_warn_if_fail (SOUP_URI_IS_VALID (one));
+	g_warn_if_fail (SOUP_URI_IS_VALID (two));
 
 	if (one->scheme != two->scheme)
 		return FALSE;
diff --git a/tests/uri-parsing.c b/tests/uri-parsing.c
index ab18170..5869ec6 100644
--- a/tests/uri-parsing.c
+++ b/tests/uri-parsing.c
@@ -355,6 +355,106 @@ do_uri (SoupURI *base_uri, const char *base_str,
 	return TRUE;
 }
 
+static void
+do_soup_uri_null_tests (void)
+{
+	SoupURI *uri, *uri2;
+	char *uri_string;
+
+	debug_printf (1, "\nsoup_uri_new (NULL)\n");
+	uri = soup_uri_new (NULL);
+	if (SOUP_URI_IS_VALID (uri) || SOUP_URI_VALID_FOR_HTTP (uri)) {
+		debug_printf (1, "  ERROR: soup_uri_new(NULL) returns valid URI?\n");
+		errors++;
+	}
+
+	/* This implicitly also verifies that none of these methods g_warn */
+	if (soup_uri_get_scheme (uri) ||
+	    soup_uri_get_user (uri) ||
+	    soup_uri_get_password (uri) ||
+	    soup_uri_get_host (uri) ||
+	    soup_uri_get_port (uri) ||
+	    soup_uri_get_path (uri) ||
+	    soup_uri_get_query (uri) ||
+	    soup_uri_get_fragment (uri)) {
+		debug_printf (1, "  ERROR: soup_uri_new(NULL) returns non-empty URI?\n");
+		errors++;
+	}
+
+	expect_warning = TRUE;
+	uri2 = soup_uri_new_with_base (uri, "/path");
+	if (uri2 || expect_warning) {
+		debug_printf (1, "  ERROR: soup_uri_new_with_base didn't fail on NULL URI?\n");
+		errors++;
+		expect_warning = FALSE;
+	}
+
+	expect_warning = TRUE;
+	uri_string = soup_uri_to_string (uri, FALSE);
+	if (expect_warning) {
+		debug_printf (1, "  ERROR: soup_uri_to_string didn't fail on NULL URI?\n");
+		errors++;
+		expect_warning = FALSE;
+	} else if (*uri_string) {
+		debug_printf (1, "  ERROR: soup_uri_to_string on NULL URI returned '%s'\n",
+			      uri_string);
+		errors++;
+	}
+	g_free (uri_string);
+
+	soup_uri_set_scheme (uri, SOUP_URI_SCHEME_HTTP);
+	if (SOUP_URI_IS_VALID (uri) || SOUP_URI_VALID_FOR_HTTP (uri)) {
+		debug_printf (1, "  ERROR: setting scheme on NULL URI makes it valid?\n");
+		errors++;
+	}
+
+	soup_uri_set_host (uri, "localhost");
+	if (SOUP_URI_IS_VALID (uri)) {
+		debug_printf (1, "  ERROR: setting scheme+host on NULL URI makes it valid?\n");
+		errors++;
+	}
+	if (SOUP_URI_VALID_FOR_HTTP (uri)) {
+		debug_printf (1, "  ERROR: setting scheme+host on NULL URI makes it valid for http?\n");
+		errors++;
+	}
+
+	expect_warning = TRUE;
+	uri2 = soup_uri_new_with_base (uri, "/path");
+	if (expect_warning) {
+		debug_printf (1, "  ERROR: soup_uri_new_with_base didn't warn on NULL+scheme URI?\n");
+		errors++;
+		expect_warning = FALSE;
+	} else if (!uri2) {
+		debug_printf (1, "  ERROR: soup_uri_new_with_base didn't fix path on NULL+scheme URI\n");
+		errors++;
+	} else
+		soup_uri_free (uri2);
+
+	expect_warning = TRUE;
+	soup_uri_set_path (uri, NULL);
+	if (expect_warning) {
+		debug_printf (1, "  ERROR: setting path to NULL doesn't warn\n");
+		errors++;
+		expect_warning = FALSE;
+	}
+	if (!uri->path || *uri->path) {
+		debug_printf (1, "  ERROR: setting path to NULL != \"\"\n");
+		errors++;
+		soup_uri_set_path (uri, "");
+	}
+
+	if (!SOUP_URI_IS_VALID (uri)) {
+		debug_printf (1, "  ERROR: setting scheme+path on NULL URI doesn't make it valid?\n");
+		errors++;
+	}
+	if (!SOUP_URI_VALID_FOR_HTTP (uri)) {
+		debug_printf (1, "  ERROR: setting scheme+host+path on NULL URI doesn't make it valid for http?\n");
+		errors++;
+	}
+
+	soup_uri_free (uri);
+}
+
 int
 main (int argc, char **argv)
 {
@@ -411,6 +511,8 @@ main (int argc, char **argv)
 		soup_uri_free (uri2);
 	}
 
+	do_soup_uri_null_tests ();
+
 	test_cleanup ();
 	return errors != 0;
 }
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]