From 1b7d59f171d0e2a3bdd234cddffac548b1f8ba57 Mon Sep 17 00:00:00 2001 From: Alexey Semenyuk Date: Fri, 31 May 2024 03:05:09 +0000 Subject: [PATCH] 8333303: Issues with DottedVersion class Reviewed-by: almatvee --- .../jdk/jpackage/internal/DottedVersion.java | 226 +++++++++++------- .../jdk/jpackage/internal/ToolValidator.java | 6 +- .../internal/WixAppImageFragmentBuilder.java | 6 +- .../jpackage/internal/WixFragmentBuilder.java | 4 +- .../internal/CompareDottedVersionTest.java | 6 +- .../jpackage/internal/DottedVersionTest.java | 139 ++++++++--- 6 files changed, 262 insertions(+), 125 deletions(-) diff --git a/src/jdk.jpackage/share/classes/jdk/jpackage/internal/DottedVersion.java b/src/jdk.jpackage/share/classes/jdk/jpackage/internal/DottedVersion.java index 61b285f4756da..91c0a4915fd43 100644 --- a/src/jdk.jpackage/share/classes/jdk/jpackage/internal/DottedVersion.java +++ b/src/jdk.jpackage/share/classes/jdk/jpackage/internal/DottedVersion.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2019, 2021, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2019, 2024, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -22,33 +22,122 @@ * or visit www.oracle.com if you need additional information or have any * questions. */ - package jdk.jpackage.internal; import java.math.BigInteger; import java.text.MessageFormat; -import java.util.ArrayList; -import java.util.List; +import java.util.Arrays; import java.util.Objects; -import java.util.regex.Pattern; import java.util.stream.Collectors; +import java.util.stream.Stream; /** - * Dotted numeric version string. - * E.g.: 1.0.37, 10, 0.5 + * Dotted numeric version string. E.g.: 1.0.37, 10, 0.5 */ -final class DottedVersion implements Comparable { +final class DottedVersion { DottedVersion(String version) { - greedy = true; - components = parseVersionString(version, greedy); - value = version; + this(version, true); } private DottedVersion(String version, boolean greedy) { - this.greedy = greedy; - components = parseVersionString(version, greedy); - value = version; + this.value = version; + if (version.isEmpty()) { + if (greedy) { + throw new IllegalArgumentException(I18N.getString("error.version-string-empty")); + } else { + this.components = new BigInteger[0]; + this.suffix = ""; + } + } else { + var ds = new DigitsSupplier(version); + components = Stream.generate(ds::getNextDigits).takeWhile(Objects::nonNull).map( + digits -> { + if (digits.isEmpty()) { + if (!greedy) { + return null; + } else { + throw new IllegalArgumentException(MessageFormat.format(I18N. + getString("error.version-string-zero-length-component"), + version)); + } + } + + try { + return new BigInteger(digits); + } catch (NumberFormatException ex) { + if (!greedy) { + return null; + } else { + throw new IllegalArgumentException(MessageFormat.format(I18N. + getString("error.version-string-invalid-component"), version, + digits)); + } + } + }).takeWhile(Objects::nonNull).toArray(BigInteger[]::new); + suffix = ds.getUnprocessedString(); + if (!suffix.isEmpty() && greedy) { + throw new IllegalArgumentException(MessageFormat.format(I18N.getString( + "error.version-string-invalid-component"), version, suffix)); + } + } + } + + private static class DigitsSupplier { + + DigitsSupplier(String input) { + this.input = input; + } + + public String getNextDigits() { + if (stoped) { + return null; + } + + var sb = new StringBuilder(); + while (cursor != input.length()) { + var chr = input.charAt(cursor++); + if (Character.isDigit(chr)) { + sb.append(chr); + } else { + var curStopAtDot = (chr == '.'); + if (!curStopAtDot) { + if (lastDotPos >= 0) { + cursor = lastDotPos; + } else { + cursor--; + } + stoped = true; + } else if (!sb.isEmpty()) { + lastDotPos = cursor - 1; + } else { + cursor = Math.max(lastDotPos, 0); + stoped = true; + } + return sb.toString(); + } + } + + if (sb.isEmpty()) { + if (lastDotPos >= 0) { + cursor = lastDotPos; + } else { + cursor--; + } + } + + stoped = true; + return sb.toString(); + } + + public String getUnprocessedString() { + return input.substring(cursor); + } + + private int cursor; + private int lastDotPos = -1; + private boolean stoped; + private final String input; } static DottedVersion greedy(String version) { @@ -59,22 +148,22 @@ static DottedVersion lazy(String version) { return new DottedVersion(version, false); } - @Override - public int compareTo(String o) { + static int compareComponents(DottedVersion a, DottedVersion b) { int result = 0; - BigInteger[] otherComponents = parseVersionString(o, greedy); - for (int i = 0; i < Math.max(components.length, otherComponents.length) + BigInteger[] aComponents = a.getComponents(); + BigInteger[] bComponents = b.getComponents(); + for (int i = 0; i < Math.max(aComponents.length, bComponents.length) && result == 0; ++i) { final BigInteger x; - if (i < components.length) { - x = components[i]; + if (i < aComponents.length) { + x = aComponents[i]; } else { x = BigInteger.ZERO; } final BigInteger y; - if (i < otherComponents.length) { - y = otherComponents[i]; + if (i < bComponents.length) { + y = bComponents[i]; } else { y = BigInteger.ZERO; } @@ -84,71 +173,30 @@ public int compareTo(String o) { return result; } - private static BigInteger[] parseVersionString(String version, boolean greedy) { - Objects.requireNonNull(version); - if (version.isEmpty()) { - if (!greedy) { - return new BigInteger[] {BigInteger.ZERO}; - } - throw new IllegalArgumentException(I18N.getString( - "error.version-string-empty")); - } - - int lastNotZeroIdx = -1; - List components = new ArrayList<>(); - for (var component : version.split("\\.", -1)) { - if (component.isEmpty()) { - if (!greedy) { - break; - } - - throw new IllegalArgumentException(MessageFormat.format( - I18N.getString( - "error.version-string-zero-length-component"), - version)); - } - - if (!DIGITS.matcher(component).matches()) { - // Catch "+N" and "-N" cases. - if (!greedy) { - break; - } - - throw new IllegalArgumentException(MessageFormat.format( - I18N.getString( - "error.version-string-invalid-component"), - version, component)); - } - - final BigInteger num; - try { - num = new BigInteger(component); - } catch (NumberFormatException ex) { - if (!greedy) { - break; - } - - throw new IllegalArgumentException(MessageFormat.format( - I18N.getString( - "error.version-string-invalid-component"), - version, component)); - } + @Override + public int hashCode() { + int hash = 3; + hash = 29 * hash + Arrays.deepHashCode(this.components); + hash = 29 * hash + Objects.hashCode(this.suffix); + return hash; + } - if (num != BigInteger.ZERO) { - lastNotZeroIdx = components.size(); - } - components.add(num); + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; } - - if (lastNotZeroIdx + 1 != components.size()) { - // Strip trailing zeros. - components = components.subList(0, lastNotZeroIdx + 1); + if (obj == null) { + return false; } - - if (components.isEmpty()) { - components.add(BigInteger.ZERO); + if (getClass() != obj.getClass()) { + return false; } - return components.toArray(BigInteger[]::new); + final DottedVersion other = (DottedVersion) obj; + if (!Objects.equals(this.suffix, other.suffix)) { + return false; + } + return Arrays.deepEquals(this.components, other.components); } @Override @@ -156,13 +204,19 @@ public String toString() { return value; } + public String getUnprocessedSuffix() { + return suffix; + } + + String toComponentsString() { + return Stream.of(components).map(BigInteger::toString).collect(Collectors.joining(".")); + } + BigInteger[] getComponents() { return components; } private final BigInteger[] components; private final String value; - private final boolean greedy; - - private static final Pattern DIGITS = Pattern.compile("\\d+"); + private final String suffix; } diff --git a/src/jdk.jpackage/share/classes/jdk/jpackage/internal/ToolValidator.java b/src/jdk.jpackage/share/classes/jdk/jpackage/internal/ToolValidator.java index 793ff623c529f..64876468afbab 100644 --- a/src/jdk.jpackage/share/classes/jdk/jpackage/internal/ToolValidator.java +++ b/src/jdk.jpackage/share/classes/jdk/jpackage/internal/ToolValidator.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2019, 2024, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -65,6 +65,10 @@ ToolValidator setMinimalVersion(Comparable v) { return this; } + ToolValidator setMinimalVersion(DottedVersion v) { + return setMinimalVersion(t -> DottedVersion.compareComponents(v, DottedVersion.lazy(t))); + } + ToolValidator setVersionParser(Function, String> v) { versionParser = v; return this; diff --git a/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WixAppImageFragmentBuilder.java b/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WixAppImageFragmentBuilder.java index 7578712ab4d5b..cf7338f7d0b48 100644 --- a/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WixAppImageFragmentBuilder.java +++ b/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WixAppImageFragmentBuilder.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2019, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2019, 2024, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -785,7 +785,7 @@ private void addRegistryKeyPath(XMLStreamWriter xml, Path path, xml.writeStartElement("RegistryKey"); xml.writeAttribute("Root", regRoot); xml.writeAttribute("Key", registryKeyPath); - if (getWixVersion().compareTo("3.6") < 0) { + if (DottedVersion.compareComponents(getWixVersion(), DottedVersion.lazy("3.6")) < 0) { xml.writeAttribute("Action", "createAndRemoveOnUninstall"); } xml.writeStartElement("RegistryValue"); @@ -799,7 +799,7 @@ private void addRegistryKeyPath(XMLStreamWriter xml, Path path, private String addDirectoryCleaner(XMLStreamWriter xml, Path path) throws XMLStreamException, IOException { - if (getWixVersion().compareTo("3.6") < 0) { + if (DottedVersion.compareComponents(getWixVersion(), DottedVersion.lazy("3.6")) < 0) { return null; } diff --git a/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WixFragmentBuilder.java b/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WixFragmentBuilder.java index 5671701e7a9fd..bc98899c65985 100644 --- a/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WixFragmentBuilder.java +++ b/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WixFragmentBuilder.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2021, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2021, 2024, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -66,7 +66,7 @@ void initFromParams(Map params) { } void logWixFeatures() { - if (wixVersion.compareTo("3.6") >= 0) { + if (DottedVersion.compareComponents(wixVersion, DottedVersion.lazy("3.6")) >= 0) { Log.verbose(MessageFormat.format(I18N.getString( "message.use-wix36-features"), wixVersion)); } diff --git a/test/jdk/tools/jpackage/junit/jdk.jpackage/jdk/jpackage/internal/CompareDottedVersionTest.java b/test/jdk/tools/jpackage/junit/jdk.jpackage/jdk/jpackage/internal/CompareDottedVersionTest.java index aef0092748a58..d77b4e85eb718 100644 --- a/test/jdk/tools/jpackage/junit/jdk.jpackage/jdk/jpackage/internal/CompareDottedVersionTest.java +++ b/test/jdk/tools/jpackage/junit/jdk.jpackage/jdk/jpackage/internal/CompareDottedVersionTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2019, 2024, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -68,6 +68,8 @@ public static List data() { data.addAll(List.of(new Object[][] { { false, "", "1", -1 }, + { false, "", "0", 0 }, + { false, "0", "", 0 }, { false, "1.2.4-R4", "1.2.4-R5", 0 }, { false, "1.2.4.-R4", "1.2.4.R5", 0 }, { false, "7+1", "7+4", 0 }, @@ -89,7 +91,7 @@ public void testIt() { } private int compare(String x, String y) { - int result = createTestee.apply(x).compareTo(y); + int result = DottedVersion.compareComponents(createTestee.apply(x), createTestee.apply(y)); if (result < 0) { return -1; diff --git a/test/jdk/tools/jpackage/junit/jdk.jpackage/jdk/jpackage/internal/DottedVersionTest.java b/test/jdk/tools/jpackage/junit/jdk.jpackage/jdk/jpackage/internal/DottedVersionTest.java index d924611b4ae4e..0443fdf64e7ea 100644 --- a/test/jdk/tools/jpackage/junit/jdk.jpackage/jdk/jpackage/internal/DottedVersionTest.java +++ b/test/jdk/tools/jpackage/junit/jdk.jpackage/jdk/jpackage/internal/DottedVersionTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2019, 2024, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -30,6 +30,7 @@ import org.junit.Test; import org.junit.rules.ExpectedException; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -54,45 +55,103 @@ public static List data() { @Rule public ExpectedException exceptionRule = ExpectedException.none(); + private static class CtorTester { + + CtorTester(String input, boolean greedy, String expectedSuffix, + int expectedComponentCount, String expectedToComponent) { + this.input = input; + this.greedy = greedy; + this.expectedSuffix = expectedSuffix; + this.expectedComponentCount = expectedComponentCount; + this.expectedToComponent = expectedToComponent; + } + + CtorTester(String input, boolean greedy, int expectedComponentCount, + String expectedToComponent) { + this(input, greedy, "", expectedComponentCount, expectedToComponent); + } + + CtorTester(String input, boolean greedy, int expectedComponentCount) { + this(input, greedy, "", expectedComponentCount, input); + } + + static CtorTester greedy(String input, int expectedComponentCount, + String expectedToComponent) { + return new CtorTester(input, true, "", expectedComponentCount, expectedToComponent); + } + + static CtorTester greedy(String input, int expectedComponentCount) { + return new CtorTester(input, true, "", expectedComponentCount, input); + } + + static CtorTester lazy(String input, String expectedSuffix, int expectedComponentCount, + String expectedToComponent) { + return new CtorTester(input, false, expectedSuffix, expectedComponentCount, + expectedToComponent); + } + + void run() { + DottedVersion dv; + if (greedy) { + dv = DottedVersion.greedy(input); + } else { + dv = DottedVersion.lazy(input); + } + + assertEquals(expectedSuffix, dv.getUnprocessedSuffix()); + assertEquals(expectedComponentCount, dv.getComponents().length); + assertEquals(expectedToComponent, dv.toComponentsString()); + } + + private final String input; + private final boolean greedy; + private final String expectedSuffix; + private final int expectedComponentCount; + private final String expectedToComponent; + } + @Test public void testValid() { - final List validStrings = List.of( - "1.0", - "1", - "2.234.045", - "2.234.0", - "0", - "0.1", - "9".repeat(1000) + final List validStrings = List.of( + new CtorTester("1.0", greedy, 2), + new CtorTester("1", greedy, 1), + new CtorTester("2.20034.045", greedy, 3, "2.20034.45"), + new CtorTester("2.234.0", greedy, 3), + new CtorTester("0", greedy, 1), + new CtorTester("0.1", greedy, 2), + new CtorTester("9".repeat(1000), greedy, 1), + new CtorTester("00.0.0", greedy, 3, "0.0.0") ); - final List validLazyStrings; + final List validLazyStrings; if (greedy) { validLazyStrings = Collections.emptyList(); } else { validLazyStrings = List.of( - "1.-1", - "5.", - "4.2.", - "3..2", - "2.a", - "0a", - ".", - " ", - " 1", - "1. 2", - "+1", - "-1", - "-0", - "+0" + CtorTester.lazy("1.-1", ".-1", 1, "1"), + CtorTester.lazy("5.", ".", 1, "5"), + CtorTester.lazy("4.2.", ".", 2, "4.2"), + CtorTester.lazy("3..2", "..2", 1, "3"), + CtorTester.lazy("3......2", "......2", 1, "3"), + CtorTester.lazy("2.a", ".a", 1, "2"), + CtorTester.lazy("a", "a", 0, ""), + CtorTester.lazy("2..a", "..a", 1, "2"), + CtorTester.lazy("0a", "a", 1, "0"), + CtorTester.lazy("120a", "a", 1, "120"), + CtorTester.lazy("120abc", "abc", 1, "120"), + CtorTester.lazy(".", ".", 0, ""), + CtorTester.lazy("....", "....", 0, ""), + CtorTester.lazy(" ", " ", 0, ""), + CtorTester.lazy(" 1", " 1", 0, ""), + CtorTester.lazy("678. 2", ". 2", 1, "678"), + CtorTester.lazy("+1", "+1", 0, ""), + CtorTester.lazy("-1", "-1", 0, ""), + CtorTester.lazy("-0", "-0", 0, ""), + CtorTester.lazy("+0", "+0", 0, "") ); } - Stream.concat(validStrings.stream(), validLazyStrings.stream()) - .forEach(value -> { - DottedVersion version = createTestee.apply(value); - assertEquals(version.toString(), value); - }); + Stream.concat(validStrings.stream(), validLazyStrings.stream()).forEach(CtorTester::run); } @Test @@ -108,8 +167,26 @@ public void testEmpty() { exceptionRule.expectMessage("Version may not be empty string"); createTestee.apply(""); } else { - assertTrue(0 == createTestee.apply("").compareTo("")); - assertTrue(0 == createTestee.apply("").compareTo("0")); + assertEquals(0, createTestee.apply("").getComponents().length); + } + } + + @Test + public void testEquals() { + DottedVersion dv = createTestee.apply("1.0"); + assertFalse(dv.equals(null)); + assertFalse(dv.equals(Integer.valueOf(1))); + + for (var ver : List.of("3", "3.4", "3.0.0")) { + DottedVersion a = createTestee.apply(ver); + DottedVersion b = createTestee.apply(ver); + assertTrue(a.equals(b)); + assertTrue(b.equals(a)); + } + + if (!greedy) { + assertTrue(createTestee.apply("3.6+67").equals(createTestee.apply("3.6+67"))); + assertFalse(createTestee.apply("3.6+67").equals(createTestee.apply("3.6+067"))); } }