[MDEP-568] go offline filter #135

bmarwell · 2021-06-01T10:09:09Z

Following this checklist to help us incorporate your
contribution quickly and easily:

Make sure there is a JIRA issue filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [MDEP-XXX] - Fixes bug in ApproximateQuantiles,
where you replace MDEP-XXX with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean verify to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
You have run the integration tests successfully (mvn -Prun-its clean verify).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

elharo

All the filters should have unit tests.

elharo · 2021-06-01T12:07:18Z

src/main/java/org/apache/maven/plugins/dependency/filters/AbstractDependencyFilter.java

@@ -0,0 +1,59 @@
+package org.apache.maven.plugins.dependency.filters;


Is this introducing a new package? Probably not needed.

Yup, because code is similar to artifact-filter's ArtifactFilter which is also in its own package. Also, more filters could be introduced later. Is there a downside of creating a package I do not know?

It tends to force methods to be public for use by other subpackages that otherwise wouldn't have to be public.

@elharo is this change a requirement for this PR to be accepted? I agree with @bmarwell approach (to be future proof), but if it is mandatory, I think it is possible to change packages.

Yes, packaghes are overused with bad consequences

src/main/java/org/apache/maven/plugins/dependency/filters/AbstractDependencyFilter.java

src/main/java/org/apache/maven/plugins/dependency/filters/ClassifierFilter.java

src/main/java/org/apache/maven/plugins/dependency/filters/FilterDependencies.java

src/main/java/org/apache/maven/plugins/dependency/filters/GroupIdFilter.java

src/main/java/org/apache/maven/plugins/dependency/filters/ArtifactIdFilter.java

antoniolucasnobar · 2021-07-09T14:01:26Z

src/main/java/org/apache/maven/plugins/dependency/filters/AbstractDependencyFilter.java

+import java.util.Set;
+
+
+abstract class AbstractDependencyFilter implements DependencyFilter


Hello @bmarwell ,
Since you created this abstract class, maybe it could contain more than just splitValues method. The inherited classes are very similar IMO; This way, this abstract class could have almost all logic here, with the filter, include and exclude methods; It is just a suggestion to avoid code duplication.

Below an example of the abstract class and one of its child. Even the filterexcludeIds and filterincludeIds could be simplified because they share similar logic.

As ScopeFilter has specific treatment to split include and exclue ids, I created two functions splitIncludeIds and splitExcludeIds

