Several improvements to URI class.

#130 Merged at 03fd6f1
  1. Martin Pecka

Followup of : separated URI-related changes to their own PR.

Closer to the RFCs defining an URI. Added URI Fragment support.

This now breaks URIs with spaces (which do not follow the RFCs), but these are now required in ign fuel. We must add support for URI encoding before these changes can be merged.

Comments (13)

  1. Martin Pecka author

    All tests pass on my Windows machine (except [av] and [graphics] components, which I don’t build on Windows).

  2. Louise Poubel

    Thanks for the PR, @peci1. Do you have a suggestion on how to handle encoding? On ign-fuel-tools we use CURL's curl_easy_escape, but it would be nice to have a solution on ign-common which doesn't require CURL.

  3. Martin Pecka author

    I think curl_easy_escape would not help if you’d like to escape scheme, path, query and fragment separately… So far, I’ve looked into Mozilla’s source code, and the encoding shouldn’t be that difficult to write. I’ll have a look at it.

    A question is how to add it to to exising ign-common code. My idea is adding an optional parameter to URI.Parse() (and URIPath.Parse() and so on) that would tell whether the given string should be encoded or not.

    1. Louise Poubel

      Awesome, thanks for looking into the Mozilla approach. I think that adding a parameter is a good idea, but it can be tricky to keep it API/ABI compatible, so we'd have to go through a deprecation cycle... Maybe a less intrusive option could be to add something like an URI.Encoding()flag which allows setting the encoding type, or maybe just a static URI.Encode() function could also get the job done.

  4. Louise Poubel

    @peci1 , what do you think of removing the whitespace changes from this PR so we can get the fragments in quickly, and revisit the space handling later with a migration plan for ign-fuel-tools? Do the following changes work for you?

    diff -r dab2bddb362d include/ignition/common/URI.hh
    --- a/include/ignition/common/URI.hh    Wed Jun 06 16:52:23 2018 +0200
    +++ b/include/ignition/common/URI.hh    Mon Jul 30 13:43:37 2018 -0700
    @@ -83,7 +83,7 @@
           /// \return The path as a string, with each path part separated by _delim.
           public: std::string Str(const std::string &_delim = "/") const;
    -      /// \brief Equal operator.
    +      /// \brief Assignment operator.
           /// \param[in] _path Another URIPath.
           /// \return Itself.
           public: URIPath &operator=(const URIPath &_path);
    @@ -135,7 +135,7 @@
           public: void Insert(const std::string &_key,
                               const std::string &_value);
    -      /// \brief Equal operator.
    +      /// \brief Assignment operator.
           /// \param[in] _query another URIQuery.
           /// \return Itself.
           public: URIQuery &operator=(const URIQuery &_query);
    @@ -192,12 +192,12 @@
           /// \brief Remove all values of the fragment
           public: void Clear();
    -      /// \brief Equal operator.
    +      /// \brief Assignment operator.
           /// \param[in] _fragment another URIFragment.
           /// \return Itself.
           public: URIFragment &operator=(const URIFragment &_fragment);
    -      /// \brief Equal operator.
    +      /// \brief Assignment operator.
           /// \param[in] _fragment another URIFragment.
           /// \return Itself.
           public: URIFragment &operator=(const std::string &_fragment);
    @@ -292,7 +292,7 @@
           /// \return A const reference of the fragment.
           public: const URIFragment &Fragment() const;
    -      /// \brief Equal operator.
    +      /// \brief Assignment operator.
           /// \param[in] _uri Another URI.
           /// \return Itself.
           public: URI &operator=(const URI &_uri);
    diff -r dab2bddb362d src/
    --- a/src/        Wed Jun 06 16:52:23 2018 +0200
    +++ b/src/        Mon Jul 30 13:43:37 2018 -0700
    @@ -35,7 +35,7 @@
       public: std::list<std::string> path;
       /// \brief Whether the path is absolute (starts with slash) or not.
    -  public: bool isAbsolute;
    +  public: bool isAbsolute{false};
       /// \brief A helper method to determine if the given string represents
       ///        an absolute path starting segment or not.
    @@ -202,7 +202,7 @@
    -                                   "[]";
    +                                   "[] ";
       if (str.find_first_not_of(allowedChars) != std::string::npos)
         return false;
    diff -r dab2bddb362d src/
    --- a/src/   Wed Jun 06 16:52:23 2018 +0200
    +++ b/src/   Mon Jul 30 13:43:37 2018 -0700
    @@ -31,14 +31,14 @@
       EXPECT_EQ(path1.Str(), "part1");
       EXPECT_EQ(path1.Str(), "part1/part2");
    -  path1.PushFront("part%200");
    -  EXPECT_EQ(path1.Str(), "part%200/part1/part2");
    +  path1.PushFront("part 0");
    +  EXPECT_EQ(path1.Str(), "part 0/part1/part2");
       path2 = path1 / "part3";
    -  EXPECT_EQ(path2.Str(), "part%200/part1/part2/part3");
    +  EXPECT_EQ(path2.Str(), "part 0/part1/part2/part3");
       path1 /= "part3";
    -  EXPECT_EQ(path1.Str(), "part%200/part1/part2/part3");
    +  EXPECT_EQ(path1.Str(), "part 0/part1/part2/part3");
       EXPECT_TRUE(path1 == path2);
    @@ -85,9 +85,7 @@
    -  EXPECT_FALSE(URIPath::Valid("/part 1/part 2/"));
    -  EXPECT_TRUE(URIPath::Valid("/part+1/part+2"));
    -  EXPECT_TRUE(URIPath::Valid("/part%201/part%202"));
    +  EXPECT_TRUE(URIPath::Valid("/part 1/part 2/"));
    @@ -103,9 +101,7 @@
    -  EXPECT_FALSE(path.Parse("/part 1/part 2/"));
    -  EXPECT_TRUE(path.Parse("/part+1/part+2"));
    -  EXPECT_TRUE(path.Parse("/part%201/part%202"));
    +  EXPECT_TRUE(path.Parse("/part 1/part 2/"));
    @@ -122,8 +118,6 @@
       EXPECT_NO_THROW(URIPath("/part 1/part2/"));
    -  EXPECT_NO_THROW(URIPath("/part+1/part+2"));
    -  EXPECT_NO_THROW(URIPath("/part%201/part%202"));
    @@ -266,7 +260,6 @@
    -  EXPECT_TRUE(URIFragment::Valid("#fragment/?!$&'()*+,;=:@%20fragment"));
    @@ -280,7 +273,6 @@
    -  EXPECT_TRUE(fragment.Parse("#fragment/?!$&'()*+,;=:@%20FRAGMENT"));
       // it should still be valid
    @@ -292,7 +284,6 @@
  5. Martin Pecka author

    Hmm, and what about commenting out the nonpassing tests with a TODO comment to enable them once whitespace is handled correctly? But that’s not a strong request, do what you think is best 🙂

    1. Louise Poubel

      Commenting out sounds good to me. We should also ticket an issue to make sure this doesn't get lost ;)

  6. Martin Pecka author

    Fixed handling of absolute paths.

    I had to decide on what to do in URIPath::PushFront and URIPath::PushBack when the parameter contains slashes. I came to the following solution:

    • I added explicit methods for setting the URIPath absolute/relative. The methods aren’t virtual, so ABI should not be affected.
    • if _part is empty: ignore the call at all
    • if _part starts with a slash

      • URIPath::PushFront: make the URI absolute, strip the slash, and issue a warning
      • URIPath::PushBack: if the URI is empty, then make it absolute, strip the slash and issue a warning
    • if _part has slashes inside (even on the end): the slashes are encoded to %2F

    I hope this is a suitable solution. Another option would be to encode even the leading slash and do not treat it special, but I somehow feel this way it might be a bit more user-friendly (e.g. constructs like URIPath path = ”/my” / ”absolute” / ”uri”;).

    1. Louise Poubel

      Thanks for addressing the comments, looking good to me!

      I have a suggestion to add a few more checks on this PR. I think we're ready to merge after that.

      1. Louise Poubel

        Approved and fixed a few cppcheck warnings I had missed / caused earlier.

        CI results pre-merge:

        • Homebrew Build Status
        • Xenial Build Status (warnings fixed in e06998d)
        • Windows Build Status (no related failures / warnings)