Make URI::relative_to the strict inverse of URI::resolve_from

src/core/uri/accessors.cc

@@ -136,8 +136,11 @@ auto URI::userinfo() const -> std::optional<std::string_view> {
 }
 
 auto URI::has_same_authority(const URI &other) const noexcept -> bool {
+  // RFC 3986 Section 3.2.2 wraps an IP literal in brackets and writes every
+  // other host bare, so a host that reads the same either way still names a
+  // different authority depending on which form it takes
   return this->userinfo_ == other.userinfo_ && this->host_ == other.host_ &&
-         this->port_ == other.port_;
+         this->port_ == other.port_ && this->ip_literal_ == other.ip_literal_;
 }
 
 } // namespace sourcemeta::core

src/core/uri/include/sourcemeta/core/uri.h

@@ -656,22 +656,37 @@ class SOURCEMETA_CORE_URI_EXPORT URI {
   /// ```
   auto resolve_from(const URI &base) -> URI &;
 
-  /// Attempt to resolve a URI relative to another URI. If the latter URI is not
-  /// a base for the former, leave the URI intact. For example:
+  /// Express a URI as a relative reference against a base URI, such that
+  /// resolving the result against that base reproduces this URI:
+  ///
+  /// ```
+  /// resolve_from(relative_to(target, base), base) == target
+  /// ```
+  ///
+  /// That equation is the definition of a correct result, as RFC 3986 states
+  /// how to resolve a reference but never how to compute one. It holds when
+  /// both URIs are absolute and the target path carries no dot segments.
+  /// Resolution strips those from a reference that keeps its scheme just as it
+  /// does from a relative one, so a target carrying them is outside the
+  /// guarantee whether or not a reference gets built. Within those bounds, a
+  /// URI left intact because no reference expresses it satisfies the equation
+  /// as well. For example:
   ///
   /// ```cpp
   /// #include <sourcemeta/core/uri.h>
   /// #include <cassert>
   ///
-  /// const sourcemeta::core::URI base{"https://www.sourcemeta.com"};
+  /// const sourcemeta::core::URI base{"https://www.sourcemeta.com/"};
   /// sourcemeta::core::URI result{"https://www.sourcemeta.com/foo"};
   /// result.relative_to(base);
   /// assert(result.recompose() == "foo");
   /// ```
   auto relative_to(const URI &base) -> URI &;
 
-  /// Attempt to change the base of a URI. If the URI is not relative to
-  /// the former, leave the URI intact. For example:
+  /// Move a URI that lies under a base to the same position under a new base.
+  /// A URI that is neither the base nor under it is left intact, and so is one
+  /// that only shares a textual prefix without matching whole path segments.
+  /// For example:
   ///
   /// ```cpp
   /// #include <sourcemeta/core/uri.h>
@@ -685,9 +700,9 @@ class SOURCEMETA_CORE_URI_EXPORT URI {
   /// ```
   auto rebase(const URI &base, const URI &new_base) -> URI &;
 
-  /// Attempt to change the base of a URI, moving components out of
-  /// `new_base` rather than copying them. If the URI is not relative to
-  /// the former base, leave the URI intact. For example:
+  /// Move a URI that lies under a base to the same position under a new base,
+  /// taking components out of `new_base` rather than copying them. A URI that
+  /// is neither the base nor under it is left intact. For example:
   ///
   /// ```cpp
   /// #include <sourcemeta/core/uri.h>

src/core/uri/recompose.cc

@@ -144,25 +144,15 @@ auto append_disambiguated_path(std::string &output,
     const auto first_segment_length{first_slash == std::string_view::npos
                                         ? path_value.size()
                                         : first_slash};
-    const auto first_segment{path_value.substr(0, first_segment_length)};
-    if (first_segment.find(':') != std::string_view::npos) {
-      std::string encoded;
-      encoded.reserve(first_segment_length + 4);
-      for (const char character : first_segment) {
-        if (character == ':') {
-          encoded += "%3A";
-        } else {
-          encoded += character;
-        }
-      }
-
-      escape_component_to_string(output, encoded, URIEscapeMode::Path, iri);
-      if (first_slash != std::string_view::npos) {
-        escape_component_to_string(output, path_value.substr(first_slash),
-                                   URIEscapeMode::Path, iri);
-      }
-
-      return;
+    // RFC 3986 Section 4.2: "A path segment that contains a colon character
+    // cannot be used as the first segment of a relative-path reference, as it
+    // would be mistaken for a scheme name. Such a segment must be preceded by
+    // a dot-segment". Percent encoding the colon would avoid the same misparse
+    // but would name a different path, since Section 6.2.2.2 only equates
+    // percent-encoded unreserved characters
+    if (path_value.substr(0, first_segment_length).find(':') !=
+        std::string_view::npos) {
+      output += "./";
     }
   }
 

src/core/uri/resolution.cc

@@ -98,6 +98,10 @@ auto URI::resolve_from(const URI &base) -> URI & {
   this->userinfo_ = base.userinfo_;
   this->host_ = base.host_;
   this->port_ = base.port_;
+  // RFC 3986 Section 5.2.2 inherits the whole authority, and whether the host
+  // is an IP literal is part of it, as Section 3.2.2 only writes the enclosing
+  // brackets for that form
+  this->ip_literal_ = base.ip_literal_;
 
   // Reference has empty path
   if (!this->path_.has_value() || this->path_.value().empty()) {
@@ -137,29 +141,23 @@ auto URI::relative_to(const URI &base) -> URI & {
   }
 
   // The full authority must match (but components can be null for URNs)
-  if (this->userinfo_ != base.userinfo_) {
-    return *this;
-  }
-
-  if (this->host_ != base.host_) {
-    return *this;
-  }
-
-  if (this->port_ != base.port_) {
+  if (!this->has_same_authority(base)) {
     return *this;
   }
 
   // Special case: both URIs are exactly the same
   if (this->path_ == base.path_ && this->query_ == base.query_ &&
       this->fragment_ == base.fragment_) {
-    // Clear all components to make it empty relative URI
+    // Clear every component the base supplies back on resolution, which is
+    // everything but the fragment. RFC 3986 Section 5.2.2 always takes the
+    // fragment from the reference, as "T.fragment = R.fragment", so an empty
+    // reference names the base without one and has to keep it here
     this->scheme_.reset();
     this->userinfo_.reset();
     this->host_.reset();
     this->port_.reset();
     this->path_.reset();
     this->query_.reset();
-    this->fragment_.reset();
     return *this;
   }
 
@@ -227,43 +225,29 @@ auto URI::relative_to(const URI &base) -> URI & {
     return *this;
   }
 
-  // Case 2: Check if this_path starts with base_path followed by "/"
-  // This handles: base="/foo" and this="/foo/bar" = "bar"
-  // But NOT: base="/spec" and this="/spec/" (different resources)
-  // RFC 3986 Section 5.2.2 uses a reference path that starts with a slash as
-  // is, so a remainder that begins with one would drop the base prefix rather
-  // than name something below it
-  const std::string base_with_slash =
-      base_path.ends_with('/') ? base_path : base_path + "/";
-  if (this_path.starts_with(base_with_slash) &&
-      this_path.length() > base_with_slash.length() &&
-      this_path[base_with_slash.length()] != '/') {
-    auto relative_path = this_path.substr(base_with_slash.length());
-
-    this->scheme_.reset();
-    this->userinfo_.reset();
-    this->host_.reset();
-    this->port_.reset();
-    this->path_ = relative_path.empty()
-                      ? std::nullopt
-                      : std::optional<std::string>{relative_path};
-
-    return *this;
-  }
-
   // Find last slash positions (needed for multiple cases below)
   const auto base_last_slash = base_path.rfind('/');
   const auto this_last_slash = this_path.rfind('/');
 
-  // Case 3: Check if both paths share the same parent directory (siblings)
+  // RFC 3986 Section 5.2.3 merges against the base path with everything after
+  // its right-most slash excluded, and Section 5.2.2 removes dot segments only
+  // once that merge has happened, so what a reference is really measured
+  // against is that prefix after normalisation. Normalising the whole base
+  // path instead would be wrong, as a dot segment sitting after the last slash
+  // is dropped by the merge rather than applied
+  const auto base_anchor =
+      base_last_slash != std::string::npos
+          ? remove_dot_segments(base_path.substr(0, base_last_slash + 1))
+          : std::string{};
+
+  // Case 2: Check if both paths share the same parent directory (siblings)
   // This handles: base="/test/bar.json" and this="/test/foo.json" =
   // "foo.json"
   if (base_last_slash != std::string::npos &&
       this_last_slash != std::string::npos) {
-    const auto base_parent = base_path.substr(0, base_last_slash + 1);
     const auto this_parent = this_path.substr(0, this_last_slash + 1);
 
-    if (base_parent == this_parent) {
+    if (base_anchor == this_parent) {
       auto relative_path = this_path.substr(this_last_slash + 1);
 
       this->scheme_.reset();
@@ -280,17 +264,13 @@ auto URI::relative_to(const URI &base) -> URI & {
     }
   }
 
-  // Case 4: General case - compute relative path using .. segments
+  // Case 3: General case - compute relative path using .. segments
   // This handles cases like: base="/schemas/foo.json" and this="/bundling/bar"
   // Result should be "../bundling/bar"
   // Note: We don't make URIs relative if the target is just a shallow path
   // like "/foo" (only one level deep) as that's not meaningfully navigable
-  const auto base_parent = base_last_slash != std::string::npos
-                               ? base_path.substr(0, base_last_slash + 1)
-                               : base_path;
-
   std::string relative_path;
-  std::string current_base_parent{base_parent};
+  std::string current_base_parent{base_anchor};
 
   while (!current_base_parent.empty() && current_base_parent != "/") {
     if (this_path.starts_with(current_base_parent)) {
@@ -361,16 +341,14 @@ auto merge_new_base_path(std::optional<std::string> &target_path,
   if (new_base_path.has_value() && saved_path.has_value()) {
     auto merged{std::move(new_base_path.value())};
     const auto &relative_path = saved_path.value();
-    const auto base_ends_with_slash = merged.ends_with('/');
-    const auto relative_starts_with_slash = relative_path.starts_with('/');
-    if (base_ends_with_slash && relative_starts_with_slash) {
-      merged.append(relative_path, 1);
-    } else if (!base_ends_with_slash && !relative_starts_with_slash) {
+    // The suffix is what lies below the old base with the separating slash
+    // already removed, so a slash it does start with opens an empty segment
+    // and must not be mistaken for that separator
+    if (!merged.empty() && !merged.ends_with('/')) {
       merged += '/';
-      merged += relative_path;
-    } else {
-      merged += relative_path;
     }
+
+    merged += relative_path;
     target_path = std::move(merged);
   } else if (new_base_path.has_value()) {
     target_path = std::move(new_base_path);
@@ -379,59 +357,69 @@ auto merge_new_base_path(std::optional<std::string> &target_path,
   }
 }
 
+// The portion of a path that lies below a base, or no value when the URI is
+// neither the base nor under it. Component boundaries are respected, so a path
+// of "/foobar" does not lie under "/foo"
+auto path_under(const URI &uri, const URI &base) -> std::optional<std::string> {
+  if (uri.scheme() != base.scheme() || !uri.has_same_authority(base)) {
+    return std::nullopt;
+  }
+
+  return URI::strip_path_prefix(uri.path().value_or(""),
+                                base.path().value_or(""));
+}
+
 } // namespace
 
 auto URI::rebase(const URI &base, const URI &new_base) -> URI & {
-  this->relative_to(base);
-  if (!this->is_relative()) {
+  auto suffix{path_under(*this, base)};
+  if (!suffix.has_value()) {
     return *this;
   }
 
-  auto saved_path = std::move(this->path_);
-  auto saved_fragment = std::move(this->fragment_);
-  auto saved_query = std::move(this->query_);
+  std::optional<std::string> relative_path;
+  if (!suffix.value().empty()) {
+    relative_path = std::move(suffix.value());
+  }
 
   this->scheme_ = new_base.scheme_;
   this->userinfo_ = new_base.userinfo_;
   this->host_ = new_base.host_;
   this->port_ = new_base.port_;
+  this->ip_literal_ = new_base.ip_literal_;
   // The new components come from the new base, so the result is an IRI if the
   // new base is one
   this->iri_ = this->iri_ || new_base.iri_;
 
   std::optional<std::string> new_base_path_copy{new_base.path_};
   merge_new_base_path(this->path_, std::move(new_base_path_copy),
-                      std::move(saved_path));
-
-  this->fragment_ = std::move(saved_fragment);
-  this->query_ = std::move(saved_query);
+                      std::move(relative_path));
 
   return *this;
 }
 
 auto URI::rebase(const URI &base, URI &&new_base) -> URI & {
-  this->relative_to(base);
-  if (!this->is_relative()) {
+  auto suffix{path_under(*this, base)};
+  if (!suffix.has_value()) {
     return *this;
   }
 
-  auto saved_path = std::move(this->path_);
-  auto saved_fragment = std::move(this->fragment_);
-  auto saved_query = std::move(this->query_);
+  std::optional<std::string> relative_path;
+  if (!suffix.value().empty()) {
+    relative_path = std::move(suffix.value());
+  }
 
   this->scheme_ = std::move(new_base.scheme_);
   this->userinfo_ = std::move(new_base.userinfo_);
   this->host_ = std::move(new_base.host_);
   this->port_ = new_base.port_;
+  this->ip_literal_ = new_base.ip_literal_;
   // The new components come from the new base, so the result is an IRI if the
   // new base is one
   this->iri_ = this->iri_ || new_base.iri_;
 
   merge_new_base_path(this->path_, std::move(new_base.path_),
-                      std::move(saved_path));
-
-  this->fragment_ = std::move(saved_fragment);
-  this->query_ = std::move(saved_query);
+                      std::move(relative_path));
 
   return *this;
 }

test/uri/CMakeLists.txt

@@ -41,6 +41,7 @@ sourcemeta_test(NAMESPACE sourcemeta PROJECT core NAME uri
     uri_normalize_path_test.cc
     uri_resolve_from_test.cc
     uri_relative_to_test.cc
+    uri_relativization_round_trip_test.cc
     uri_extension_test.cc
     uri_user_info_test.cc
     uri_is_iri_test.cc

test/uri/uri_has_same_authority_test.cc

@@ -102,3 +102,10 @@ TEST(iri_same_unicode_host) {
       "https://\xE4\xBE\x8B\xE3\x81\x88.jp/bar")};
   EXPECT_TRUE(left.has_same_authority(right));
 }
+
+TEST(ip_literal_vs_registered_name_of_the_same_text) {
+  const sourcemeta::core::URI left{"https://[v1.x]/foo"};
+  const sourcemeta::core::URI right{"https://v1.x/foo"};
+  EXPECT_FALSE(left.has_same_authority(right));
+  EXPECT_FALSE(right.has_same_authority(left));
+}

test/uri/uri_parse_test.cc

@@ -969,3 +969,12 @@ TEST(ipvfuture_missing_content_after_dot) {
 TEST(ipvfuture_invalid_content_character) {
   EXPECT_FALSE(sourcemeta::core::URI::is_uri("http://[v1.a%]/"));
 }
+
+TEST(success_with_percent_encoded_unreserved_is_decoded) {
+  // RFC 3986 Section 6.2.2.2 equates a percent-encoded unreserved character
+  // with the character itself, so decoding one can turn what looks like an
+  // ordinary segment into a dot segment
+  sourcemeta::core::URI uri{"https://www.example.com/a/%2E%2E/b"};
+  EXPECT_EQ(uri.path(), "/a/../b");
+  EXPECT_EQ(uri.recompose(), "https://www.example.com/a/../b");
+}