michael-o commented on code in PR #194:
URL: https://github.com/apache/maven-resolver/pull/194#discussion_r981133064


##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/named/GAVNameMapper.java:
##########
@@ -19,39 +19,112 @@
  * under the License.
  */
 
+import java.util.Collection;
+import java.util.TreeSet;
+
+import org.eclipse.aether.RepositorySystemSession;
 import org.eclipse.aether.artifact.Artifact;
 import org.eclipse.aether.metadata.Metadata;
 
+import static java.util.Objects.requireNonNull;
+
 /**
  * Artifact GAV {@link NameMapper}, uses artifact and metadata coordinates to 
name their corresponding locks. Is not
- * considering local repository, only the artifact coordinates.
+ * considering local repository, only the artifact coordinates. May use custom 
prefixes and sufixes and separators,
+ * hence this instance may or may not be filesystem friendly (depends on 
strings used).
  */
-public class GAVNameMapper extends NameMapperSupport
+public class GAVNameMapper implements NameMapper
 {
+    private final boolean fileSystemFriendly;
+
+    private final String artifactPrefix;
+
+    private final String artifactSuffix;
+
+    private final String metadataPrefix;
+
+    private final String metadataSuffix;
+
+    private final String fieldSeparator;
+
+    public GAVNameMapper( boolean fileSystemFriendly,
+                          String artifactPrefix, String artifactSuffix,
+                          String metadataPrefix, String metadataSuffix,
+                          String fieldSeparator )
+    {
+        this.fileSystemFriendly = fileSystemFriendly;
+        this.artifactPrefix = requireNonNull( artifactPrefix );
+        this.artifactSuffix = requireNonNull( artifactSuffix );
+        this.metadataPrefix = requireNonNull( metadataPrefix );
+        this.metadataSuffix = requireNonNull( metadataSuffix );
+        this.fieldSeparator = requireNonNull( fieldSeparator );
+    }
+
     @Override
-    protected String getArtifactName( Artifact artifact )
+    public boolean isFileSystemFriendly()
     {
-        return "artifact:" + artifact.getGroupId()
-                + ":" + artifact.getArtifactId()
-                + ":" + artifact.getBaseVersion();
+        return fileSystemFriendly;
     }
 
     @Override
-    protected String getMetadataName( Metadata metadata )
+    public Collection<String> nameLocks( final RepositorySystemSession session,
+                                         final Collection<? extends Artifact> 
artifacts,
+                                         final Collection<? extends Metadata> 
metadatas )
+    {
+        // Deadlock prevention: https://stackoverflow.com/a/16780988/696632
+        // We must acquire multiple locks always in the same order!
+        TreeSet<String> keys = new TreeSet<>();
+        if ( artifacts != null )
+        {
+            for ( Artifact artifact : artifacts )
+            {
+                keys.add( getArtifactName( artifact ) );
+            }
+        }
+
+        if ( metadatas != null )
+        {
+            for ( Metadata metadata : metadatas )
+            {
+                keys.add( getMetadataName( metadata ) );
+            }
+        }
+        return keys;
+    }
+
+    private String getArtifactName( Artifact artifact )
+    {
+        return artifactPrefix + artifact.getGroupId()
+                + fieldSeparator + artifact.getArtifactId()
+                + fieldSeparator + artifact.getBaseVersion()
+                + artifactSuffix;
+    }
+
+    private String getMetadataName( Metadata metadata )
     {
-        String name = "metadata:";
+        String name = metadataPrefix;
         if ( !metadata.getGroupId().isEmpty() )
         {
             name += metadata.getGroupId();
             if ( !metadata.getArtifactId().isEmpty() )
             {
-                name += ":" + metadata.getArtifactId();
+                name += fieldSeparator + metadata.getArtifactId();
                 if ( !metadata.getVersion().isEmpty() )
                 {
-                    name += ":" + metadata.getVersion();
+                    name += fieldSeparator + metadata.getVersion();
                 }
             }
         }
-        return name;
+        return name + metadataSuffix;
+    }
+
+    public static NameMapper gav()
+    {
+        return new GAVNameMapper( false, "artifact:", "", "metadata:", "", ":" 
);
+    }
+
+    public static NameMapper fileGav()
+    {
+        return new GAVNameMapper( true, "", ".lock", "", ".lock", "~" );

Review Comment:
   Attention, whe you obtain group level locks on metadata this can lead to 
very coarse locks. Why not have prefixes: `metadata~` and `artifact~? ?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to