Karaf
  1. Karaf
  2. KARAF-1066

make features xml parser more forgiving

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.2.9, 2.3.0, 3.0.0
    • Component/s: karaf-tooling
    • Labels:
      None

      Description

      features-maven-plugin:2.2.4:add-features-to-repo

      1) currently, an entry in features.xml like this works fine:

      <repository>mvn:org.apache.karaf.assemblies.features/standard/2.2.4/xml/features</repository>

      2) but plugin blows up with exception, if format is like this

      <repository>
      mvn:org.apache.karaf.assemblies.features/standard/2.2.4/xml/features
      </repository>

      "Error populating repository: Illegal character in scheme name at index 0:"

      3) I suggest to make features xml parser more forgiving

      1. karaf-3.0_1066.diff
        6 kB
        Heath Kesler
      2. karaf-3.0_1066_test.diff
        4 kB
        Heath Kesler
      3. karaf-3.0_1066_test_failure.diff
        0.6 kB
        Heath Kesler
      4. karaf-2.3_1066_test.diff
        3 kB
        Heath Kesler

        Activity

        Hide
        Jean-Baptiste Onofré added a comment -

        Agree with that

        Show
        Jean-Baptiste Onofré added a comment - Agree with that
        Hide
        Andreas Pieber added a comment -

        also +1

        Show
        Andreas Pieber added a comment - also +1
        Hide
        Heath Kesler added a comment -

        the repo parsing now removes all white space and newline/endline using regexp "
        s".

        I don't think we can have URI's with spaces, but I could be wrong.

        I implemented a test also that exercised the issue using a repo2.xml file.

        Show
        Heath Kesler added a comment - the repo parsing now removes all white space and newline/endline using regexp " s". I don't think we can have URI's with spaces, but I could be wrong. I implemented a test also that exercised the issue using a repo2.xml file.
        Hide
        Freeman Fang added a comment -

        Hi Heath,

        Thanks for this patch
        I think in the RepositoryImpl.java, simply using
        uri = uri.trim();
        is better than
        uri = uri.replaceAll("
        s", "");
        from your patch. As String.trim() can trim whitespace, \t\r\n from the string, but won't remove any whitespace in the middle of the string, so it avoid the potential risk if URI have whitespace.

        Moreover, you patch is valuable when parse the features.xml during runtime, but this issue is mainly about the features-maven-plugin:add-features-to-repo have issues, so your patch doesn't really fix it, we should fix it in some where like AddToRepositoryMojo.java.

        I'll take care of it and fix it soon.

        Best Regards

        Freeman

        Show
        Freeman Fang added a comment - Hi Heath, Thanks for this patch I think in the RepositoryImpl.java, simply using uri = uri.trim(); is better than uri = uri.replaceAll(" s", ""); from your patch. As String.trim() can trim whitespace, \t\r\n from the string, but won't remove any whitespace in the middle of the string, so it avoid the potential risk if URI have whitespace. Moreover, you patch is valuable when parse the features.xml during runtime, but this issue is mainly about the features-maven-plugin:add-features-to-repo have issues, so your patch doesn't really fix it, we should fix it in some where like AddToRepositoryMojo.java. I'll take care of it and fix it soon. Best Regards Freeman
        Hide
        Freeman Fang added a comment -

        Hi,

        I'm going to apply your patch with the change I mentioned firstly and then fix the AddToRepositoryMojo issue.

        Freeman

        Show
        Freeman Fang added a comment - Hi, I'm going to apply your patch with the change I mentioned firstly and then fix the AddToRepositoryMojo issue. Freeman
        Hide
        Freeman Fang added a comment -

        apply patch on behalf of Heath Kesler with thanks
        http://svn.apache.org/viewvc?rev=1372740&view=rev

        Show
        Freeman Fang added a comment - apply patch on behalf of Heath Kesler with thanks http://svn.apache.org/viewvc?rev=1372740&view=rev
        Show
        Freeman Fang added a comment - commit fix http://svn.apache.org/viewvc?rev=1372747&view=rev for trunk http://svn.apache.org/viewvc?rev=1372749&view=rev for 2.3.x branch http://svn.apache.org/viewvc?rev=1372748&view=rev for 2.2.x branch
        Hide
        Heath Kesler added a comment -

        @Freeman - thanks for the feedback, I had thought about using trim but was not sure about white spaces in the uri and since the tests passed I left it with the replaceAll.

        The test I wrote fails with white spaces in the URI using trim(). I will look at the patch you wrote and see what you did differently.

        Show
        Heath Kesler added a comment - @Freeman - thanks for the feedback, I had thought about using trim but was not sure about white spaces in the uri and since the tests passed I left it with the replaceAll. The test I wrote fails with white spaces in the URI using trim(). I will look at the patch you wrote and see what you did differently.
        Hide
        Heath Kesler added a comment -

        Freeman, here is a test to exercise the changes you made for the AddToRepositoryMojo class.

        Show
        Heath Kesler added a comment - Freeman, here is a test to exercise the changes you made for the AddToRepositoryMojo class.
        Hide
        Heath Kesler added a comment -

        I did test the trim() against a test I have for the RepositoryImpl class and it fails when there is a space in the URI, should I open a different jira for that or just continue to track it here?

        Thanks for helping me on this.

        I will add the test that fails to this jira so you can verify it.

        Show
        Heath Kesler added a comment - I did test the trim() against a test I have for the RepositoryImpl class and it fails when there is a space in the URI, should I open a different jira for that or just continue to track it here? Thanks for helping me on this. I will add the test that fails to this jira so you can verify it.
        Hide
        Heath Kesler added a comment -

        Here is the patch that will exercise the test failure when a non-encoded white space is in the uri.

        Let me know what you think.

        Show
        Heath Kesler added a comment - Here is the patch that will exercise the test failure when a non-encoded white space is in the uri. Let me know what you think.
        Hide
        Freeman Fang added a comment -

        Hi Heath,

        First of all, I don't think we should change content of the URI, so if any user has unencoded whitespace inside URI, they should get exception so that they know they do something wrong, just like if users has typo in URI hence can't get files they should know that they do something wrong and they need correct it themselves.

        But remove the whitespace \t\r\n from begining or end of the URI is OK, it doesn't change the URI content, it just get a better format URI.

        Btw, I like your AddToRepositoryMojoTest, it prove the trim works, and I will apply it to trunk(with minor change, I don't wanna the space inside URI and actually your test not run into URI parse)

        Freeman

        Show
        Freeman Fang added a comment - Hi Heath, First of all, I don't think we should change content of the URI, so if any user has unencoded whitespace inside URI, they should get exception so that they know they do something wrong, just like if users has typo in URI hence can't get files they should know that they do something wrong and they need correct it themselves. But remove the whitespace \t\r\n from begining or end of the URI is OK, it doesn't change the URI content, it just get a better format URI. Btw, I like your AddToRepositoryMojoTest, it prove the trim works, and I will apply it to trunk(with minor change, I don't wanna the space inside URI and actually your test not run into URI parse) Freeman
        Hide
        Freeman Fang added a comment -

        apply patch which add AddToRepositoryMojoTest on behalf of Heath Kesler with thanks
        http://svn.apache.org/viewvc?rev=1373188&view=rev

        @Heath,
        Do you wanna backport AddToRepositoryMojoTest to Karaf 2.x line?

        Show
        Freeman Fang added a comment - apply patch which add AddToRepositoryMojoTest on behalf of Heath Kesler with thanks http://svn.apache.org/viewvc?rev=1373188&view=rev @Heath, Do you wanna backport AddToRepositoryMojoTest to Karaf 2.x line?
        Hide
        Heath Kesler added a comment -

        Hi Freeman, your comments make sense, thanks for the feedback.

        I will backport the test to the karaf 2.x also.

        Show
        Heath Kesler added a comment - Hi Freeman, your comments make sense, thanks for the feedback. I will backport the test to the karaf 2.x also.
        Hide
        Heath Kesler added a comment -

        Freeman, here is the test for the 2.3 branch

        Show
        Heath Kesler added a comment - Freeman, here is the test for the 2.3 branch
        Hide
        Freeman Fang added a comment -

        apply patch which add AddFeaturesToRepoMojoTest on behalf of Heath Kesler with thanks
        http://svn.apache.org/viewvc?rev=1374896&view=rev for 2.3.x branch
        http://svn.apache.org/viewvc?rev=1374897&view=rev for 2.2.x branch

        Show
        Freeman Fang added a comment - apply patch which add AddFeaturesToRepoMojoTest on behalf of Heath Kesler with thanks http://svn.apache.org/viewvc?rev=1374896&view=rev for 2.3.x branch http://svn.apache.org/viewvc?rev=1374897&view=rev for 2.2.x branch

          People

          • Assignee:
            Freeman Fang
            Reporter:
            Andrei Pozolotin
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development