Fix a case in IdentifierManager.release(), reformat, add tests
The case when all are reserved was not handled properly
git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1909807 13f79535-47bb-0310-9956-ffa450edef68
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 981727a..7121651 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 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 @@
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 @@
}
}
+ // 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 @@
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 @@
}
}
return true;
- }
- else {
+ } else {
/* id was not reserved, return false */
break;
}
@@ -224,7 +223,8 @@
*/
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 370b2c3..d239754 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 @@
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 @@
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 @@
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());
+ }
}
