gnodet-bot commented on code in PR #2290: URL: https://github.com/apache/maven-resolver/pull/2290#discussion_r4504261594
########## maven-resolver-transport-s3/src/main/java/org/eclipse/aether/transport/s3/ObjectNameMapperFactory.java: ########## @@ -0,0 +1,41 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.eclipse.aether.transport.s3; + +import java.util.Map; + +import org.eclipse.aether.RepositorySystemSession; +import org.eclipse.aether.repository.RemoteRepository; +import software.amazon.awssdk.services.s3.S3Client; + +/** + * Factory for {@link ObjectNameMapper}. + * + * @since 2.0.26 + */ +public interface ObjectNameMapperFactory { + /** + * Creates a factory for mapping artifact layout path onto {@link ObjectName}. Should never return {@code null}. Review Comment: 🔤 **Javadoc accuracy:** The method comment says "Creates a *factory*" but the method returns an `ObjectNameMapper` (a mapper), not a factory. `ObjectNameMapperFactory` is itself the factory; `create` is the factory method. ```suggestion * Creates an {@link ObjectNameMapper} for the given session, repository, and client configuration. Should never return {@code null}. ``` ########## maven-resolver-transport-s3/src/main/java/org/eclipse/aether/transport/s3/ObjectName.java: ########## @@ -0,0 +1,82 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.eclipse.aether.transport.s3; + +import java.util.Objects; + +import static java.util.Objects.requireNonNull; + +/** + * S3 Object name, bucket + name. + * + * @since 2.0.26 + */ +public final class ObjectName { + private final String bucket; + private final String name; + private final int hashCode; + + public ObjectName(String bucket, String name) { + this.bucket = requireNonNull(bucket); + this.name = requireNonNull(name); + + if (bucket.contains("/")) { + throw new IllegalArgumentException("invalid bucket name: " + bucket); + } + if (name.contains("\\")) { + throw new IllegalArgumentException("invalid object name: " + name); + } + + this.hashCode = Objects.hash(bucket, name); + } + + public String getBucket() { + return bucket; + } + + public String getName() { + return name; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + ObjectName that = (ObjectName) o; + return Objects.equals(bucket, that.bucket) && Objects.equals(name, that.name); + } + + @Override + public int hashCode() { + return hashCode; + } + + @Override + public String toString() { + return bucket + "/" + name; + } + + public static String normalize(String name) { Review Comment: 📝 **Missing Javadoc on public method:** `normalize` is a `public static` method in a `public final` class with no Javadoc. What does it do, and why? A single sentence explaining that it normalises filesystem path separators for S3 key naming (backslash → forward slash) would help implementors and users of the SPI understand when to call it. ```suggestion /** * Normalises a filesystem path to a valid S3 object key by replacing backslashes with forward slashes. */ public static String normalize(String name) { ``` ########## src/site/markdown/deployment.md: ########## @@ -47,5 +47,6 @@ URL Protocol | Implementation | Description `file`, `bundle` | `org.eclipse.aether.transport.file.FileTransporter` | Writes the artifact or metadata to the file system. `http`, `https` | multiple | Issues an HTTP PUT request for each artifact or metadata. `classpath` | `org.eclipse.aether.transport.classpath.ClasspathTransporter` | Unsupported -`minio+http`, `minio+https`, `s3+http`, `s3+https` | `org.eclipse.aether.transport.minio.MinioTransporter` | Uploads the artifact or metadata as an object to the bucket. The configuration defines how the transporter converts the location from the `RepositoryLayout` into an object name and a bucket name. +`s3+http`, `s3+https` (and the `minio+` forms) | `org.eclipse.aether.transport.s3.S3Transporter` | Uploads the artifact or metadata as an object to the bucket through the AWS SDK for Java 2.x. The configuration defines how the transporter converts the location from the `RepositoryLayout` into an object name and a bucket name. +`minio+http`, `minio+https` | `org.eclipse.aether.transport.minio.MinioTransporter` | Uploads the artifact or metadata as an object to the bucket. The configuration defines how the transporter converts the location from the `RepositoryLayout` into an object name and a bucket name. Review Comment: 📋 **Priority interaction not documented:** The S3 row already notes that `S3TransporterFactory` handles both `s3+` and `minio+` schemes. However, `S3TransporterFactory.DEFAULT_PRIORITY = 1.0f` while MinIO's factory defaults to `0.0f` — meaning `S3TransporterFactory` will intercept `minio+` URLs first whenever both modules are on the classpath, and the MinIO-specific row below becomes unreachable. The table should note this: either warn that MinIO transport is inactive when the S3 module is present, or document that the MinIO transporter is only active when the S3 module is absent. ########## maven-resolver-transport-s3/src/main/java/org/eclipse/aether/transport/s3/ObjectNameMapperFactory.java: ########## @@ -0,0 +1,41 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.eclipse.aether.transport.s3; + +import java.util.Map; + +import org.eclipse.aether.RepositorySystemSession; +import org.eclipse.aether.repository.RemoteRepository; +import software.amazon.awssdk.services.s3.S3Client; + +/** + * Factory for {@link ObjectNameMapper}. + * + * @since 2.0.26 + */ +public interface ObjectNameMapperFactory { + /** + * Creates a factory for mapping artifact layout path onto {@link ObjectName}. Should never return {@code null}. + */ + ObjectNameMapper create( + RepositorySystemSession session, + RemoteRepository repository, + S3Client s3Client, + Map<String, String> extraHeaders); Review Comment: ⚠️ **API design — `S3Client` on public SPI interface:** Both shipped implementations (`FixedBucketObjectNameMapperFactory`, `RepositoryIdObjectNameMapperFactory`) declare this parameter as `S3Client unused` — it is never consumed. Placing an unused, heavyweight AWS SDK type on a public SPI interface that third parties can implement creates a binary compatibility obligation with no benefit: future implementors must import `software.amazon.awssdk.services.s3.S3Client` even if their naming scheme is purely config-driven. Two options before this interface stabilises at 2.0.26: 1. **Remove `S3Client` from the signature** — if no implementation needs it today, don't put it in the SPI. If a future naming strategy needs it (e.g. listing buckets), that strategy can call `S3Client` itself since the transporter already holds a reference. 2. **Keep it, document the intent** — add a Javadoc comment explaining why the client is offered (e.g. for naming schemes that need to query bucket metadata before constructing the name). Option 1 is safer for binary compatibility. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
