You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/12/14 14:26:35 UTC
[GitHub] [maven-resolver] cstamas opened a new pull request #139: Checksum is not a digest
cstamas opened a new pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139
Distance ourselves from MessageDigest, and open up ways
to define and support different (real) checksum implementations
as well.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787580506
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/Maven2RepositoryLayoutFactory.java
##########
@@ -83,23 +106,33 @@ public RepositoryLayout newInstance( RepositorySystemSession session, RemoteRepo
throw new NoRepositoryLayoutException( repository );
}
boolean forSignature = ConfigUtils.getBoolean( session, false, CONFIG_PROP_SIGNATURE_CHECKSUMS );
- List<String> checksumsAlgorithms = Arrays.asList( ConfigUtils.getString( session,
- DEFAULT_CHECKSUMS_ALGORITHMS, CONFIG_PROP_CHECKSUMS_ALGORITHMS ).split( "," ) );
+ // ensure order of (potentially user set) algorithm list is kept and is unique
+ LinkedHashSet<String> checksumsAlgorithmNames = new LinkedHashSet<>( Arrays.asList(
Review comment:
User provides a "comma separated list", hence it is a LIST, and in case of layout, ORDER DOES MATTER (as that is the order resolver will ask for checksum).
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported bitrot detection checksums extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r768905236
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultChecksumImplementationSelector.java
##########
@@ -0,0 +1,75 @@
+package org.eclipse.aether.internal.impl;
+
+/*
+ * 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.
+ */
+
+import java.security.NoSuchAlgorithmException;
+import java.util.Collections;
+import java.util.Map;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Provider;
+import javax.inject.Singleton;
+
+import org.eclipse.aether.spi.connector.checksum.ChecksumImplementation;
+import org.eclipse.aether.spi.connector.checksum.ChecksumImplementationSelectorSupport;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link org.eclipse.aether.spi.connector.checksum.ChecksumImplementationSelector} that
+ * is extensible.
+ *
+ * @since TBD
+ */
+@Singleton
+@Named
+public final class DefaultChecksumImplementationSelector
Review comment:
Well, partially wrong: connector-basic does have java.inject as dep.
Nevertheless, ifaces does belong to SPI, and support class is really just to reuse it in basic-connector tests and in impl where we add the injection of possible components by SPI extension.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #139: [MRESOLVER-230] Make supported bitrot detection checksums extensible
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r768895575
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultChecksumImplementationSelector.java
##########
@@ -0,0 +1,75 @@
+package org.eclipse.aether.internal.impl;
+
+/*
+ * 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.
+ */
+
+import java.security.NoSuchAlgorithmException;
+import java.util.Collections;
+import java.util.Map;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Provider;
+import javax.inject.Singleton;
+
+import org.eclipse.aether.spi.connector.checksum.ChecksumImplementation;
+import org.eclipse.aether.spi.connector.checksum.ChecksumImplementationSelectorSupport;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link org.eclipse.aether.spi.connector.checksum.ChecksumImplementationSelector} that
+ * is extensible.
+ *
+ * @since TBD
+ */
+@Singleton
+@Named
+public final class DefaultChecksumImplementationSelector
Review comment:
But why then split into this and the supporting class?
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] kwin commented on pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
kwin commented on pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#issuecomment-1016285437
Why not using `SortedSet` then instead of `List` for the layout?
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787601498
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumAlgorithmHelper.java
##########
@@ -0,0 +1,106 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * 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.
+ */
+
+import java.io.BufferedInputStream;
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.Buffer;
+import java.nio.ByteBuffer;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Helper for checksum operations.
+ *
+ * @since TBD
+ */
+public final class ChecksumAlgorithmHelper
+{
+ private ChecksumAlgorithmHelper()
+ {
+ // nop
+ }
+
+ /**
+ * Calculates checksums for specified data.
+ *
+ * @param data The content for which to calculate checksums, must not be {@code null}.
+ * @param factories The checksum algorithm factories to use, must not be {@code null}.
+ * @return The calculated checksums, indexed by algorithm name, or the exception that occurred while trying to
+ * calculate it, never {@code null}.
+ * @throws IOException In case of any problem.
+ */
+ public static Map<String, String> calculate( byte[] data, List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ return calculate( new ByteArrayInputStream( data ), factories );
+ }
+
+ /**
+ * Calculates checksums for specified file.
+ *
+ * @param file The file for which to calculate checksums, must not be {@code null}.
+ * @param factories The checksum algorithm factories to use, must not be {@code null}.
+ * @return The calculated checksums, indexed by algorithm name, or the exception that occurred while trying to
+ * calculate it, never {@code null}.
+ * @throws IOException In case of any problem.
+ */
+ public static Map<String, String> calculate( File file, List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ return calculate( new BufferedInputStream( new FileInputStream( file ) ), factories );
+ }
+
+ private static Map<String, String> calculate( InputStream inputStream, List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ LinkedHashMap<String, ChecksumAlgorithm> algorithms = new LinkedHashMap<>();
+ factories.forEach( f -> algorithms.put( f.getName(), f.getAlgorithm() ) );
+ try ( InputStream in = inputStream )
+ {
+ ByteBuffer byteBuffer = ByteBuffer.allocate( 32 * 1024 );
Review comment:
Changing to `byte[]` for simplicity sake
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#issuecomment-1016326439
@michael-o actually no, changed my mind re `ByteBuffer`: it is so proliferated even in public API (see transfer listener etc), that I think it is okay (even w/ some overhead) to keep same thing internally as well.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787542898
##########
File path: maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/ChecksumCalculator.java
##########
@@ -46,50 +47,32 @@
static class Checksum
{
- final String algorithm;
+ final ChecksumAlgorithmFactory checksumAlgorithmFactory;
- final MessageDigest digest;
+ ChecksumAlgorithm algorithm;
Exception error;
- Checksum( String algorithm )
+ Checksum( ChecksumAlgorithmFactory checksumAlgorithmFactory )
{
- this.algorithm = algorithm;
- MessageDigest digest = null;
- try
- {
- digest = MessageDigest.getInstance( algorithm );
- }
- catch ( NoSuchAlgorithmException e )
- {
- error = e;
- }
- this.digest = digest;
+ this.checksumAlgorithmFactory = requireNonNull( checksumAlgorithmFactory );
+ this.algorithm = checksumAlgorithmFactory.getAlgorithm();
}
- public void update( ByteBuffer buffer )
+ public void reset()
{
- if ( digest != null )
- {
- digest.update( buffer );
- }
+ this.algorithm = checksumAlgorithmFactory.getAlgorithm();
Review comment:
Yes, while `MessageDigest` does support `reset()`, did not want to enforce having it on possible other implementations, is simpler IMO just to "toss away and get new instance" to reset.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported bitrot detection checksums extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r768900963
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultChecksumImplementationSelector.java
##########
@@ -0,0 +1,75 @@
+package org.eclipse.aether.internal.impl;
+
+/*
+ * 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.
+ */
+
+import java.security.NoSuchAlgorithmException;
+import java.util.Collections;
+import java.util.Map;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Provider;
+import javax.inject.Singleton;
+
+import org.eclipse.aether.spi.connector.checksum.ChecksumImplementation;
+import org.eclipse.aether.spi.connector.checksum.ChecksumImplementationSelectorSupport;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link org.eclipse.aether.spi.connector.checksum.ChecksumImplementationSelector} that
+ * is extensible.
+ *
+ * @since TBD
+ */
+@Singleton
+@Named
+public final class DefaultChecksumImplementationSelector
Review comment:
Split is due to:
1) SPI gives some "good defaults" to implementors
2) basic connectors needs impl for test (and it reuses Support one) -- basic-connector depends on SPI but not on javax.inject
3) this is a component, so needs javax.inject
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported bitrot detection checksums extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r768893893
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultChecksumImplementationSelector.java
##########
@@ -0,0 +1,75 @@
+package org.eclipse.aether.internal.impl;
+
+/*
+ * 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.
+ */
+
+import java.security.NoSuchAlgorithmException;
+import java.util.Collections;
+import java.util.Map;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Provider;
+import javax.inject.Singleton;
+
+import org.eclipse.aether.spi.connector.checksum.ChecksumImplementation;
+import org.eclipse.aether.spi.connector.checksum.ChecksumImplementationSelectorSupport;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link org.eclipse.aether.spi.connector.checksum.ChecksumImplementationSelector} that
+ * is extensible.
+ *
+ * @since TBD
+ */
+@Singleton
+@Named
+public final class DefaultChecksumImplementationSelector
Review comment:
This is default implementation of selector
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787548146
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/layout/RepositoryLayout.java
##########
@@ -83,39 +84,34 @@ public static Checksum forLocation( URI location, String algorithm )
{
throw new IllegalArgumentException( "resource location must not have a fragment: " + location );
}
- String extension = '.' + algorithm.replace( "-", "" ).toLowerCase( Locale.ENGLISH );
- return new Checksum( algorithm, URI.create( location.toString() + extension ) );
+ return new ChecksumLocation( URI.create( location + "." + checksumAlgorithmFactory.getExtension() ),
+ checksumAlgorithmFactory );
}
- private static void verify( String algorithm, URI location )
+ private static void verify( URI location, ChecksumAlgorithmFactory checksumAlgorithmFactory )
{
- requireNonNull( algorithm, "checksum algorithm cannot be null" );
- if ( algorithm.length() == 0 )
- {
- throw new IllegalArgumentException( "checksum algorithm cannot be empty" );
- }
requireNonNull( location, "checksum location cannot be null" );
if ( location.isAbsolute() )
{
throw new IllegalArgumentException( "checksum location must be relative" );
}
+ requireNonNull( checksumAlgorithmFactory, "checksum algorithm cannot be null" );
Review comment:
fixed
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#issuecomment-1016279906
@michael-o other "global" answers:
* algorithm names `Set` vs `List`: important distinction, factory really does not propose any "order", hence it gives you the set of supported algorithm names w/o any order. OTOH, the **order of checksums in layout** is very important: that's the order how resolver will ask for them (and "first found, the logic stops" -- hence, today if you have SHA-1 ONLY but no MD5, build will still work as resolver with be satisfied with SHA-1 that it asks for the first time). Hence, order when used in layout is important. This is reflected by the fact, that selector gives you (unordered) names it supports, that is usually superset of the names used by layout. OTOH, the layout DOES DEFINE the order, as actually that is the order it asks for, Finally, due this, the user config (comma separated LIST is a list, has order, but we also want to make sure it is unique -- `SHA-1, MD5, MD5` is wrong).
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #139: [MRESOLVER-230] Make supported bitrot detection checksums extensible
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r768795479
##########
File path: maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/ChecksumImplementation.java
##########
@@ -0,0 +1,41 @@
+package org.eclipse.aether.connector.basic;
+
+/*
+ * 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.
+ */
+
+import java.nio.ByteBuffer;
+
+/**
+ * Implementation performing checksum calculation for specific algorithm. Instances of this interface as stateful,
+ * should be reused only after {@link #reset()}.
+ *
+ * <strong>Important note:</strong> if implementation of this interface is component, it MUST NOT be singleton, but
+ * must adhere to "prototype" pattern. Alternative is just to implement a provider, and on each get invocation create
+ * new instance.
+ *
+ * @since TBD
+ */
+public interface ChecksumImplementation
+{
+ void update( ByteBuffer input );
+
+ void reset();
+
+ byte[] digest();
Review comment:
I think `digest()` is the wrong name since we don't need digests here. Do we? `#checksum()` would be it.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported bitrot detection checksums extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r768900963
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultChecksumImplementationSelector.java
##########
@@ -0,0 +1,75 @@
+package org.eclipse.aether.internal.impl;
+
+/*
+ * 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.
+ */
+
+import java.security.NoSuchAlgorithmException;
+import java.util.Collections;
+import java.util.Map;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Provider;
+import javax.inject.Singleton;
+
+import org.eclipse.aether.spi.connector.checksum.ChecksumImplementation;
+import org.eclipse.aether.spi.connector.checksum.ChecksumImplementationSelectorSupport;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link org.eclipse.aether.spi.connector.checksum.ChecksumImplementationSelector} that
+ * is extensible.
+ *
+ * @since TBD
+ */
+@Singleton
+@Named
+public final class DefaultChecksumImplementationSelector
Review comment:
Split is due to:
1) SPI gives some "good defaults" to implementors
2) basic connectors needs impl for test (and it reuses Support one) -- basic-connector depends on SPI but not on javax.inject
3) this is a component, so needs javax.inject, so it can go into impl only, but then basic-connector misses it
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported bitrot detection checksums extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r768900963
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultChecksumImplementationSelector.java
##########
@@ -0,0 +1,75 @@
+package org.eclipse.aether.internal.impl;
+
+/*
+ * 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.
+ */
+
+import java.security.NoSuchAlgorithmException;
+import java.util.Collections;
+import java.util.Map;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Provider;
+import javax.inject.Singleton;
+
+import org.eclipse.aether.spi.connector.checksum.ChecksumImplementation;
+import org.eclipse.aether.spi.connector.checksum.ChecksumImplementationSelectorSupport;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link org.eclipse.aether.spi.connector.checksum.ChecksumImplementationSelector} that
+ * is extensible.
+ *
+ * @since TBD
+ */
+@Singleton
+@Named
+public final class DefaultChecksumImplementationSelector
Review comment:
Split is due to:
1) SPI gives some "good defaults" to implementors
2) basic connectors needs impl for test (and it reuses Support one) -- basic depends on SPI but not on javax.inject
3) this is a component, so needs javax.inject
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787566790
##########
File path: maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/BasicRepositoryConnector.java
##########
@@ -495,16 +510,16 @@ protected void runTask()
}
class PutTaskRunner
- extends TaskRunner
+ extends TaskRunner
{
private final File file;
private final FileTransformer fileTransformer;
- private final Collection<RepositoryLayout.Checksum> checksums;
+ private final Collection<RepositoryLayout.ChecksumLocation> checksums;
Review comment:
Fixed
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/layout/RepositoryLayout.java
##########
@@ -36,45 +37,45 @@
{
/**
- * A descriptor for a checksum file. This descriptor simply associates the location of a checksum file with the
- * underlying algorithm used to calculate/verify it. Checksum algorithms are denoted by names as used with
- * {@link java.security.MessageDigest#getInstance(String)}, e.g. {@code "SHA-1"} or {@code "MD5"}.
+ * A descriptor for a checksum location. This descriptor simply associates the location of a checksum file with the
+ * underlying checksum algorithm used to calculate/verify it.
*/
- final class Checksum
+ final class ChecksumLocation
Review comment:
Agreed
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787567347
##########
File path: maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/BasicRepositoryConnector.java
##########
@@ -571,25 +587,26 @@ private void uploadChecksums( File file, byte[] bytes )
}
try
{
- Set<String> algos = new HashSet<>();
- for ( RepositoryLayout.Checksum checksum : checksums )
+ ArrayList<ChecksumAlgorithmFactory> algorithms = new ArrayList<>();
+ for ( RepositoryLayout.ChecksumLocation checksumLocation : checksums )
{
- algos.add( checksum.getAlgorithm() );
+ algorithms.add( checksumLocation.getChecksumAlgorithmFactory() );
}
- Map<String, Object> sumsByAlgo;
+ Map<String, String> sumsByAlgo;
if ( bytes != null )
{
- sumsByAlgo = ChecksumUtils.calc( bytes, algos );
+ sumsByAlgo = ChecksumAlgorithmHelper.calculate( bytes, algorithms );
}
else
{
- sumsByAlgo = ChecksumUtils.calc( file, algos );
+ sumsByAlgo = ChecksumAlgorithmHelper.calculate( file, algorithms );
}
- for ( RepositoryLayout.Checksum checksum : checksums )
+ for ( RepositoryLayout.ChecksumLocation checksum : checksums )
Review comment:
Fixed
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787601048
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumAlgorithm.java
##########
@@ -0,0 +1,46 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * 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.
+ */
+
+import java.nio.ByteBuffer;
+
+/**
+ * Implementation performing checksum calculation for specific algorithm. Instances of this interface are stateful,
+ * non-thread safe, and should not be reused.
+ *
+ * @since TBD
+ */
+public interface ChecksumAlgorithm
+{
+ /**
+ * Updates the checksum algorithm inner state with input.
+ */
+ void update( ByteBuffer input );
Review comment:
frankly, am more for `byte[]` due simplicity, but did not want to stir a lot around this code.... will make it `byte[]`
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787546876
##########
File path: src/site/markdown/about-checksums.md
##########
@@ -0,0 +1,72 @@
+# About checksums
+<!--
+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.
+-->
+
+Maven Resolver uses "checksums" to verify the consistency of the downloaded
+artifacts. Checksums are usually laid out in repositories next to the file in question, with file
+extension telling the checksum algorithm that produced the given checksum file content. Current Maven Repository
+layout contains SHA-1 and MD5 checksums by default (they are produced by Resolver by default).
+
+Historically, Maven Resolver used `java.security.MessageDigest` to implement checksums. So to say, secure one-way
+hashes provided by Java Cryptography Architecture were (mis)used to implement checksums for transport integrity
+validation. There is no misunderstanding here, secure hashes MAY be used as checksums, as there is quite some
+overlap between "checksums" and "hashes" in general. But this simplicity comes at a price: cryptographically "safe"
+algorithms require way more CPU cycles to calculate checksum, while all their purpose is just
+"integrity validation", nothing more. There is no "security", "trust" or whatever else implied or expected from
+them.
+
+If you are interested in "trust" in your artifacts, it is Artifact Signatures (for example
+[GPG Signatures](https://maven.apache.org/plugins/maven-gpg-plugin/)) that you should look for.
+
+Hence, the usual argument that "XXX algorithm is unsafe, deprecated, not secure anymore" does not stand in use case
+of Maven Resolver: there is nothing "secure" being involved with checksums. Moreover, this is true not only for SHA-1
+algorithm, but even for it's "elder brother" MD5. Both algorithms are still widely used today as "transport integrity
Review comment:
fixed
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#issuecomment-1022019745
Thanks, scheduled for this week to re-review.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#issuecomment-1022018041
@michael-o @gnodet ping
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] kwin commented on pull request #139: [MRESOLVER-230] Make supported bitrot detection checksums extensible
Posted by GitBox <gi...@apache.org>.
kwin commented on pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#issuecomment-995198780
Can you adjust https://github.com/apache/maven-resolver/blob/master/src/site/markdown/configuration.md on how to configure this (probably requires adjustment of description for `aether.checksums.algorithms`)?
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #139: [MRESOLVER-230] Make supported bitrot detection checksums extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#issuecomment-995552306
> Can you adjust https://github.com/apache/maven-resolver/blob/master/src/site/markdown/configuration.md on how to configure this (probably requires adjustment of description for `aether.checksums.algorithms`)?
@kwin once this merged nothing changes re that doco, as "out of the box" resolver will still fallback to `MessageDigest`. But, at the same time this opens a door to extend checksum support, and probably resolver itself will add some new ones (but not in 1.x line, probably 2.x line).
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #139: [MRESOLVER-230] Make supported bitrot detection checksums extensible
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r768837596
##########
File path: maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/ChecksumCalculator.java
##########
@@ -125,7 +131,14 @@ private ChecksumCalculator( File targetFile, Collection<RepositoryLayout.Checksu
String algo = checksum.getAlgorithm();
if ( algos.add( algo ) )
{
- this.checksums.add( new Checksum( algo ) );
+ try
+ {
+ this.checksums.add( new Checksum( algo, selector.select( algo ) ) );
+ }
+ catch ( NoSuchAlgorithmException e )
+ {
+ this.checksums.add( new Checksum( algo, e ) );
Review comment:
I never liked this abuse approach, but should not be addressed here.
##########
File path: maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/ChecksumValidator.java
##########
@@ -65,26 +66,32 @@ boolean fetchChecksum( URI remote, File local )
private final Collection<Checksum> checksums;
+ private final ChecksumImplementationSelector checksumImplementationSelector;
+
private final Map<File, Object> checksumFiles;
- ChecksumValidator( File dataFile, FileProcessor fileProcessor,
- ChecksumFetcher checksumFetcher, ChecksumPolicy checksumPolicy,
- Collection<Checksum> checksums )
+ ChecksumValidator( File dataFile,
+ FileProcessor fileProcessor,
+ ChecksumFetcher checksumFetcher,
+ ChecksumPolicy checksumPolicy,
+ Collection<Checksum> checksums,
+ ChecksumImplementationSelector checksumImplementationSelector )
Review comment:
This order is not consistent with other ctors. In other ctors you have added the selector after the checksum policy. Did you do this for backward compat?
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultChecksumImplementationSelector.java
##########
@@ -0,0 +1,75 @@
+package org.eclipse.aether.internal.impl;
+
+/*
+ * 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.
+ */
+
+import java.security.NoSuchAlgorithmException;
+import java.util.Collections;
+import java.util.Map;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Provider;
+import javax.inject.Singleton;
+
+import org.eclipse.aether.spi.connector.checksum.ChecksumImplementation;
+import org.eclipse.aether.spi.connector.checksum.ChecksumImplementationSelectorSupport;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link org.eclipse.aether.spi.connector.checksum.ChecksumImplementationSelector} that
+ * is extensible.
+ *
+ * @since TBD
+ */
+@Singleton
+@Named
+public final class DefaultChecksumImplementationSelector
+ extends ChecksumImplementationSelectorSupport
+{
+ private final Map<String, Provider<ChecksumImplementation>> providers;
+
+ /**
+ * Ctor for ServiceLocator.
+ */
+ public DefaultChecksumImplementationSelector()
Review comment:
You should deprecate this ctor right away since it is going to be removed anyway.
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumImplementation.java
##########
@@ -0,0 +1,41 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * 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.
+ */
+
+import java.nio.ByteBuffer;
+
+/**
+ * Implementation performing checksum calculation for specific algorithm. Instances of this interface as stateful,
+ * should be reused only after {@link #reset()}.
+ *
+ * <strong>Important note:</strong> if implementation of this interface is component, it MUST NOT be singleton, but
Review comment:
Correct!
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumImplementation.java
##########
@@ -0,0 +1,41 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * 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.
+ */
+
+import java.nio.ByteBuffer;
+
+/**
+ * Implementation performing checksum calculation for specific algorithm. Instances of this interface as stateful,
Review comment:
as => are
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumImplementation.java
##########
@@ -0,0 +1,41 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * 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.
+ */
+
+import java.nio.ByteBuffer;
+
+/**
+ * Implementation performing checksum calculation for specific algorithm. Instances of this interface as stateful,
+ * should be reused only after {@link #reset()}.
+ *
+ * <strong>Important note:</strong> if implementation of this interface is component, it MUST NOT be singleton, but
+ * must adhere to "prototype" pattern. Alternative is just to implement a provider, and on each get invocation create
+ * new instance.
+ *
+ * @since TBD
+ */
+public interface ChecksumImplementation
+{
+ void update( ByteBuffer input );
+
+ void reset();
+
+ byte[] digest();
Review comment:
Should be renamed
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultChecksumImplementationSelector.java
##########
@@ -0,0 +1,75 @@
+package org.eclipse.aether.internal.impl;
+
+/*
+ * 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.
+ */
+
+import java.security.NoSuchAlgorithmException;
+import java.util.Collections;
+import java.util.Map;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Provider;
+import javax.inject.Singleton;
+
+import org.eclipse.aether.spi.connector.checksum.ChecksumImplementation;
+import org.eclipse.aether.spi.connector.checksum.ChecksumImplementationSelectorSupport;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link org.eclipse.aether.spi.connector.checksum.ChecksumImplementationSelector} that
+ * is extensible.
+ *
+ * @since TBD
+ */
+@Singleton
+@Named
+public final class DefaultChecksumImplementationSelector
Review comment:
This isn't a default impl, but rather a `delegating`/`fallback` implementation.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787563671
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumAlgorithmHelper.java
##########
@@ -0,0 +1,106 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * 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.
+ */
+
+import java.io.BufferedInputStream;
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.Buffer;
+import java.nio.ByteBuffer;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Helper for checksum operations.
+ *
+ * @since TBD
+ */
+public final class ChecksumAlgorithmHelper
+{
+ private ChecksumAlgorithmHelper()
+ {
+ // nop
+ }
+
+ /**
+ * Calculates checksums for specified data.
+ *
+ * @param data The content for which to calculate checksums, must not be {@code null}.
+ * @param factories The checksum algorithm factories to use, must not be {@code null}.
+ * @return The calculated checksums, indexed by algorithm name, or the exception that occurred while trying to
+ * calculate it, never {@code null}.
+ * @throws IOException In case of any problem.
+ */
+ public static Map<String, String> calculate( byte[] data, List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ return calculate( new ByteArrayInputStream( data ), factories );
+ }
+
+ /**
+ * Calculates checksums for specified file.
+ *
+ * @param file The file for which to calculate checksums, must not be {@code null}.
+ * @param factories The checksum algorithm factories to use, must not be {@code null}.
+ * @return The calculated checksums, indexed by algorithm name, or the exception that occurred while trying to
+ * calculate it, never {@code null}.
+ * @throws IOException In case of any problem.
+ */
+ public static Map<String, String> calculate( File file, List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ return calculate( new BufferedInputStream( new FileInputStream( file ) ), factories );
+ }
+
+ private static Map<String, String> calculate( InputStream inputStream, List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ LinkedHashMap<String, ChecksumAlgorithm> algorithms = new LinkedHashMap<>();
+ factories.forEach( f -> algorithms.put( f.getName(), f.getAlgorithm() ) );
+ try ( InputStream in = inputStream )
Review comment:
Agreed, before this method was public but made into private, fixed
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787568046
##########
File path: maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/ChecksumCalculator.java
##########
@@ -116,16 +100,16 @@ public static ChecksumCalculator newInstance( File targetFile, Collection<Reposi
return new ChecksumCalculator( targetFile, checksums );
}
- private ChecksumCalculator( File targetFile, Collection<RepositoryLayout.Checksum> checksums )
+ private ChecksumCalculator( File targetFile,
+ Collection<RepositoryLayout.ChecksumLocation> checksums )
{
this.checksums = new ArrayList<>();
Set<String> algos = new HashSet<>();
- for ( RepositoryLayout.Checksum checksum : checksums )
+ for ( RepositoryLayout.ChecksumLocation checksum : checksums )
Review comment:
Fixed
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas edited a comment on pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas edited a comment on pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#issuecomment-1016279906
@michael-o other "global" answers:
* algorithm names `Set` vs `List`: important distinction, ~~factory~~ selector really does not propose any "order", hence it gives you the set of supported algorithm names w/o any order. OTOH, the **order of checksums in layout** is very important: that's the order how resolver will ask for them (and "first found, the logic stops" -- hence, today if you have SHA-1 ONLY but no MD5, build will still work as resolver with be satisfied with SHA-1 that it asks for the first time). Hence, order when used in layout is important. This is reflected by the fact, that selector gives you (unordered) names it supports, that is usually superset of the names used by layout. OTOH, the layout DOES DEFINE the order, as actually that is the order it asks for, Finally, due this, the user config (comma separated LIST is a list, has order, but we also want to make sure it is unique -- `SHA-1, MD5, MD5` is wrong).
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] kwin commented on pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
kwin commented on pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#issuecomment-1016313402
My bad, I thought that https://docs.oracle.com/javase/8/docs/api/java/util/LinkedHashSet.html was implementing a `SortedSet` (set with an order), but it does not. Also `SortedSet` does always assume that the sorting is based on a comparator or natural ordering which is obviously not true for `LinkedHashSet`. So keeping `List` is fine for me (maybe mentioning that every item is only supposed to appear once).
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#issuecomment-1016296713
> This is exactly what is needed here IIUC.
I don't quite get you: one run I can configure maven like `-Daether.checksums.algorithms=SHA-1,MD5` and other run I can use `-Daether.checksums.algorithms=MD5,SHA-1`... this means that in first run I want resolver to first ask for SHA-1s (like it does by default), then in second run I want resolver to first ask MD5s for some reason (reversed order).
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas edited a comment on pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas edited a comment on pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#issuecomment-1016296713
> This is exactly what is needed here IIUC.
I don't quite get you: one run I can configure maven like `-Daether.checksums.algorithms=SHA-1,MD5` and other run I can use `-Daether.checksums.algorithms=MD5,SHA-1`... this means that in first run I want resolver to first ask for SHA-1s (like it does by default), then in second run I want resolver to first ask MD5s for some reason (reversed order). So in case of layout, it is more "user ordered list".
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#issuecomment-1023188647
OK, let me put this into testing...
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r784760358
##########
File path: maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/BasicRepositoryConnector.java
##########
@@ -571,25 +587,26 @@ private void uploadChecksums( File file, byte[] bytes )
}
try
{
- Set<String> algos = new HashSet<>();
- for ( RepositoryLayout.Checksum checksum : checksums )
+ ArrayList<ChecksumAlgorithmFactory> algorithms = new ArrayList<>();
+ for ( RepositoryLayout.ChecksumLocation checksumLocation : checksums )
{
- algos.add( checksum.getAlgorithm() );
+ algorithms.add( checksumLocation.getChecksumAlgorithmFactory() );
}
- Map<String, Object> sumsByAlgo;
+ Map<String, String> sumsByAlgo;
if ( bytes != null )
{
- sumsByAlgo = ChecksumUtils.calc( bytes, algos );
+ sumsByAlgo = ChecksumAlgorithmHelper.calculate( bytes, algorithms );
}
else
{
- sumsByAlgo = ChecksumUtils.calc( file, algos );
+ sumsByAlgo = ChecksumAlgorithmHelper.calculate( file, algorithms );
}
- for ( RepositoryLayout.Checksum checksum : checksums )
+ for ( RepositoryLayout.ChecksumLocation checksum : checksums )
Review comment:
Same here
##########
File path: maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/BasicRepositoryConnector.java
##########
@@ -495,16 +510,16 @@ protected void runTask()
}
class PutTaskRunner
- extends TaskRunner
+ extends TaskRunner
{
private final File file;
private final FileTransformer fileTransformer;
- private final Collection<RepositoryLayout.Checksum> checksums;
+ private final Collection<RepositoryLayout.ChecksumLocation> checksums;
Review comment:
`checksums` => `checksumLocations`?
##########
File path: maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/BasicRepositoryConnector.java
##########
@@ -571,25 +587,26 @@ private void uploadChecksums( File file, byte[] bytes )
}
try
{
- Set<String> algos = new HashSet<>();
- for ( RepositoryLayout.Checksum checksum : checksums )
+ ArrayList<ChecksumAlgorithmFactory> algorithms = new ArrayList<>();
+ for ( RepositoryLayout.ChecksumLocation checksumLocation : checksums )
{
- algos.add( checksum.getAlgorithm() );
+ algorithms.add( checksumLocation.getChecksumAlgorithmFactory() );
}
- Map<String, Object> sumsByAlgo;
+ Map<String, String> sumsByAlgo;
Review comment:
Since the value is not an object anymore `uploadChecksum()` cannot throw with `checksum instanceof Exception`. Can you explain how this is supposed to work?
##########
File path: maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/ChecksumCalculator.java
##########
@@ -46,50 +47,32 @@
static class Checksum
{
- final String algorithm;
+ final ChecksumAlgorithmFactory checksumAlgorithmFactory;
- final MessageDigest digest;
+ ChecksumAlgorithm algorithm;
Exception error;
- Checksum( String algorithm )
+ Checksum( ChecksumAlgorithmFactory checksumAlgorithmFactory )
{
- this.algorithm = algorithm;
- MessageDigest digest = null;
- try
- {
- digest = MessageDigest.getInstance( algorithm );
- }
- catch ( NoSuchAlgorithmException e )
- {
- error = e;
- }
- this.digest = digest;
+ this.checksumAlgorithmFactory = requireNonNull( checksumAlgorithmFactory );
+ this.algorithm = checksumAlgorithmFactory.getAlgorithm();
}
- public void update( ByteBuffer buffer )
+ public void reset()
{
- if ( digest != null )
- {
- digest.update( buffer );
- }
+ this.algorithm = checksumAlgorithmFactory.getAlgorithm();
Review comment:
So the `ChecksumAlgorithm` does not support a `reset()` directly and a new object is required?
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/layout/RepositoryLayout.java
##########
@@ -36,45 +37,45 @@
{
/**
- * A descriptor for a checksum file. This descriptor simply associates the location of a checksum file with the
- * underlying algorithm used to calculate/verify it. Checksum algorithms are denoted by names as used with
- * {@link java.security.MessageDigest#getInstance(String)}, e.g. {@code "SHA-1"} or {@code "MD5"}.
+ * A descriptor for a checksum location. This descriptor simply associates the location of a checksum file with the
+ * underlying checksum algorithm used to calculate/verify it.
*/
- final class Checksum
+ final class ChecksumLocation
Review comment:
Reasonable rename since the class does not contain a checksum.
##########
File path: maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/ChecksumCalculator.java
##########
@@ -116,16 +100,16 @@ public static ChecksumCalculator newInstance( File targetFile, Collection<Reposi
return new ChecksumCalculator( targetFile, checksums );
}
- private ChecksumCalculator( File targetFile, Collection<RepositoryLayout.Checksum> checksums )
+ private ChecksumCalculator( File targetFile,
+ Collection<RepositoryLayout.ChecksumLocation> checksums )
{
this.checksums = new ArrayList<>();
Set<String> algos = new HashSet<>();
- for ( RepositoryLayout.Checksum checksum : checksums )
+ for ( RepositoryLayout.ChecksumLocation checksum : checksums )
Review comment:
Rename `checksum`
##########
File path: maven-resolver-connector-basic/src/test/java/org/eclipse/aether/connector/basic/ChecksumCalculatorTest.java
##########
@@ -39,22 +42,16 @@
public class ChecksumCalculatorTest
{
- private static final String SHA512 = "SHA-512";
-
- private static final String SHA256 = "SHA-256";
-
- private static final String SHA1 = "SHA-1";
-
- private static final String MD5 = "MD5";
-
private File file;
+ private final TestChecksumAlgorithmSelector selector = new TestChecksumAlgorithmSelector();
+
private ChecksumCalculator newCalculator( String... algos )
{
- List<RepositoryLayout.Checksum> checksums = new ArrayList<>();
+ List<RepositoryLayout.ChecksumLocation> checksums = new ArrayList<>();
Review comment:
Rename
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/Maven2RepositoryLayoutFactory.java
##########
@@ -83,23 +106,33 @@ public RepositoryLayout newInstance( RepositorySystemSession session, RemoteRepo
throw new NoRepositoryLayoutException( repository );
}
boolean forSignature = ConfigUtils.getBoolean( session, false, CONFIG_PROP_SIGNATURE_CHECKSUMS );
- List<String> checksumsAlgorithms = Arrays.asList( ConfigUtils.getString( session,
- DEFAULT_CHECKSUMS_ALGORITHMS, CONFIG_PROP_CHECKSUMS_ALGORITHMS ).split( "," ) );
+ // ensure order of (potentially user set) algorithm list is kept and is unique
+ LinkedHashSet<String> checksumsAlgorithmNames = new LinkedHashSet<>( Arrays.asList(
Review comment:
We don't do this for other config options. I don't know whether we should do this just for one.
##########
File path: maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/ChecksumValidator.java
##########
@@ -63,21 +62,23 @@ boolean fetchChecksum( URI remote, File local )
private final ChecksumPolicy checksumPolicy;
- private final Collection<Checksum> checksums;
+ private final Collection<ChecksumLocation> checksums;
Review comment:
Rename
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumAlgorithmFactorySelector.java
##########
@@ -0,0 +1,44 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * 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.
+ */
+
+import java.util.Set;
+
+/**
+ * Component performing selection of {@link ChecksumAlgorithmFactory} based on known factory names.
+ *
+ * @since TBD
+ */
+public interface ChecksumAlgorithmFactorySelector
+{
+ /**
+ * Returns factory for given algorithm name, or throws if algorithm not supported.
+ *
+ * @throws IllegalArgumentException if asked algorithm name is not supported.
+ */
+ ChecksumAlgorithmFactory select( String algorithmName ) throws IllegalArgumentException;
+
+ /**
+ * Returns a set of supported algorithm names. This set represents ALL the algorithms supported by Resolver, and is
+ * NOT in any relation to given repository layout used checksums, returned by method {@link
+ * org.eclipse.aether.spi.connector.layout.RepositoryLayout#getChecksumAlgorithmNames()} (is super set of it).
+ */
+ Set<String> getChecksumAlgorithmNames();
Review comment:
Do we need the semantics of `Set` or a a read-only collection suffcient?
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumAlgorithmHelper.java
##########
@@ -0,0 +1,106 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * 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.
+ */
+
+import java.io.BufferedInputStream;
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.Buffer;
+import java.nio.ByteBuffer;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Helper for checksum operations.
+ *
+ * @since TBD
+ */
+public final class ChecksumAlgorithmHelper
+{
+ private ChecksumAlgorithmHelper()
+ {
+ // nop
+ }
+
+ /**
+ * Calculates checksums for specified data.
+ *
+ * @param data The content for which to calculate checksums, must not be {@code null}.
+ * @param factories The checksum algorithm factories to use, must not be {@code null}.
+ * @return The calculated checksums, indexed by algorithm name, or the exception that occurred while trying to
+ * calculate it, never {@code null}.
+ * @throws IOException In case of any problem.
+ */
+ public static Map<String, String> calculate( byte[] data, List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ return calculate( new ByteArrayInputStream( data ), factories );
+ }
+
+ /**
+ * Calculates checksums for specified file.
+ *
+ * @param file The file for which to calculate checksums, must not be {@code null}.
+ * @param factories The checksum algorithm factories to use, must not be {@code null}.
+ * @return The calculated checksums, indexed by algorithm name, or the exception that occurred while trying to
+ * calculate it, never {@code null}.
+ * @throws IOException In case of any problem.
+ */
+ public static Map<String, String> calculate( File file, List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ return calculate( new BufferedInputStream( new FileInputStream( file ) ), factories );
Review comment:
This should be rather in a try-with-resources clause.
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumAlgorithmHelper.java
##########
@@ -0,0 +1,106 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * 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.
+ */
+
+import java.io.BufferedInputStream;
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.Buffer;
+import java.nio.ByteBuffer;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Helper for checksum operations.
+ *
+ * @since TBD
+ */
+public final class ChecksumAlgorithmHelper
+{
+ private ChecksumAlgorithmHelper()
+ {
+ // nop
+ }
+
+ /**
+ * Calculates checksums for specified data.
+ *
+ * @param data The content for which to calculate checksums, must not be {@code null}.
+ * @param factories The checksum algorithm factories to use, must not be {@code null}.
+ * @return The calculated checksums, indexed by algorithm name, or the exception that occurred while trying to
+ * calculate it, never {@code null}.
+ * @throws IOException In case of any problem.
+ */
+ public static Map<String, String> calculate( byte[] data, List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ return calculate( new ByteArrayInputStream( data ), factories );
+ }
+
+ /**
+ * Calculates checksums for specified file.
+ *
+ * @param file The file for which to calculate checksums, must not be {@code null}.
+ * @param factories The checksum algorithm factories to use, must not be {@code null}.
+ * @return The calculated checksums, indexed by algorithm name, or the exception that occurred while trying to
+ * calculate it, never {@code null}.
+ * @throws IOException In case of any problem.
+ */
+ public static Map<String, String> calculate( File file, List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ return calculate( new BufferedInputStream( new FileInputStream( file ) ), factories );
+ }
+
+ private static Map<String, String> calculate( InputStream inputStream, List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ LinkedHashMap<String, ChecksumAlgorithm> algorithms = new LinkedHashMap<>();
+ factories.forEach( f -> algorithms.put( f.getName(), f.getAlgorithm() ) );
+ try ( InputStream in = inputStream )
Review comment:
I don't like this. We haven't opened the IS, we should not close this.
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/named/DiscriminatingNameMapper.java
##########
@@ -115,25 +113,22 @@ private String createDiscriminator( final RepositorySystemSession session )
String hostname = ConfigUtils.getString( session, this.hostname, CONFIG_PROP_HOSTNAME );
File basedir = session.getLocalRepository().getBasedir();
discriminator = hostname + ":" + basedir;
- try
- {
- Map<String, Object> checksums = ChecksumUtils
- .calc( discriminator.getBytes( StandardCharsets.UTF_8 ), Collections.singletonList( "SHA-1" ) );
- Object checksum = checksums.get( "SHA-1" );
-
- if ( checksum instanceof Exception )
- {
- throw (Exception) checksum;
- }
-
- return String.valueOf( checksum );
- }
- catch ( Exception e )
- {
- LOGGER.warn( "Failed to calculate discriminator digest, using '{}'", DEFAULT_DISCRIMINATOR_DIGEST, e );
- return DEFAULT_DISCRIMINATOR_DIGEST;
- }
+ return sha1String( discriminator );
Review comment:
Can we leave this block as-is right now since it is not related for artifact/metadata checksums at all? I actually need a fixed length nash to properly map input data.
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumAlgorithmHelper.java
##########
@@ -0,0 +1,106 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * 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.
+ */
+
+import java.io.BufferedInputStream;
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.Buffer;
+import java.nio.ByteBuffer;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Helper for checksum operations.
+ *
+ * @since TBD
+ */
+public final class ChecksumAlgorithmHelper
+{
+ private ChecksumAlgorithmHelper()
+ {
+ // nop
+ }
+
+ /**
+ * Calculates checksums for specified data.
+ *
+ * @param data The content for which to calculate checksums, must not be {@code null}.
+ * @param factories The checksum algorithm factories to use, must not be {@code null}.
+ * @return The calculated checksums, indexed by algorithm name, or the exception that occurred while trying to
+ * calculate it, never {@code null}.
+ * @throws IOException In case of any problem.
+ */
+ public static Map<String, String> calculate( byte[] data, List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ return calculate( new ByteArrayInputStream( data ), factories );
+ }
+
+ /**
+ * Calculates checksums for specified file.
+ *
+ * @param file The file for which to calculate checksums, must not be {@code null}.
+ * @param factories The checksum algorithm factories to use, must not be {@code null}.
+ * @return The calculated checksums, indexed by algorithm name, or the exception that occurred while trying to
+ * calculate it, never {@code null}.
+ * @throws IOException In case of any problem.
+ */
+ public static Map<String, String> calculate( File file, List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ return calculate( new BufferedInputStream( new FileInputStream( file ) ), factories );
+ }
+
+ private static Map<String, String> calculate( InputStream inputStream, List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ LinkedHashMap<String, ChecksumAlgorithm> algorithms = new LinkedHashMap<>();
+ factories.forEach( f -> algorithms.put( f.getName(), f.getAlgorithm() ) );
+ try ( InputStream in = inputStream )
+ {
+ ByteBuffer byteBuffer = ByteBuffer.allocate( 32 * 1024 );
Review comment:
What is the benefit of a byte buffer compared to a pure byte array?
##########
File path: src/site/markdown/about-checksums.md
##########
@@ -0,0 +1,72 @@
+# About checksums
+<!--
+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.
+-->
+
+Maven Resolver uses "checksums" to verify the consistency of the downloaded
+artifacts. Checksums are usually laid out in repositories next to the file in question, with file
+extension telling the checksum algorithm that produced the given checksum file content. Current Maven Repository
+layout contains SHA-1 and MD5 checksums by default (they are produced by Resolver by default).
+
+Historically, Maven Resolver used `java.security.MessageDigest` to implement checksums. So to say, secure one-way
+hashes provided by Java Cryptography Architecture were (mis)used to implement checksums for transport integrity
+validation. There is no misunderstanding here, secure hashes MAY be used as checksums, as there is quite some
+overlap between "checksums" and "hashes" in general. But this simplicity comes at a price: cryptographically "safe"
+algorithms require way more CPU cycles to calculate checksum, while all their purpose is just
+"integrity validation", nothing more. There is no "security", "trust" or whatever else implied or expected from
Review comment:
Very good.
##########
File path: src/site/markdown/about-checksums.md
##########
@@ -0,0 +1,72 @@
+# About checksums
+<!--
+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.
+-->
+
+Maven Resolver uses "checksums" to verify the consistency of the downloaded
Review comment:
Why in quotes?
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/layout/RepositoryLayout.java
##########
@@ -83,39 +84,34 @@ public static Checksum forLocation( URI location, String algorithm )
{
throw new IllegalArgumentException( "resource location must not have a fragment: " + location );
}
- String extension = '.' + algorithm.replace( "-", "" ).toLowerCase( Locale.ENGLISH );
- return new Checksum( algorithm, URI.create( location.toString() + extension ) );
+ return new ChecksumLocation( URI.create( location + "." + checksumAlgorithmFactory.getExtension() ),
+ checksumAlgorithmFactory );
}
- private static void verify( String algorithm, URI location )
+ private static void verify( URI location, ChecksumAlgorithmFactory checksumAlgorithmFactory )
{
- requireNonNull( algorithm, "checksum algorithm cannot be null" );
- if ( algorithm.length() == 0 )
- {
- throw new IllegalArgumentException( "checksum algorithm cannot be empty" );
- }
requireNonNull( location, "checksum location cannot be null" );
if ( location.isAbsolute() )
{
throw new IllegalArgumentException( "checksum location must be relative" );
}
+ requireNonNull( checksumAlgorithmFactory, "checksum algorithm cannot be null" );
Review comment:
factory missing
##########
File path: src/site/markdown/about-checksums.md
##########
@@ -0,0 +1,72 @@
+# About checksums
+<!--
+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.
+-->
+
+Maven Resolver uses "checksums" to verify the consistency of the downloaded
+artifacts. Checksums are usually laid out in repositories next to the file in question, with file
+extension telling the checksum algorithm that produced the given checksum file content. Current Maven Repository
+layout contains SHA-1 and MD5 checksums by default (they are produced by Resolver by default).
+
+Historically, Maven Resolver used `java.security.MessageDigest` to implement checksums. So to say, secure one-way
+hashes provided by Java Cryptography Architecture were (mis)used to implement checksums for transport integrity
+validation. There is no misunderstanding here, secure hashes MAY be used as checksums, as there is quite some
+overlap between "checksums" and "hashes" in general. But this simplicity comes at a price: cryptographically "safe"
+algorithms require way more CPU cycles to calculate checksum, while all their purpose is just
+"integrity validation", nothing more. There is no "security", "trust" or whatever else implied or expected from
+them.
+
+If you are interested in "trust" in your artifacts, it is Artifact Signatures (for example
+[GPG Signatures](https://maven.apache.org/plugins/maven-gpg-plugin/)) that you should look for.
+
+Hence, the usual argument that "XXX algorithm is unsafe, deprecated, not secure anymore" does not stand in use case
+of Maven Resolver: there is nothing "secure" being involved with checksums. Moreover, this is true not only for SHA-1
+algorithm, but even for it's "elder brother" MD5. Both algorithms are still widely used today as "transport integrity
Review comment:
its
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumAlgorithmFactory.java
##########
@@ -0,0 +1,48 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * 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.
+ */
+
+/**
+ * A component representing a checksum factory: provides {@link ChecksumAlgorithm} instances, name and extension to be
+ * used with this algorithm. While directly injecting components of this type is possible, it is not recommended. To
+ * obtain factory instances use {@link ChecksumAlgorithmFactorySelector} instead.
+ *
+ * @since TBD
+ */
+public interface ChecksumAlgorithmFactory
+{
+ /**
+ * Returns the algorithm name, usually used as key, never {@code null} value. The name is a standard name of
+ * algorithm (if applicable) or any other designator that is algorithm commonly referred with. Example: "SHA-1".
+ */
+ String getName();
+
+ /**
+ * Returns the file extension to be used for given checksum file (without leading dot), never {@code null}. The
+ * extension should be file and URL path friendly, and usually differs from value returned by {@link #getName()}.
+ * Example: "sha1".
+ */
+ String getExtension();
Review comment:
Maybe this should rather be `getFileExtension()`/`getPathExtension()`. The context of "extension" isn't clear w/o reading the Javadoc.
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumAlgorithm.java
##########
@@ -0,0 +1,46 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * 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.
+ */
+
+import java.nio.ByteBuffer;
+
+/**
+ * Implementation performing checksum calculation for specific algorithm. Instances of this interface are stateful,
+ * non-thread safe, and should not be reused.
+ *
+ * @since TBD
+ */
+public interface ChecksumAlgorithm
+{
+ /**
+ * Updates the checksum algorithm inner state with input.
+ */
+ void update( ByteBuffer input );
Review comment:
Do we receive data as `ByteBuffer` or rather `byte[]`?
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumAlgorithmFactorySelector.java
##########
@@ -0,0 +1,44 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * 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.
+ */
+
+import java.util.Set;
+
+/**
+ * Component performing selection of {@link ChecksumAlgorithmFactory} based on known factory names.
+ *
+ * @since TBD
+ */
+public interface ChecksumAlgorithmFactorySelector
+{
+ /**
+ * Returns factory for given algorithm name, or throws if algorithm not supported.
+ *
+ * @throws IllegalArgumentException if asked algorithm name is not supported.
+ */
+ ChecksumAlgorithmFactory select( String algorithmName ) throws IllegalArgumentException;
Review comment:
No need for `throws`. It is a runtime exception. No effect.
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/layout/RepositoryLayout.java
##########
@@ -36,45 +37,45 @@
{
/**
- * A descriptor for a checksum file. This descriptor simply associates the location of a checksum file with the
- * underlying algorithm used to calculate/verify it. Checksum algorithms are denoted by names as used with
- * {@link java.security.MessageDigest#getInstance(String)}, e.g. {@code "SHA-1"} or {@code "MD5"}.
+ * A descriptor for a checksum location. This descriptor simply associates the location of a checksum file with the
+ * underlying checksum algorithm used to calculate/verify it.
*/
- final class Checksum
+ final class ChecksumLocation
{
-
- private final String algorithm;
-
private final URI location;
+ private final ChecksumAlgorithmFactory checksumAlgorithmFactory;
+
/**
* Creates a new checksum file descriptor with the specified algorithm and location. The method
- * {@link #forLocation(URI, String)} is usually more convenient though.
- *
- * @param algorithm The algorithm used to calculate the checksum, must not be {@code null}.
- * @param location The relative URI to the checksum file within a repository, must not be {@code null}.
+ * {@link #forLocation(URI, ChecksumAlgorithmFactory)} is usually more convenient though.
+ *
+ * @param location The relative URI to the checksum file within a repository, must not be {@code
+ * null}.
+ * @param checksumAlgorithmFactory The checksum type used to calculate the checksum, must not be {@code null}.
*/
- public Checksum( String algorithm, URI location )
+ public ChecksumLocation( URI location, ChecksumAlgorithmFactory checksumAlgorithmFactory )
{
- verify( algorithm, location );
- this.algorithm = algorithm;
+ verify( location, checksumAlgorithmFactory );
this.location = location;
+ this.checksumAlgorithmFactory = checksumAlgorithmFactory;
}
/**
- * Creates a checksum file descriptor for the specified artifact/metadata location and algorithm. The location
+ * Creates a checksum descriptor for the specified artifact/metadata location and algorithm. The location
* of the checksum file itself is derived from the supplied resource URI by appending the file extension
* corresponding to the algorithm. The file extension in turn is derived from the algorithm name by stripping
* out any hyphen ('-') characters and lower-casing the name, e.g. "SHA-1" is mapped to ".sha1".
Review comment:
There two lines aren't correct anymore since most of it is handled of to the chksum factory.
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/layout/RepositoryLayout.java
##########
@@ -126,55 +122,63 @@ public URI getLocation()
@Override
public String toString()
{
- return location + " (" + algorithm + ")";
+ return location + " (" + checksumAlgorithmFactory.getName() + ")";
}
-
}
+ /**
+ * Returns the collection of checksum names this instance of layout uses, never {@code null}.
+ *
+ * @since TBD
+ */
+ List<String> getChecksumAlgorithmNames();
Review comment:
This one is a list, up earlier it was a set...
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787579778
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumAlgorithmFactorySelector.java
##########
@@ -0,0 +1,44 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * 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.
+ */
+
+import java.util.Set;
+
+/**
+ * Component performing selection of {@link ChecksumAlgorithmFactory} based on known factory names.
+ *
+ * @since TBD
+ */
+public interface ChecksumAlgorithmFactorySelector
+{
+ /**
+ * Returns factory for given algorithm name, or throws if algorithm not supported.
+ *
+ * @throws IllegalArgumentException if asked algorithm name is not supported.
+ */
+ ChecksumAlgorithmFactory select( String algorithmName ) throws IllegalArgumentException;
+
+ /**
+ * Returns a set of supported algorithm names. This set represents ALL the algorithms supported by Resolver, and is
+ * NOT in any relation to given repository layout used checksums, returned by method {@link
+ * org.eclipse.aether.spi.connector.layout.RepositoryLayout#getChecksumAlgorithmNames()} (is super set of it).
+ */
+ Set<String> getChecksumAlgorithmNames();
Review comment:
In this case, this is selector, "all supported algorithms by resolver" this is unordered (set), no order defined or needed here.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported bitrot detection checksums extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r768895058
##########
File path: maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/ChecksumImplementation.java
##########
@@ -0,0 +1,41 @@
+package org.eclipse.aether.connector.basic;
+
+/*
+ * 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.
+ */
+
+import java.nio.ByteBuffer;
+
+/**
+ * Implementation performing checksum calculation for specific algorithm. Instances of this interface as stateful,
+ * should be reused only after {@link #reset()}.
+ *
+ * <strong>Important note:</strong> if implementation of this interface is component, it MUST NOT be singleton, but
+ * must adhere to "prototype" pattern. Alternative is just to implement a provider, and on each get invocation create
+ * new instance.
+ *
+ * @since TBD
+ */
+public interface ChecksumImplementation
+{
+ void update( ByteBuffer input );
+
+ void reset();
+
+ byte[] digest();
Review comment:
fixed
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultChecksumImplementationSelector.java
##########
@@ -0,0 +1,75 @@
+package org.eclipse.aether.internal.impl;
+
+/*
+ * 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.
+ */
+
+import java.security.NoSuchAlgorithmException;
+import java.util.Collections;
+import java.util.Map;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Provider;
+import javax.inject.Singleton;
+
+import org.eclipse.aether.spi.connector.checksum.ChecksumImplementation;
+import org.eclipse.aether.spi.connector.checksum.ChecksumImplementationSelectorSupport;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link org.eclipse.aether.spi.connector.checksum.ChecksumImplementationSelector} that
+ * is extensible.
+ *
+ * @since TBD
+ */
+@Singleton
+@Named
+public final class DefaultChecksumImplementationSelector
+ extends ChecksumImplementationSelectorSupport
+{
+ private final Map<String, Provider<ChecksumImplementation>> providers;
+
+ /**
+ * Ctor for ServiceLocator.
+ */
+ public DefaultChecksumImplementationSelector()
Review comment:
fixed
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumImplementation.java
##########
@@ -0,0 +1,41 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * 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.
+ */
+
+import java.nio.ByteBuffer;
+
+/**
+ * Implementation performing checksum calculation for specific algorithm. Instances of this interface as stateful,
+ * should be reused only after {@link #reset()}.
+ *
+ * <strong>Important note:</strong> if implementation of this interface is component, it MUST NOT be singleton, but
+ * must adhere to "prototype" pattern. Alternative is just to implement a provider, and on each get invocation create
+ * new instance.
+ *
+ * @since TBD
+ */
+public interface ChecksumImplementation
+{
+ void update( ByteBuffer input );
+
+ void reset();
+
+ byte[] digest();
Review comment:
fixed
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787586267
##########
File path: maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/BasicRepositoryConnector.java
##########
@@ -571,25 +587,26 @@ private void uploadChecksums( File file, byte[] bytes )
}
try
{
- Set<String> algos = new HashSet<>();
- for ( RepositoryLayout.Checksum checksum : checksums )
+ ArrayList<ChecksumAlgorithmFactory> algorithms = new ArrayList<>();
+ for ( RepositoryLayout.ChecksumLocation checksumLocation : checksums )
{
- algos.add( checksum.getAlgorithm() );
+ algorithms.add( checksumLocation.getChecksumAlgorithmFactory() );
}
- Map<String, Object> sumsByAlgo;
+ Map<String, String> sumsByAlgo;
Review comment:
The `sumsByAlgo` was always populated from (now deprecated) `ChecksumUtils.calc` method, that in turn returned non-string in map value ONLY in case of `NoSuchAlgorithmException`. This PR OTOH makes sure that checksums "fail fast", so you cannot even start (get factory from selector) for algorithm that does not exists. So to say, old code was more forgiving if you asked for algorithm not supported by MessageDigester (all this as actual "name evaluation" was happening way before, and it just accepted any string), while in this PR you must pass in a factory, that you can obtain ONLY for existing algorithms.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#issuecomment-1016289307
> Why not using `SortedSet` then instead of `List` for the layout?
Well, in case of layout it is not "sorted" per se, but "in order as user defined", no?
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] kwin commented on pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
kwin commented on pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#issuecomment-1016291896
SortedSet:
> "A Set that further provides a total ordering on its elements."
from https://docs.oracle.com/javase/8/docs/api/java/util/SortedSet.html
This is exactly what is needed here IIUC.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r792996067
##########
File path: src/site/markdown/about-checksums.md
##########
@@ -0,0 +1,72 @@
+# About checksums
Review comment:
About Checksums
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/impl/guice/AetherModule.java
##########
@@ -42,6 +42,11 @@
import org.eclipse.aether.impl.RepositoryEventDispatcher;
import org.eclipse.aether.internal.impl.DefaultTrackingFileManager;
import org.eclipse.aether.internal.impl.TrackingFileManager;
+import org.eclipse.aether.internal.impl.checksum.ChecksumAlgorithmFactoryMD5;
Review comment:
I think the also name should be at the of the class name. Just like `FileInputStream` and not `InputStreamFile`.
##########
File path: src/site/site.xml
##########
@@ -27,6 +27,7 @@ under the License.
<menu name="Overview">
<item name="Introduction" href="index.html"/>
<item name="Configuration" href="configuration.html"/>
+ <item name="About checksums" href="about-checksums.html"/>
Review comment:
About Checksums
##########
File path: maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/ChecksumValidator.java
##########
@@ -241,12 +252,12 @@ public void commit()
{
if ( tmp instanceof File )
{
- fileProcessor.move( (File) tmp, checksumFile );
+ fileProcessor.move( ( File ) tmp, checksumFile );
Review comment:
As far as I know we don't have spaces around casts.
##########
File path: src/site/markdown/about-checksums.md
##########
@@ -0,0 +1,72 @@
+# About checksums
+<!--
+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.
+-->
+
+Maven Resolver uses checksums to verify the consistency of the downloaded
+artifacts. Checksums are usually laid out in repositories next to the file in question, with file
+extension telling the checksum algorithm that produced the given checksum file content. Current Maven Repository
+layout contains SHA-1 and MD5 checksums by default (they are produced by Resolver by default).
+
+Historically, Maven Resolver used `java.security.MessageDigest` to implement checksums. So to say, secure one-way
+hashes provided by Java Cryptography Architecture were (mis)used to implement checksums for transport integrity
+validation. There is no misunderstanding here, secure hashes MAY be used as checksums, as there is quite some
+overlap between "checksums" and "hashes" in general. But this simplicity comes at a price: cryptographically "safe"
+algorithms require way more CPU cycles to calculate checksum, while all their purpose is just
+"integrity validation", nothing more. There is no "security", "trust" or whatever else implied or expected from
+them.
+
+If you are interested in "trust" in your artifacts, it is Artifact Signatures (for example
+[GPG Signatures](https://maven.apache.org/plugins/maven-gpg-plugin/)) that you should look for.
+
+Hence, the usual argument that "XXX algorithm is unsafe, deprecated, not secure anymore" does not stand in use case
+of Maven Resolver: there is nothing "secure" being involved with checksums. Moreover, this is true not only for SHA-1
+algorithm, but even for its "elder brother" MD5. Both algorithms are still widely used today as "transport integrity
+validation" or "error detection" (aka "bit-rot detection").
+
+## Checksum changes
Review comment:
Checksum Changes
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/Maven2RepositoryLayoutFactory.java
##########
@@ -163,45 +204,47 @@ public URI getLocation( Metadata metadata, boolean upload )
return toUri( path.toString() );
}
- public List<Checksum> getChecksums( Artifact artifact, boolean upload, URI location )
+ @Override
+ public List<ChecksumLocation> getChecksumLocations( Artifact artifact, boolean upload, URI location )
{
return getChecksums( location );
}
- public List<Checksum> getChecksums( Metadata metadata, boolean upload, URI location )
+ @Override
+ public List<ChecksumLocation> getChecksumLocations( Metadata metadata, boolean upload, URI location )
{
return getChecksums( location );
}
- private List<Checksum> getChecksums( URI location )
+ private List<ChecksumLocation> getChecksums( URI location )
Review comment:
`getChecksumLocations`?
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] asfgit closed pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787557242
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/named/DiscriminatingNameMapper.java
##########
@@ -115,25 +113,22 @@ private String createDiscriminator( final RepositorySystemSession session )
String hostname = ConfigUtils.getString( session, this.hostname, CONFIG_PROP_HOSTNAME );
File basedir = session.getLocalRepository().getBasedir();
discriminator = hostname + ":" + basedir;
- try
- {
- Map<String, Object> checksums = ChecksumUtils
- .calc( discriminator.getBytes( StandardCharsets.UTF_8 ), Collections.singletonList( "SHA-1" ) );
- Object checksum = checksums.get( "SHA-1" );
-
- if ( checksum instanceof Exception )
- {
- throw (Exception) checksum;
- }
-
- return String.valueOf( checksum );
- }
- catch ( Exception e )
- {
- LOGGER.warn( "Failed to calculate discriminator digest, using '{}'", DEFAULT_DISCRIMINATOR_DIGEST, e );
- return DEFAULT_DISCRIMINATOR_DIGEST;
- }
+ return sha1String( discriminator );
Review comment:
Reset code, left as is, but the as-is code in turn now uses deprecated `ChecksumUtils.calc` method...
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787570194
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumAlgorithmHelper.java
##########
@@ -0,0 +1,106 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * 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.
+ */
+
+import java.io.BufferedInputStream;
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.Buffer;
+import java.nio.ByteBuffer;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Helper for checksum operations.
+ *
+ * @since TBD
+ */
+public final class ChecksumAlgorithmHelper
+{
+ private ChecksumAlgorithmHelper()
+ {
+ // nop
+ }
+
+ /**
+ * Calculates checksums for specified data.
+ *
+ * @param data The content for which to calculate checksums, must not be {@code null}.
+ * @param factories The checksum algorithm factories to use, must not be {@code null}.
+ * @return The calculated checksums, indexed by algorithm name, or the exception that occurred while trying to
+ * calculate it, never {@code null}.
+ * @throws IOException In case of any problem.
+ */
+ public static Map<String, String> calculate( byte[] data, List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ return calculate( new ByteArrayInputStream( data ), factories );
+ }
+
+ /**
+ * Calculates checksums for specified file.
+ *
+ * @param file The file for which to calculate checksums, must not be {@code null}.
+ * @param factories The checksum algorithm factories to use, must not be {@code null}.
+ * @return The calculated checksums, indexed by algorithm name, or the exception that occurred while trying to
+ * calculate it, never {@code null}.
+ * @throws IOException In case of any problem.
+ */
+ public static Map<String, String> calculate( File file, List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ return calculate( new BufferedInputStream( new FileInputStream( file ) ), factories );
Review comment:
Fixed
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787578984
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/layout/RepositoryLayout.java
##########
@@ -126,55 +122,63 @@ public URI getLocation()
@Override
public String toString()
{
- return location + " (" + algorithm + ")";
+ return location + " (" + checksumAlgorithmFactory.getName() + ")";
}
-
}
+ /**
+ * Returns the collection of checksum names this instance of layout uses, never {@code null}.
+ *
+ * @since TBD
+ */
+ List<String> getChecksumAlgorithmNames();
Review comment:
This is layout property, order is important (but the returned list is also unique).
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas edited a comment on pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas edited a comment on pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#issuecomment-1016326439
@michael-o actually no, changed my mind re `ByteBuffer`: it is so proliferated even in public API (see transfer listener etc), that I think it is okay (even w/ some overhead) to keep same thing internally as well. That said, I did simplify things where ByteBuffer was kinda "overused" or whatever.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787549585
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/layout/RepositoryLayout.java
##########
@@ -36,45 +37,45 @@
{
/**
- * A descriptor for a checksum file. This descriptor simply associates the location of a checksum file with the
- * underlying algorithm used to calculate/verify it. Checksum algorithms are denoted by names as used with
- * {@link java.security.MessageDigest#getInstance(String)}, e.g. {@code "SHA-1"} or {@code "MD5"}.
+ * A descriptor for a checksum location. This descriptor simply associates the location of a checksum file with the
+ * underlying checksum algorithm used to calculate/verify it.
*/
- final class Checksum
+ final class ChecksumLocation
{
-
- private final String algorithm;
-
private final URI location;
+ private final ChecksumAlgorithmFactory checksumAlgorithmFactory;
+
/**
* Creates a new checksum file descriptor with the specified algorithm and location. The method
- * {@link #forLocation(URI, String)} is usually more convenient though.
- *
- * @param algorithm The algorithm used to calculate the checksum, must not be {@code null}.
- * @param location The relative URI to the checksum file within a repository, must not be {@code null}.
+ * {@link #forLocation(URI, ChecksumAlgorithmFactory)} is usually more convenient though.
+ *
+ * @param location The relative URI to the checksum file within a repository, must not be {@code
+ * null}.
+ * @param checksumAlgorithmFactory The checksum type used to calculate the checksum, must not be {@code null}.
*/
- public Checksum( String algorithm, URI location )
+ public ChecksumLocation( URI location, ChecksumAlgorithmFactory checksumAlgorithmFactory )
{
- verify( algorithm, location );
- this.algorithm = algorithm;
+ verify( location, checksumAlgorithmFactory );
this.location = location;
+ this.checksumAlgorithmFactory = checksumAlgorithmFactory;
}
/**
- * Creates a checksum file descriptor for the specified artifact/metadata location and algorithm. The location
+ * Creates a checksum descriptor for the specified artifact/metadata location and algorithm. The location
* of the checksum file itself is derived from the supplied resource URI by appending the file extension
* corresponding to the algorithm. The file extension in turn is derived from the algorithm name by stripping
* out any hyphen ('-') characters and lower-casing the name, e.g. "SHA-1" is mapped to ".sha1".
Review comment:
Fixed
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787547036
##########
File path: src/site/markdown/about-checksums.md
##########
@@ -0,0 +1,72 @@
+# About checksums
+<!--
+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.
+-->
+
+Maven Resolver uses "checksums" to verify the consistency of the downloaded
Review comment:
fixed
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported bitrot detection checksums extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r779181566
##########
File path: maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/ChecksumCalculator.java
##########
@@ -125,7 +131,14 @@ private ChecksumCalculator( File targetFile, Collection<RepositoryLayout.Checksu
String algo = checksum.getAlgorithm();
if ( algos.add( algo ) )
{
- this.checksums.add( new Checksum( algo ) );
+ try
+ {
+ this.checksums.add( new Checksum( algo, selector.select( algo ) ) );
+ }
+ catch ( NoSuchAlgorithmException e )
+ {
+ this.checksums.add( new Checksum( algo, e ) );
Review comment:
fixed
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#issuecomment-1011013483
> > Can you adjust https://github.com/apache/maven-resolver/blob/master/src/site/markdown/configuration.md on how to configure this (probably requires adjustment of description for `aether.checksums.algorithms`)?
>
> @kwin once this merged nothing changes re that doco, as "out of the box" resolver will still fallback to `MessageDigest`. But, at the same time this opens a door to extend checksum support, and probably resolver itself will add some new ones (but not in 1.x line, probably 2.x line).
Doco updated, as after last changes this above ("nothing changes") is untrue: MessageDigest is not "exposed" as before.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787569872
##########
File path: maven-resolver-connector-basic/src/test/java/org/eclipse/aether/connector/basic/ChecksumCalculatorTest.java
##########
@@ -39,22 +42,16 @@
public class ChecksumCalculatorTest
{
- private static final String SHA512 = "SHA-512";
-
- private static final String SHA256 = "SHA-256";
-
- private static final String SHA1 = "SHA-1";
-
- private static final String MD5 = "MD5";
-
private File file;
+ private final TestChecksumAlgorithmSelector selector = new TestChecksumAlgorithmSelector();
+
private ChecksumCalculator newCalculator( String... algos )
{
- List<RepositoryLayout.Checksum> checksums = new ArrayList<>();
+ List<RepositoryLayout.ChecksumLocation> checksums = new ArrayList<>();
Review comment:
Fixed
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787569310
##########
File path: maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/ChecksumValidator.java
##########
@@ -63,21 +62,23 @@ boolean fetchChecksum( URI remote, File local )
private final ChecksumPolicy checksumPolicy;
- private final Collection<Checksum> checksums;
+ private final Collection<ChecksumLocation> checksums;
Review comment:
Fixed
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787580506
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/Maven2RepositoryLayoutFactory.java
##########
@@ -83,23 +106,33 @@ public RepositoryLayout newInstance( RepositorySystemSession session, RemoteRepo
throw new NoRepositoryLayoutException( repository );
}
boolean forSignature = ConfigUtils.getBoolean( session, false, CONFIG_PROP_SIGNATURE_CHECKSUMS );
- List<String> checksumsAlgorithms = Arrays.asList( ConfigUtils.getString( session,
- DEFAULT_CHECKSUMS_ALGORITHMS, CONFIG_PROP_CHECKSUMS_ALGORITHMS ).split( "," ) );
+ // ensure order of (potentially user set) algorithm list is kept and is unique
+ LinkedHashSet<String> checksumsAlgorithmNames = new LinkedHashSet<>( Arrays.asList(
Review comment:
User provides a "comma separated list", hence it is a LIST, and in case of layout, ORDER DOES MATTER (as that is the order resolver will ask for checksum). Here we also ensure that list is _unique_.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787552537
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumAlgorithmFactory.java
##########
@@ -0,0 +1,48 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * 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.
+ */
+
+/**
+ * A component representing a checksum factory: provides {@link ChecksumAlgorithm} instances, name and extension to be
+ * used with this algorithm. While directly injecting components of this type is possible, it is not recommended. To
+ * obtain factory instances use {@link ChecksumAlgorithmFactorySelector} instead.
+ *
+ * @since TBD
+ */
+public interface ChecksumAlgorithmFactory
+{
+ /**
+ * Returns the algorithm name, usually used as key, never {@code null} value. The name is a standard name of
+ * algorithm (if applicable) or any other designator that is algorithm commonly referred with. Example: "SHA-1".
+ */
+ String getName();
+
+ /**
+ * Returns the file extension to be used for given checksum file (without leading dot), never {@code null}. The
+ * extension should be file and URL path friendly, and usually differs from value returned by {@link #getName()}.
+ * Example: "sha1".
+ */
+ String getExtension();
Review comment:
Made it `getFileExtension()`
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r787550179
##########
File path: maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumAlgorithmFactorySelector.java
##########
@@ -0,0 +1,44 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * 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.
+ */
+
+import java.util.Set;
+
+/**
+ * Component performing selection of {@link ChecksumAlgorithmFactory} based on known factory names.
+ *
+ * @since TBD
+ */
+public interface ChecksumAlgorithmFactorySelector
+{
+ /**
+ * Returns factory for given algorithm name, or throws if algorithm not supported.
+ *
+ * @throws IllegalArgumentException if asked algorithm name is not supported.
+ */
+ ChecksumAlgorithmFactory select( String algorithmName ) throws IllegalArgumentException;
Review comment:
Fixed
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #139: [MRESOLVER-230] Make supported checksum algorithms extensible
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r793535691
##########
File path: maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/ChecksumValidator.java
##########
@@ -241,12 +252,12 @@ public void commit()
{
if ( tmp instanceof File )
{
- fileProcessor.move( (File) tmp, checksumFile );
+ fileProcessor.move( ( File ) tmp, checksumFile );
Review comment:
Seems IDEA setup published on Maven site does have? Will remove, but also need to check (as this is now formatter on my oss-laptop)
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org