From e774bb33ec7462fb7d4275d94e1a5bfd7f78fda3 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Thu, 23 Mar 2023 10:42:40 -0400 Subject: [PATCH 1/3] Optimize Quantity.parse to avoid regex overhead --- .../kubernetes/api/model/Quantity.java | 52 ++++++++++++++++--- .../kubernetes/api/model/QuantityTest.java | 29 +++++++++++ 2 files changed, 74 insertions(+), 7 deletions(-) diff --git a/kubernetes-model-generator/kubernetes-model-core/src/main/java/io/fabric8/kubernetes/api/model/Quantity.java b/kubernetes-model-generator/kubernetes-model-core/src/main/java/io/fabric8/kubernetes/api/model/Quantity.java index 31a6e9ebaf6..16ecd2f5ca3 100644 --- a/kubernetes-model-generator/kubernetes-model-core/src/main/java/io/fabric8/kubernetes/api/model/Quantity.java +++ b/kubernetes-model-generator/kubernetes-model-core/src/main/java/io/fabric8/kubernetes/api/model/Quantity.java @@ -55,8 +55,6 @@ }) @Buildable(editableEnabled = false, validationEnabled = false, generateBuilderPackage = true, builderPackage = "io.fabric8.kubernetes.api.builder") public class Quantity implements Serializable { - - private static final String AT_LEAST_ONE_DIGIT_REGEX = ".*\\d+.*"; private String amount; private String format = ""; private Map additionalProperties = new HashMap<>(); @@ -148,7 +146,7 @@ public static BigDecimal getAmountInBytes(Quantity quantity) throws ArithmeticEx Quantity amountFormatPair = parse(value); String formatStr = amountFormatPair.getFormat(); // Handle Decimal exponent case - if ((formatStr.matches(AT_LEAST_ONE_DIGIT_REGEX)) && formatStr.length() > 1) { + if (containsAtLeastOneDigit(formatStr) && formatStr.length() > 1) { int exponent = Integer.parseInt(formatStr.substring(1)); return new BigDecimal("10").pow(exponent, MathContext.DECIMAL64).multiply(new BigDecimal(amountFormatPair.getAmount())); } @@ -213,6 +211,19 @@ public static BigDecimal getAmountInBytes(Quantity quantity) throws ArithmeticEx return digit.multiply(multiple); } + /** + * @param value + * @return true if the specified value contains at least one digit, otherwise false + */ + static boolean containsAtLeastOneDigit(String value) { + for (int i = 0; i < value.length(); i++) { + if (Character.isDigit(value.charAt(i))) { + return true; + } + } + return false; + } + @Override public boolean equals(Object o) { if (this == o) { @@ -249,17 +260,44 @@ public static Quantity parse(String quantityAsString) { if (quantityAsString == null || quantityAsString.isEmpty()) { throw new IllegalArgumentException("Invalid quantity string format passed."); } - String[] quantityComponents = quantityAsString.split("[eEinumkKMGTP]+"); - String amountStr = quantityComponents[0]; - String formatStr = quantityAsString.substring(quantityComponents[0].length()); + + int unitIndex = indexOfUnit(quantityAsString); + String amountStr = quantityAsString.substring(0, unitIndex); + String formatStr = quantityAsString.substring(unitIndex); // For cases like 4e9 or 129e-6, formatStr would be e9 and e-6 respectively // we need to check whether this is valid too. It must not end with character. - if (formatStr.matches(AT_LEAST_ONE_DIGIT_REGEX) && Character.isAlphabetic(formatStr.charAt(formatStr.length() - 1))) { + if (containsAtLeastOneDigit(formatStr) && Character.isAlphabetic(formatStr.charAt(formatStr.length() - 1))) { throw new IllegalArgumentException("Invalid quantity string format passed"); } return new Quantity(amountStr, formatStr); } + /** + * @param quantityAsString quantity as a string + * @return the first index containing a unit character, or the length of the string if no element provided + */ + static int indexOfUnit(String quantityAsString) { + for (int i = 0; i < quantityAsString.length(); i++) { + char ch = quantityAsString.charAt(i); + switch (ch) { + case 'e': + case 'E': + case 'i': + case 'n': + case 'u': + case 'm': + case 'k': + case 'K': + case 'M': + case 'G': + case 'T': + case 'P': + return i; + } + } + return quantityAsString.length(); + } + @JsonAnyGetter public Map getAdditionalProperties() { return this.additionalProperties; diff --git a/kubernetes-model-generator/kubernetes-model-core/src/test/java/io/fabric8/kubernetes/api/model/QuantityTest.java b/kubernetes-model-generator/kubernetes-model-core/src/test/java/io/fabric8/kubernetes/api/model/QuantityTest.java index e050c4edfc3..5c1916dbb23 100644 --- a/kubernetes-model-generator/kubernetes-model-core/src/test/java/io/fabric8/kubernetes/api/model/QuantityTest.java +++ b/kubernetes-model-generator/kubernetes-model-core/src/test/java/io/fabric8/kubernetes/api/model/QuantityTest.java @@ -23,8 +23,10 @@ import java.math.BigDecimal; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; public class QuantityTest { private final ObjectMapper mapper = new ObjectMapper(); @@ -164,4 +166,31 @@ public void testParseFailure() { assertThrows(IllegalArgumentException.class, () -> Quantity.getAmountInBytes(new Quantity("4MiB"))); assertThrows(IllegalArgumentException.class, () -> Quantity.getAmountInBytes(new Quantity("4megabyte"))); } + + @Test + @DisplayName("Test containsAtLeastOneDigit method") + public void testContainsAtLeastOneDigit() { + assertTrue(Quantity.containsAtLeastOneDigit("0")); + assertTrue(Quantity.containsAtLeastOneDigit(".1")); + assertTrue(Quantity.containsAtLeastOneDigit(".1e2")); + assertTrue(Quantity.containsAtLeastOneDigit("1.2e3")); + assertTrue(Quantity.containsAtLeastOneDigit("-1K")); + + assertFalse(Quantity.containsAtLeastOneDigit("")); + assertFalse(Quantity.containsAtLeastOneDigit("e")); + assertFalse(Quantity.containsAtLeastOneDigit("Mi")); + } + + @Test + @DisplayName("Test indexOfUnit method") + public void testIndexOfUnit() { + assertEquals(0, Quantity.indexOfUnit("")); + assertEquals(1, Quantity.indexOfUnit("0")); + assertEquals(1, Quantity.indexOfUnit("1")); + assertEquals(3, Quantity.indexOfUnit("123")); + assertEquals(2, Quantity.indexOfUnit("12Mi")); + assertEquals(3, Quantity.indexOfUnit("123K")); + assertEquals(1, Quantity.indexOfUnit("1e3")); + assertEquals(4, Quantity.indexOfUnit("123 K")); + } } From fc5dd0d1fd679264be28cc05b52a852bb915ac88 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Thu, 23 Mar 2023 11:27:49 -0400 Subject: [PATCH 2/3] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 50633c64cd4..35ea221f3ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ #### Improvements * Fix #4477 exposing LeaderElector.release to force an elector to give up the lease +* Fix #4992: Optimize Quantity parsing to avoid regex overhead #### Dependency Upgrade From 115a52054716844ac8e3d08022cea059a161c9f3 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Thu, 23 Mar 2023 12:52:16 -0400 Subject: [PATCH 3/3] Address sonar warning and test coverage --- .../main/java/io/fabric8/kubernetes/api/model/Quantity.java | 3 +++ .../java/io/fabric8/kubernetes/api/model/QuantityTest.java | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/kubernetes-model-generator/kubernetes-model-core/src/main/java/io/fabric8/kubernetes/api/model/Quantity.java b/kubernetes-model-generator/kubernetes-model-core/src/main/java/io/fabric8/kubernetes/api/model/Quantity.java index 16ecd2f5ca3..065bde20052 100644 --- a/kubernetes-model-generator/kubernetes-model-core/src/main/java/io/fabric8/kubernetes/api/model/Quantity.java +++ b/kubernetes-model-generator/kubernetes-model-core/src/main/java/io/fabric8/kubernetes/api/model/Quantity.java @@ -293,6 +293,9 @@ static int indexOfUnit(String quantityAsString) { case 'T': case 'P': return i; + default: + //noinspection UnnecessaryContinue - satisfy Sonar + continue; } } return quantityAsString.length(); diff --git a/kubernetes-model-generator/kubernetes-model-core/src/test/java/io/fabric8/kubernetes/api/model/QuantityTest.java b/kubernetes-model-generator/kubernetes-model-core/src/test/java/io/fabric8/kubernetes/api/model/QuantityTest.java index 5c1916dbb23..6d539b3911b 100644 --- a/kubernetes-model-generator/kubernetes-model-core/src/test/java/io/fabric8/kubernetes/api/model/QuantityTest.java +++ b/kubernetes-model-generator/kubernetes-model-core/src/test/java/io/fabric8/kubernetes/api/model/QuantityTest.java @@ -88,6 +88,7 @@ public void testNegativeExponents() { public void testExponents() { assertEquals("129000000", Quantity.getAmountInBytes(new Quantity("129e6")).toString()); assertEquals("129000000", Quantity.getAmountInBytes(new Quantity("129e+6")).toString()); + assertEquals("1234567890", Quantity.getAmountInBytes(new Quantity("1234567890")).toString()); assertEquals("8192", Quantity.getAmountInBytes(new Quantity("8Ki")).toString()); assertEquals("7340032", Quantity.getAmountInBytes(new Quantity("7Mi")).toString()); assertEquals("6442450944", Quantity.getAmountInBytes(new Quantity("6Gi")).toString()); @@ -131,6 +132,7 @@ public void testEquality() { public void testExponent() { assertEquals("10000", Quantity.getAmountInBytes(new Quantity("1e4")).toString()); assertEquals("2000000000", Quantity.getAmountInBytes(new Quantity("2E9")).toString()); + assertEquals("2000000000000", Quantity.getAmountInBytes(new Quantity("2E12")).toString()); } @Test @@ -150,6 +152,7 @@ public void testStringConversion() { assertEquals("1Ki", new Quantity("1Ki").toString()); assertEquals("32Mi", new Quantity("32Mi").toString()); assertEquals("1e3", new Quantity("1e3").toString()); + assertEquals("1e10", new Quantity("1e10").toString()); assertEquals("1e-3", new Quantity("1e-3").toString()); assertEquals("100k", new Quantity("100k").toString()); assertEquals("100001m", new Quantity("100001m").toString()); @@ -165,6 +168,7 @@ public void testParseFailure() { assertThrows(IllegalArgumentException.class, () -> Quantity.getAmountInBytes(new Quantity())); assertThrows(IllegalArgumentException.class, () -> Quantity.getAmountInBytes(new Quantity("4MiB"))); assertThrows(IllegalArgumentException.class, () -> Quantity.getAmountInBytes(new Quantity("4megabyte"))); + assertThrows(IllegalArgumentException.class, () -> Quantity.getAmountInBytes(new Quantity("4c"))); } @Test @@ -192,5 +196,6 @@ public void testIndexOfUnit() { assertEquals(3, Quantity.indexOfUnit("123K")); assertEquals(1, Quantity.indexOfUnit("1e3")); assertEquals(4, Quantity.indexOfUnit("123 K")); + assertEquals(4, Quantity.indexOfUnit("123c")); } }