abstract class AbstractDependencyFilter implements DependencyFilter { protected final String includeIds; protected final String excludeIds; public AbstractDependencyFilter(String includeIds, String excludeIds) { this.includeIds = includeIds == null ? "" : includeIds; this.excludeIds = excludeIds == null ? "" : excludeIds; } @Override public Set<Dependency> filter(Set<Dependency> dependencies) { Set<Dependency> filtered = new HashSet<>(dependencies); filtered = filterincludeIds(filtered); filtered = filterexcludeIds(filtered); return filtered; } private Set<Dependency> filterexcludeIds(Set<Dependency> dependencies) { if (excludeIds.trim().isEmpty()) { return dependencies; } final Set<String> excludedIds = splitExcludeIds(excludeIds); Set<Dependency> filtered = new HashSet<>(dependencies.size()); for (Dependency dependency : dependencies) { if (excludedIds.contains(getContainsProperty(dependency))) { continue; } filtered.add(dependency); } return filtered; } private Set<Dependency> filterincludeIds(Set<Dependency> dependencies) { if (includeIds.trim().isEmpty()) { return dependencies; } Set<String> includedIds = splitIncludeIds(includeIds); Set<Dependency> filtered = new HashSet<>(dependencies.size()); for (Dependency dependency : dependencies) { if (includedIds.contains(getContainsProperty(dependency))) { filtered.add(dependency); } } return filtered; } protected Set<String> splitExcludeIds(String excludeIds) { return splitValues(excludeIds); } protected Set<String> splitIncludeIds(String includeIds) { return splitValues(includeIds); } protected Set<String> splitValues(String csvValueList) { // ommited here } abstract protected String getContainsProperty(Dependency dependency); }

public class GroupIdFilter extends AbstractDependencyFilter { public GroupIdFilter(String includeGroupIds, String excludeGroupIds) { super(includeGroupIds, excludeGroupIds); } protected String getContainsProperty(Dependency dependency) { return dependency.getGroupId(); } }

I was thinking of closing this PR. We would need the same logic for other goals as well, and this goal could be replaced with the combination of two other goals.

I think you could just create a PR to my branch of you're interested to keep it up.

Created the PR to your branch: #145

Should I change anything else? There is a apparently unfinished thread about package names in June 1st.

Do I have to sign any documents? Please let me know.

PS.: I am sorry, butI did not understand why you are thinking in close this PR. Which goals are you talking about? Should they be a part in this PR? I think this PR solves the go-offline problem, which is the subject of MDEP-568.

bmarwell · 2021-08-13T11:34:56Z

@elharo can you please look over this PR again?

TimKnight-DWP · 2022-10-28T08:02:53Z

Would really appreciate this being re-looked at and merged, it causes us problems when caching dependencies in Gitlab if these filters don't actually work.

Happy to provide input/coding

@elharo @bmarwell

elharo

assorted comments; though there seems to be some question from others about the overall approach that you might want to address first

elharo · 2022-10-31T12:15:23Z

src/it/projects/mdep-568-go-offline-exclude/pom.xml

@@ -0,0 +1,57 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+  ~ Licensed to the Apache Software Foundation (ASF) under one


Why all the tildes in the license?

removed in #258. It is a pull request to this branch.

I don't understand. I still see them. Do you need to merge to master?

elharo · 2022-10-31T12:16:07Z

src/it/projects/mdep-568-go-offline-exclude/pom.xml

+      <version>2.0.6</version>
+    </dependency>
+    <dependency>
+      <groupId>non.existant.group</groupId>


nit: existent with an e

fixed in #258

elharo · 2022-10-31T12:17:26Z

src/main/java/org/apache/maven/plugins/dependency/filters/AbstractDependencyFilter.java

+        return filtered;
+    }
+
+    private Set<Dependency> filterexcludeIds( Set<Dependency> dependencies )


filterExcludeIds

fixed in #258

elharo · 2022-10-31T12:18:18Z

src/main/java/org/apache/maven/plugins/dependency/filters/AbstractDependencyFilter.java

+        Set<Dependency> filtered = new HashSet<>( dependencies.size() );
+        for ( Dependency dependency : dependencies )
+        {
+            if ( excludedIds.contains( getContainsProperty( dependency ) ) )


This is a bit awkward. Might be worth adding a local variable, renaming, the method, and/or refactoring with a more complete method

fixed in #258

It is now like this:

boolean shouldFilterDependency = ids.contains( getId( dependency ) ) == expectedCondition; if ( shouldFilterDependency ) { filtered.add( dependency ); }

expectedConditionis true for the includeIds case anda false for the exclude. I created one method to remove the code duplication.

elharo · 2022-10-31T12:18:43Z

src/main/java/org/apache/maven/plugins/dependency/filters/AbstractDependencyFilter.java

+        {
+            if ( excludedIds.contains( getContainsProperty( dependency ) ) )
+            {
+                continue;


don't use continue; just change the if to if not

fixed in #258

elharo · 2022-10-31T12:30:17Z

src/test/java/org/apache/maven/plugins/dependency/filters/ClassifierFilterTest.java

+    public void testArtifactFilterInclude()
+    {
+        // given
+        final Dependency comSunTools = COM_SUN_TOOLS_SYSTEM_JAR;


local variables seem redundant here

fixed in #258

elharo · 2022-10-31T12:30:33Z

src/test/java/org/apache/maven/plugins/dependency/filters/GroupIdFilterTest.java

+    public void testGroupFilterInclude()
+    {
+        // given
+        final Dependency comSunTools = COM_SUN_TOOLS_SYSTEM_JAR;


local variables seem redundant here

fixed in #258

elharo · 2022-10-31T12:30:41Z

src/test/java/org/apache/maven/plugins/dependency/filters/ScopeFilterTest.java

+    public void testScopeFilterExclude()
+    {
+        // given
+        final Dependency comSunTools = COM_SUN_TOOLS_SYSTEM_JAR;


local variables seem redundant here

fixed in #258

elharo · 2022-10-31T12:31:03Z

src/test/java/org/apache/maven/plugins/dependency/filters/TypeFilterTest.java

+    @Test
+    public void testTypeFilterExclude()
+    {
+        // given


local variables seem redundant here

fixed in #258

elharo · 2022-10-31T12:31:29Z

src/test/java/org/apache/maven/plugins/dependency/resolvers/TestGoOfflineMojo.java

+public class TestGoOfflineMojo extends AbstractDependencyMojoTestCase
+{
+
+    protected void setUp() throws Exception


fixed in #258

antoniolucasnobar · 2022-11-06T04:15:47Z

@elharo I fixed in #258 most of your questions.
Also, I resolved the conflicts with master.

About the overall aproach, I was never able to fully understand what is the problem with this one. @bmarwell could you please help with this?

…wnload artifacts, download the artifacts in the plugin code after filtering them with the provided parameters.

* [MDEP-568] Move common methods to parent class. * [MDEP-568] Fix spacing * [MDEP-568] Fix spacing * [MDEP-568] Fix formatting Co-authored-by: Antonio <antonio.lucas@trt4.jus.br>

jshbrntt · 2023-08-09T20:58:11Z

Is this going to be merged anytime soon?

It would be really helpful to install and cache dependencies prior to mvn package.

elharo

I don't understand what's going on with this commit. Please make sure it's up to date against master, and comments are addressed here, not in other PRs.

elharo · 2023-11-03T22:30:35Z

src/it/projects/mdep-568-go-offline-exclude/pom.xml

@@ -0,0 +1,57 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+  ~ Licensed to the Apache Software Foundation (ASF) under one


I don't understand. I still see them. Do you need to merge to master?

elharo · 2023-11-03T22:31:20Z

src/main/java/org/apache/maven/plugins/dependency/filters/AbstractDependencyFilter.java

@@ -0,0 +1,59 @@
+package org.apache.maven.plugins.dependency.filters;


Yes, packaghes are overused with bad consequences

bmarwell · 2023-11-04T11:54:42Z

This is old. If interested, please do a new PR. I'll leave the branch for a while.

bmarwell mentioned this pull request Jun 1, 2021

[MDEP-568] respect excludeGroupIds in go-offline #2

Closed

bmarwell requested review from mthmulders and slachiewicz June 1, 2021 10:09

elharo reviewed Jun 1, 2021

View reviewed changes

bmarwell force-pushed the MDEP-568_go-offline_filter branch from 68600d6 to ab8d8d3 Compare June 1, 2021 17:26

bmarwell requested a review from elharo June 1, 2021 17:27

elharo reviewed Jun 1, 2021

View reviewed changes

src/main/java/org/apache/maven/plugins/dependency/filters/ArtifactIdFilter.java Outdated Show resolved Hide resolved

bmarwell requested a review from elharo June 2, 2021 07:03

antoniolucasnobar reviewed Jul 9, 2021

View reviewed changes

antoniolucasnobar mentioned this pull request Jul 14, 2021

[MDEP-568] Moves similar methods to parent class #145

Merged

8 tasks

elharo reviewed Oct 31, 2022

View reviewed changes

Theoderich and others added 3 commits November 6, 2022 12:39

[MDEP-568] instead of relying on "requiresDependencyResolution" to do…

3233fce

…wnload artifacts, download the artifacts in the plugin code after filtering them with the provided parameters.

[MDEP-568] filter dependencies in go-offline as documented

ec4854d

[MDEP-568] Moves similar methods to parent class (#145)

8746246

* [MDEP-568] Move common methods to parent class. * [MDEP-568] Fix spacing * [MDEP-568] Fix spacing * [MDEP-568] Fix formatting Co-authored-by: Antonio <antonio.lucas@trt4.jus.br>

slachiewicz force-pushed the MDEP-568_go-offline_filter branch from 5c59113 to 8746246 Compare November 6, 2022 11:42

michael-o requested review from elharo and antoniolucasnobar November 3, 2023 20:01

elharo requested changes Nov 3, 2023

View reviewed changes

bmarwell closed this Nov 4, 2023

		@@ -0,0 +1,59 @@
		package org.apache.maven.plugins.dependency.filters;

		import java.util.Set;


		abstract class AbstractDependencyFilter implements DependencyFilter

[MDEP-568] go offline filter #135

[MDEP-568] go offline filter #135

Conversation

bmarwell commented Jun 1, 2021

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoniolucasnobar Jul 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarwell commented Aug 13, 2021

TimKnight-DWP commented Oct 28, 2022

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoniolucasnobar commented Nov 6, 2022

jshbrntt commented Aug 9, 2023

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarwell commented Nov 4, 2023

antoniolucasnobar Jul 9, 2021 •

edited