diff --git a/poi-ooxml/src/main/java/org/apache/poi/ooxml/util/IdentifierManager.java b/poi-ooxml/src/main/java/org/apache/poi/ooxml/util/IdentifierManager.java index 981727aba4..7121651f06 100644 --- a/poi-ooxml/src/main/java/org/apache/poi/ooxml/util/IdentifierManager.java +++ b/poi-ooxml/src/main/java/org/apache/poi/ooxml/util/IdentifierManager.java @@ -39,17 +39,15 @@ public class IdentifierManager { public IdentifierManager(long lowerbound, long upperbound) { if (lowerbound > upperbound) { throw new IllegalArgumentException("lowerbound must not be greater than upperbound, had " + lowerbound + " and " + upperbound); - } - else if (lowerbound < MIN_ID) { - String message = "lowerbound must be greater than or equal to " + Long.toString(MIN_ID); + } else if (lowerbound < MIN_ID) { + String message = "lowerbound must be greater than or equal to " + MIN_ID; throw new IllegalArgumentException(message); - } - else if (upperbound > MAX_ID) { + } else if (upperbound > MAX_ID) { /* * while MAX_ID is Long.MAX_VALUE, this check is pointless. But if * someone subclasses / tweaks the limits, this check is fine. */ - throw new IllegalArgumentException("upperbound must be less than or equal to " + Long.toString(MAX_ID) + " but had " + upperbound); + throw new IllegalArgumentException("upperbound must be less than or equal to " + MAX_ID + " but had " + upperbound); } this.lowerbound = lowerbound; this.upperbound = upperbound; @@ -92,25 +90,21 @@ public class IdentifierManager { Segment segment = iter.next(); if (segment.end < id) { continue; - } - else if (segment.start > id) { + } else if (segment.start > id) { break; - } - else if (segment.start == id) { + } else if (segment.start == id) { segment.start = id + 1; if (segment.end < segment.start) { iter.remove(); } return id; - } - else if (segment.end == id) { + } else if (segment.end == id) { segment.end = id - 1; if (segment.start > segment.end) { iter.remove(); } return id; - } - else { + } else { iter.add(new Segment(id + 1, segment.end)); segment.end = id - 1; return id; @@ -159,6 +153,13 @@ public class IdentifierManager { } } + // if there are no segments then all are reserved currently + // and so we need to mark this id as released now + if (segments.isEmpty()) { + segments.add(new Segment(id, id)); + return true; + } + if (id == lowerbound) { Segment firstSegment = segments.getFirst(); if (firstSegment.start == lowerbound + 1) { @@ -189,8 +190,7 @@ public class IdentifierManager { if (segment.start == higher) { segment.start = id; return true; - } - else if (segment.end == lower) { + } else if (segment.end == lower) { segment.end = id; /* check if releasing this elements glues two segments into one */ if (iter.hasNext()) { @@ -201,8 +201,7 @@ public class IdentifierManager { } } return true; - } - else { + } else { /* id was not reserved, return false */ break; } @@ -224,7 +223,8 @@ public class IdentifierManager { */ private void verifyIdentifiersLeft() { if (segments.isEmpty()) { - throw new IllegalStateException("No identifiers left"); + throw new IllegalStateException("No identifiers left for range " + + "[" + lowerbound + "," + upperbound + "]"); } } diff --git a/poi-ooxml/src/test/java/org/apache/poi/util/tests/TestIdentifierManager.java b/poi-ooxml/src/test/java/org/apache/poi/util/tests/TestIdentifierManager.java index 370b2c3a56..d239754692 100644 --- a/poi-ooxml/src/test/java/org/apache/poi/util/tests/TestIdentifierManager.java +++ b/poi-ooxml/src/test/java/org/apache/poi/util/tests/TestIdentifierManager.java @@ -27,8 +27,7 @@ import org.junit.jupiter.api.Test; class TestIdentifierManager { @Test - void testBasic() - { + void testBasic() { IdentifierManager manager = new IdentifierManager(0L,100L); assertEquals(101L,manager.getRemainingIdentifiers()); assertEquals(0L,manager.reserveNew()); @@ -42,6 +41,7 @@ class TestIdentifierManager { long min = IdentifierManager.MIN_ID; long max = IdentifierManager.MAX_ID; IdentifierManager manager = new IdentifierManager(min,max); + //noinspection ConstantValue assertTrue(max - min + 1 > 0, "Limits lead to a long variable overflow"); assertTrue(manager.getRemainingIdentifiers() > 0, "Limits lead to a long variable overflow"); assertEquals(min,manager.reserveNew()); @@ -103,4 +103,60 @@ class TestIdentifierManager { assertEquals(11L,manager.reserve(11L)); assertTrue(manager.release(12L)); } + + @Test + void testBounds() { + assertThrows(IllegalArgumentException.class, + () -> new IdentifierManager(1, 0), + "Lower bound higher than upper"); + + assertThrows(IllegalArgumentException.class, + () -> new IdentifierManager(-1, 0), + "Out of lower bound"); + + assertThrows(IllegalArgumentException.class, + () -> new IdentifierManager(0, Long.MAX_VALUE), + "Out of upper bound"); + + IdentifierManager manager = new IdentifierManager(10, 100); + assertThrows(IllegalArgumentException.class, + () -> manager.reserve(9), + "Reserve out of lower bound"); + assertThrows(IllegalArgumentException.class, + () -> manager.reserve(101), + "Reserve out of upper bound"); + + // normal reserving works + assertEquals(10, manager.reserve(10)); + assertEquals(100, manager.reserve(100)); + + assertEquals(11, manager.reserve(10)); + assertEquals(12, manager.reserve(100)); + + for (int i = 13; i < 100; i++) { + assertEquals(i, manager.reserve(10)); + assertEquals(100 - i - 1, + manager.getRemainingIdentifiers()); + } + + // now the manager is exhausted + assertThrows(IllegalStateException.class, + () -> manager.reserve(10), + "No more ids left any more"); + + assertThrows(IllegalArgumentException.class, + () -> manager.release(9), + "Release out of lower bound"); + assertThrows(IllegalArgumentException.class, + () -> manager.release(101), + "Release out of upper bound"); + + for (int i = 10; i < 100; i++) { + assertTrue(manager.release(i)); + assertEquals(i - 10 + 1, + manager.getRemainingIdentifiers()); + } + + assertEquals(100 - 10, manager.getRemainingIdentifiers()); + } }