gnodet commented on code in PR #1837: URL: https://github.com/apache/maven-resolver/pull/1837#discussion_r3094298902
########## maven-resolver-util/src/main/java/org/eclipse/aether/util/version/GenericQualifiers.java: ########## @@ -0,0 +1,117 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.eclipse.aether.util.version; + +import java.util.HashMap; +import java.util.Locale; +import java.util.Map; +import java.util.Optional; + +/** + * The recognized generic version qualifiers. Qualifiers may apply a "shift" to versions (ie when sorting), if present. + * + * @since 2.0.17 + */ +public final class GenericQualifiers { + private GenericQualifiers() {} + + public static final String LABEL_ALPHA = "alpha"; + + public static final String LABEL_BETA = "beta"; + + public static final String LABEL_MILESTONE = "milestone"; + + public static final Integer QUALIFIER_ALPHA = -5; + + public static final Integer QUALIFIER_BETA = -4; + + public static final Integer QUALIFIER_MILESTONE = -3; + + public static final Integer QUALIFIER_RC = -2; + + public static final Integer QUALIFIER_SNAPSHOT = -1; + + public static final Integer QUALIFIER_ZERO = 0; + + public static final Integer QUALIFIER_SP = 1; + + private static final Map<String, Integer> QUALIFIERS; + + static { + QUALIFIERS = new HashMap<>(); + QUALIFIERS.put(LABEL_ALPHA, QUALIFIER_ALPHA); + QUALIFIERS.put(LABEL_BETA, QUALIFIER_BETA); + QUALIFIERS.put(LABEL_MILESTONE, QUALIFIER_MILESTONE); + QUALIFIERS.put("rc", QUALIFIER_RC); + QUALIFIERS.put("cr", QUALIFIER_RC); + QUALIFIERS.put("snapshot", QUALIFIER_SNAPSHOT); + QUALIFIERS.put("ga", QUALIFIER_ZERO); + QUALIFIERS.put("final", QUALIFIER_ZERO); + QUALIFIERS.put("release", QUALIFIER_ZERO); + QUALIFIERS.put("", QUALIFIER_ZERO); // TODO: is this entry valid? + QUALIFIERS.put("sp", QUALIFIER_SP); Review Comment: nit: This TODO was carried over from the original internal code. Now that this class is part of the public API, it might be worth either resolving it (documenting why the empty-string entry exists) or removing it if it's no longer relevant. ########## maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/versions/GenericQualifiersVersionFilterTest.java: ########## @@ -0,0 +1,70 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.eclipse.aether.util.graph.versions; + +import org.eclipse.aether.collection.VersionFilter.VersionFilterContext; +import org.eclipse.aether.util.graph.version.GenericQualifiersVersionFilter; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertSame; + +public class GenericQualifiersVersionFilterTest extends AbstractVersionFilterTest { + + @Test + void testPreviewVersionFilter() { + GenericQualifiersVersionFilter filter = GenericQualifiersVersionFilter.previewVersionFilter(); + VersionFilterContext ctx = newContext( + "g:a:[1,9]", + "1-M1", + "1", + "2-SNAPSHOT", + "3.0-GA", + "3.1", + "4.0-SNAPSHOT", + "5.0.0.rc", + "5.0.0", + "5.0.0-sp"); + filter.filterVersions(ctx); + assertVersions(ctx, "1", "2-SNAPSHOT", "3.0-GA", "3.1", "4.0-SNAPSHOT", "5.0.0", "5.0.0-sp"); + } + + @Test + void testAnyQualifierVersionFilter() { + GenericQualifiersVersionFilter filter = GenericQualifiersVersionFilter.anyQualifierVersionFilter(); + VersionFilterContext ctx = newContext( + "g:a:[1,9]", + "1-M1", + "1", + "2-SNAPSHOT", + "3.0-GA", + "3.1", + "4.0-SNAPSHOT", + "5.0.0.rc", + "5.0.0", + "5.0.0-sp"); + filter.filterVersions(ctx); + assertVersions(ctx, "1", "3.1", "5.0.0"); + } + + @Test + void testDeriveChildFilter() { + GenericQualifiersVersionFilter filter = GenericQualifiersVersionFilter.previewVersionFilter(); + assertSame(filter, derive(filter, "g:a:1")); + } +} Review Comment: The filter-level tests are good and cover the main scenarios. It might be valuable to add direct unit tests for `GenericQualifiers.qualifier(String)` as well — particularly for edge cases like single-character versions, the short-form detection (`"a1"`, `"b2"`, `"m3"`), and version strings that could trigger the substring false positives mentioned in the other comment. ########## maven-resolver-util/src/main/java/org/eclipse/aether/util/version/GenericQualifiers.java: ########## @@ -0,0 +1,117 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.eclipse.aether.util.version; + +import java.util.HashMap; +import java.util.Locale; +import java.util.Map; +import java.util.Optional; + +/** + * The recognized generic version qualifiers. Qualifiers may apply a "shift" to versions (ie when sorting), if present. + * + * @since 2.0.17 + */ +public final class GenericQualifiers { + private GenericQualifiers() {} + + public static final String LABEL_ALPHA = "alpha"; + + public static final String LABEL_BETA = "beta"; + + public static final String LABEL_MILESTONE = "milestone"; + + public static final Integer QUALIFIER_ALPHA = -5; + + public static final Integer QUALIFIER_BETA = -4; + + public static final Integer QUALIFIER_MILESTONE = -3; + + public static final Integer QUALIFIER_RC = -2; + + public static final Integer QUALIFIER_SNAPSHOT = -1; + + public static final Integer QUALIFIER_ZERO = 0; + + public static final Integer QUALIFIER_SP = 1; + + private static final Map<String, Integer> QUALIFIERS; + + static { + QUALIFIERS = new HashMap<>(); + QUALIFIERS.put(LABEL_ALPHA, QUALIFIER_ALPHA); + QUALIFIERS.put(LABEL_BETA, QUALIFIER_BETA); + QUALIFIERS.put(LABEL_MILESTONE, QUALIFIER_MILESTONE); + QUALIFIERS.put("rc", QUALIFIER_RC); + QUALIFIERS.put("cr", QUALIFIER_RC); + QUALIFIERS.put("snapshot", QUALIFIER_SNAPSHOT); + QUALIFIERS.put("ga", QUALIFIER_ZERO); + QUALIFIERS.put("final", QUALIFIER_ZERO); + QUALIFIERS.put("release", QUALIFIER_ZERO); + QUALIFIERS.put("", QUALIFIER_ZERO); // TODO: is this entry valid? + QUALIFIERS.put("sp", QUALIFIER_SP); + } + + /** + * Returns qualifier (an {@link Integer} for given token, if detected. This method is used in {@link GenericVersion} + * that tokenizes version, and uses string token in call. The input must have {@link String#toLowerCase(Locale)} + * applied. + */ + static Optional<Integer> tokenQualifier(String token) { + return Optional.ofNullable(QUALIFIERS.get(token)); + } + + /** + * Returns qualifier (an {@link Integer} for given string, if detected. Qualifier, if present, defines "shift", + * if negative, it is a "preview version" (e.g. alpha, beta, milestone, release candidate or snapshot), if zero, + * it is a "final version" (e.g. ga, final, release), if positive, it is a "service pack" version (e.g. sp). + * If no qualifier is detected, an empty optional is returned. + * + * @param token the string to analyze for qualifier, must not be {@code null} + */ + public static Optional<Integer> qualifier(String token) { + // most trivial preview version is "a1" + if (token.length() > 1) { + String v = token.toLowerCase(Locale.ENGLISH); + // simple case: full qualifier label is present (assuming once) + for (Map.Entry<String, Integer> entry : QUALIFIERS.entrySet()) { + // TODO on map we have empty string "" as key + if (!entry.getKey().isEmpty() && v.contains(entry.getKey())) { Review Comment: The `v.contains(entry.getKey())` substring check can produce false positives for short qualifier labels. For example: - `"1.0-arced"` → matches `"rc"` → incorrectly detected as RC - `"1.0-legacy"` → matches `"ga"` → incorrectly detected as GA - `"1.0-sacred"` → matches `"cr"` → incorrectly detected as RC - `"1.0-dispatcher"` → matches `"sp"` → incorrectly detected as SP In practice Maven versions are fairly structured so the false positive rate may be low, but since this is a new public API (`@since 2.0.17`), it might be worth considering a tokenization-based or word-boundary approach (e.g., splitting on `.`, `-`, `_` first and then doing exact matches on each token — similar to what `tokenQualifier` already does internally for the parsed path). What do you think? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
