Skip to content

Commit 63ab7fb

Browse files
Stephen Robertscopybara-github
authored andcommitted
Fix off-by-one error in CelExprFactory
PiperOrigin-RevId: 842335394
1 parent 8b4c579 commit 63ab7fb

File tree

2 files changed

+27
-1
lines changed

2 files changed

+27
-1
lines changed

common/src/main/java/dev/cel/common/ast/CelExprFactory.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -266,7 +266,7 @@ protected long nextExprId() {
266266

267267
/** Attempts to decrement the next expr ID if possible. */
268268
protected void maybeDeleteId(long id) {
269-
if (id == exprId - 1) {
269+
if (id == exprId) {
270270
exprId--;
271271
}
272272
}

common/src/test/java/dev/cel/common/ast/CelExprFactoryTest.java

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,4 +37,30 @@ public void nextExprId_startingDefaultIsOne() {
3737
assertThat(exprFactory.nextExprId()).isEqualTo(1L);
3838
assertThat(exprFactory.nextExprId()).isEqualTo(2L);
3939
}
40+
41+
@Test
42+
public void maybeDeleteId_deletesLastId() {
43+
CelExprFactory exprFactory = CelExprFactory.newInstance();
44+
long id1 = exprFactory.nextExprId(); // 1
45+
assertThat(id1).isEqualTo(1L);
46+
47+
exprFactory.maybeDeleteId(id1);
48+
49+
// Should be reused
50+
assertThat(exprFactory.nextExprId()).isEqualTo(1L);
51+
}
52+
53+
@Test
54+
public void maybeDeleteId_doesNotDeletePreviouslyAllocatedId() {
55+
CelExprFactory exprFactory = CelExprFactory.newInstance();
56+
long id1 = exprFactory.nextExprId(); // 1
57+
long id2 = exprFactory.nextExprId(); // 2
58+
59+
// Try to delete id1. Since id2 was allocated after, it should NOT delete id1
60+
// because that would rewind the counter and cause a collision with id2.
61+
exprFactory.maybeDeleteId(id1);
62+
63+
// Should NOT be reused. Next should be 3.
64+
assertThat(exprFactory.nextExprId()).isEqualTo(3L);
65+
}
4066
}

0 commit comments

Comments
 (0)