From 466e0f0ac014e071c6c70cb94fc4dbdf38beef5b Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Thu, 9 Feb 2012 09:35:00 -0500 Subject: [PATCH] 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... --- libsoup/soup-uri.c | 62 ++++++++++++++++++++----------- tests/uri-parsing.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 142 insertions(+), 22 deletions(-) diff --git a/libsoup/soup-uri.c b/libsoup/soup-uri.c index d2d752a..a0158a9 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)); /* IF YOU CHANGE ANYTHING IN THIS FUNCTION, RUN * tests/uri-parsing AFTERWARD. @@ -506,7 +517,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; @@ -543,8 +555,8 @@ 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_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 || @@ -775,7 +787,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); } @@ -807,7 +819,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; } @@ -843,7 +855,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; } @@ -877,7 +889,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; } @@ -911,7 +923,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; } @@ -951,7 +963,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; } @@ -985,7 +997,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; } @@ -1001,7 +1013,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); @@ -1020,7 +1037,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; } @@ -1098,7 +1115,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; } @@ -1134,13 +1151,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; } @@ -1160,8 +1178,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); @@ -1186,9 +1204,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; } -- 1.7.7.6