Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
package org.commonjava.indy.koji.util;

import org.commonjava.indy.core.content.PathMaskChecker;
import org.commonjava.indy.model.core.HostedRepository;
import org.commonjava.indy.model.core.RemoteRepository;
import org.junit.Test;

import java.util.List;
import java.util.Map;
import java.util.Set;

import static org.junit.Assert.*;

public class PathMaskCheckerTest
{

@Test
public void extractGroupIdPath() throws Exception
{
List<Map.Entry<String, String>> metadataFiles = List.of(
Map.entry("com/github/fge/msg-simple/maven-metadata.xml", "com/github/fge/"),
Map.entry("org/jboss/eap/jboss-eap-parent/maven-metadata.xml", "org/jboss/eap/"),
Map.entry("org/apache/lucene/lucene-core/maven-metadata.xml", "org/apache/lucene/"),
Map.entry("org/wildfly/security/wildfly-elytron/maven-metadata.xml", "org/wildfly/security/"),
Map.entry("io/dropwizard/metrics/metrics-core/maven-metadata.xml", "io/dropwizard/metrics/"),
Map.entry("org/jboss/spec/javax/el/jboss-el-api_3.0_spec/maven-metadata.xml", "org/jboss/spec/javax/el/")
);

for (var entry : metadataFiles)
{
assertEquals( entry.getValue(), PathMaskChecker.extractGroupIdPath(entry.getKey()) );
}
}

@Test
public void checkListingMask() throws Exception
{
HostedRepository hostedRepository = new HostedRepository(
"mvn-hosted"
);

RemoteRepository mrrcRepo = new RemoteRepository(
"mrrc-ga",
"http://example.url"
);
mrrcRepo.setPathMaskPatterns(
Set.of("r|.+[-.]redhat[-_]\\d+.*|")
);

RemoteRepository repositoryMatched = new RemoteRepository(
"koji-org.infinispan-infinispan-parent-9.4.2.Final_redhat_00001-2",
"http://example.url"
);
repositoryMatched.setPathMaskPatterns(
Set.of("r|org\\/infinispan\\/.+\\/9.4.2.Final-redhat-00001\\/.+|",
"org/infinispan/infinispan-query-dsl/maven-metadata.xml",
"r|org\\/infinispan\\/server\\/.+\\/9.4.2.Final-redhat-00001\\/.+|"));

RemoteRepository repositoryUnMatched = new RemoteRepository(
"koji-org.jboss.eap-jboss-eap-parent-7.2.0.GA_redhat_00002-2",
"http://example.url"
);
repositoryUnMatched.setPathMaskPatterns(
Set.of("r|org\\/jboss\\/eap\\/.+\\/7.2.0.GA-redhat-00002\\/.+|",
"org/jboss/eap/jboss-eap-parent/maven-metadata.xml"));

RemoteRepository repositoryWithMultipleGroupIds = new RemoteRepository(
"koji-com.sun.mail-all-1.6.1.redhat_1-1",
"http://example.url"
);
repositoryWithMultipleGroupIds.setPathMaskPatterns(
Set.of("javax/mail/javax.mail-api/maven-metadata.xml",
"com/sun/mail/javax.mail-api/maven-metadata.xml",
"r|javax\\/mail\\/.+\\/1.6.1.redhat-1\\/.+|",
"r|com\\/sun\\/mail\\/.+\\/1.6.1.redhat-1\\/.+|"));

assertTrue(PathMaskChecker.checkListingMask(repositoryMatched, "org/"));
assertTrue(PathMaskChecker.checkListingMask(repositoryMatched, "org/infinispan/"));
assertTrue(PathMaskChecker.checkListingMask(repositoryMatched, "org/infinispan/infinispan-component-annotations/"));
assertTrue(PathMaskChecker.checkListingMask(repositoryWithMultipleGroupIds, "com/sun/mail/"));
assertTrue(PathMaskChecker.checkListingMask(repositoryWithMultipleGroupIds, "javax/mail/"));
assertFalse(PathMaskChecker.checkListingMask(repositoryUnMatched, "org/infinispan/"));

assertTrue(PathMaskChecker.checkListingMask(hostedRepository, "org/infinispan/"));
assertTrue(PathMaskChecker.checkListingMask(mrrcRepo, "org/infinispan/"));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.util.Arrays;
import java.util.List;
import java.util.Objects;
import java.util.Set;
import java.util.stream.Collectors;

public class PathMaskChecker
{
Expand Down Expand Up @@ -71,13 +75,34 @@ public static boolean checkListingMask( final ArtifactStore store, final String
return true;
}

for ( String pattern : maskPatterns )
// if the pattern contains the metadata path, let's try to extract the groupId to filter the repos.
List<String> metadataPatterns = maskPatterns.stream()
.filter(pattern -> pattern.endsWith("maven-metadata.xml"))
.collect( Collectors.toList() );

if ( metadataPatterns.isEmpty() )
{
if ( isRegexPattern( pattern ) )
for ( String pattern : maskPatterns )
{
if ( isRegexPattern( pattern ) )
{
// if there is a regexp pattern we cannot check presence of directory listing, because we would have to
// check only the beginning of the regexp and that's impossible, so we have to assume that the path is
// present
return true;
}
}
}
else
{
boolean matches = metadataPatterns.stream()
.map(pattern -> extractGroupIdPath(pattern))
.filter( Objects::nonNull )
.anyMatch(groupIdPath -> path.startsWith( groupIdPath ) || groupIdPath.startsWith( path ) );

if ( matches )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this mean it is faster to check the groupId against meta pattern before checking all patterns?

Copy link
Member Author

@sswguo sswguo Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking all patterns does not work for some cases, like the following patterns of the repo maven:remote:koji-com.sun.mail-all-1.6.1.redhat_1-1:

"path_mask_patterns": [ "com/sun/mail/javax.mail/maven-metadata.xml", "r|com\\/sun\\/mail\\/.+\\/1.6.1.redhat-1\\/.+|", "r|javax\\/mail\\/.+\\/1.6.1.redhat-1\\/.+|" ],

it does not work if the directory listing path is "/com/sun/mail/android". And the android-1.6.1.redhat-1.pom is also one of the artifacts produced by that brew build.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing this https://issues.redhat.com/browse/MMENG-4453 will resolve the above issue, but we need to make it compatible on the exiting remote koji repos.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this make a lot many repos return true? if group matches (/com/sun/mail), such repo will be listed even the artifact not match.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function is aiming to reduce the repo candidates for further query against cassandra or brew. When I debug this, it now returns all koji-* repos which is ~2500 on prod.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand this pr is to fix the file missing issue when listing, so you use the groupId to filter first. Am I right? If so, let's go with it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but why it now returns all koji-* repos? if the patterns do not match, it should filter most of them...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it because old line 76? if ( isRegexPattern( pattern ) ) --> this could return true when hit the regex pattern. maybe it always hit it and finally returns all koji repos.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that's it line 76 and the reason in the comment line 78~80.

And this PR is still for the listing performance issue, https://issues.redhat.com/browse/MMENG-4447 .
There are attached logs I found during tests that would be helpful to know how it costs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

{
// if there is a regexp pattern we cannot check presence of directory listing, because we would have to
// check only the beginning of the regexp and that's impossible, so we have to assume that the path is
// present
logger.trace( "Checking mask in: {}, pattern with groupId path. - MATCH.", store.getName() );
return true;
}
}
Expand All @@ -96,6 +121,20 @@ public static boolean checkListingMask( final ArtifactStore store, final String
return false;
}

public static String extractGroupIdPath(String metadataPath)
{
if (metadataPath == null || !metadataPath.endsWith("maven-metadata.xml"))
{
return null;
}

// Need at least groupId/artifactId/filename
String[] parts = metadataPath.split("/");
if (parts.length < 3) return null;

return String.join("/", Arrays.copyOf(parts, parts.length - 2)) + "/";
}

public static boolean checkMavenMetadataMask( final ArtifactStore store, final String path )
{
Set<String> maskPatterns = store.getPathMaskPatterns();
Expand Down