Maven Shared Components
  1. Maven Shared Components
  2. MSHARED-7

Add stricter pattern filters to maven-common-artifact-filters

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Labels:
      None

      Description

      There is a todo in http://svn.apache.org/viewvc/maven/components/trunk/maven-artifact/src/main/java/org/apache/maven/artifact/resolver/filter/IncludesArtifactFilter.java?view=markup to add regex. I checked all the sources and could only find usages of this code by maven-assembly-plugin, webstart-maven-plugin and exec-maven-plugin. The latter two are in mojo.

      If you look at http://svn.palle.net/projects/hauskeeper/hauskeeper-server/src/assemblies/debian.xml, Trygvis is assuming that wildcards work, when in fact they do not. Arguably, this is a documentation bug that it does not work.

      The attached patch fixes this problem.

      1. IncludesArtifactFilter.patch
        1.0 kB
        Brian Topping
      2. MNG-2621.patch
        13 kB
        Mark Hobson

        Activity

        Hide
        Jerome Lacoste added a comment -

        It would be nice if you could add a unit test. Then +1 from me

        Show
        Jerome Lacoste added a comment - It would be nice if you could add a unit test. Then +1 from me
        Hide
        Jason van Zyl added a comment -

        Not allowing wildcard filters. Powerful but could potentially wreak havoc.

        Show
        Jason van Zyl added a comment - Not allowing wildcard filters. Powerful but could potentially wreak havoc.
        Hide
        Brett Porter added a comment -

        sounds like you meant won't fix

        Show
        Brett Porter added a comment - sounds like you meant won't fix
        Hide
        Mark Hobson added a comment -

        Attaching alternative patch as discussed in:
        http://www.mail-archive.com/dev@maven.apache.org/msg67727.html

        Show
        Mark Hobson added a comment - Attaching alternative patch as discussed in: http://www.mail-archive.com/dev@maven.apache.org/msg67727.html
        Hide
        Mark Hobson added a comment -

        For future-reference, wildcard filters are available in the maven-common-artifact-filters shared component:
        http://svn.apache.org/repos/asf/maven/shared/trunk/maven-common-artifact-filters/

        Show
        Mark Hobson added a comment - For future-reference, wildcard filters are available in the maven-common-artifact-filters shared component: http://svn.apache.org/repos/asf/maven/shared/trunk/maven-common-artifact-filters/
        Hide
        John Casey added a comment -

        I've checked out the attached patch, and it looks good for the most part. There are a couple of small logical and other errors:

        1. short-circuit on tokens vs. patterns length doesn't actually short circuit
        2. String.contains() is @since 1.5, so we need to switch to indexOf(..) > -1

        We could probably add this as a new class to the common-artifact-filters project, called something like StrictPatternIncludesFilter

        Better yet, we could add a new boolean parameter to the constructor, and allow this filter to be used as inclusion or exclusion by simply reversing the output when the constructor flag == false.

        Mark, do you have access to add this new filter to the maven-common-artifact-filters project directly?

        Also, note MASSEMBLY-180 when dealing with common-artifact-filters, as it looks like something is slightly broken in there (I haven't had time to go over it yet).

        Show
        John Casey added a comment - I've checked out the attached patch, and it looks good for the most part. There are a couple of small logical and other errors: 1. short-circuit on tokens vs. patterns length doesn't actually short circuit 2. String.contains() is @since 1.5, so we need to switch to indexOf(..) > -1 We could probably add this as a new class to the common-artifact-filters project, called something like StrictPatternIncludesFilter Better yet, we could add a new boolean parameter to the constructor, and allow this filter to be used as inclusion or exclusion by simply reversing the output when the constructor flag == false. Mark, do you have access to add this new filter to the maven-common-artifact-filters project directly? Also, note MASSEMBLY-180 when dealing with common-artifact-filters, as it looks like something is slightly broken in there (I haven't had time to go over it yet).
        Hide
        John Casey added a comment -

        I'll follow this up, to make sure it doesn't linger. Assigning to myself.

        Show
        John Casey added a comment - I'll follow this up, to make sure it doesn't linger. Assigning to myself.
        Hide
        Mark Hobson added a comment -

        Hi John, sure I can add this to maven-common-artifact-filters directly. I'll assign this to myself, refactor the code as mentioned above and commit, if that's okay? I'm not sure I understand you regarding point (1), the for-loop will be short-circuited since matched will be false.

        Show
        Mark Hobson added a comment - Hi John, sure I can add this to maven-common-artifact-filters directly. I'll assign this to myself, refactor the code as mentioned above and commit, if that's okay? I'm not sure I understand you regarding point (1), the for-loop will be short-circuited since matched will be false.
        Hide
        Mark Hobson added a comment -

        Updated issue title to reflect current direction.

        Show
        Mark Hobson added a comment - Updated issue title to reflect current direction.
        Hide
        Mark Hobson added a comment -

        Applied patch to maven-common-artifact-filters with John's suggestions: new filters are called StrictPatternIncludesArtifactFilter and StrictPatternExcludesArtifactFilter.

        Show
        Mark Hobson added a comment - Applied patch to maven-common-artifact-filters with John's suggestions: new filters are called StrictPatternIncludesArtifactFilter and StrictPatternExcludesArtifactFilter.
        Hide
        John Casey added a comment -

        my apologies, didn't read the patch closely enough.

        Show
        John Casey added a comment - my apologies, didn't read the patch closely enough.

          People

          • Assignee:
            Mark Hobson
            Reporter:
            Brian Topping
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development