diff --git a/common/src/main/java/dev/cel/common/ast/CelExprFactory.java b/common/src/main/java/dev/cel/common/ast/CelExprFactory.java index 9ad23952e..6718f6d04 100644 --- a/common/src/main/java/dev/cel/common/ast/CelExprFactory.java +++ b/common/src/main/java/dev/cel/common/ast/CelExprFactory.java @@ -266,7 +266,7 @@ protected long nextExprId() { /** Attempts to decrement the next expr ID if possible. */ protected void maybeDeleteId(long id) { - if (id == exprId - 1) { + if (id == exprId) { exprId--; } } diff --git a/common/src/test/java/dev/cel/common/ast/CelExprFactoryIntegrationTest.java b/common/src/test/java/dev/cel/common/ast/CelExprFactoryIntegrationTest.java new file mode 100644 index 000000000..da4b7b80d --- /dev/null +++ b/common/src/test/java/dev/cel/common/ast/CelExprFactoryIntegrationTest.java @@ -0,0 +1,39 @@ +// Copyright 2025 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package dev.cel.common.ast; + +import dev.cel.compiler.CelCompiler; +import dev.cel.compiler.CelCompilerFactory; +import dev.cel.parser.CelMacro; +import java.util.Optional; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class CelExprFactoryIntegrationTest { + + @Test + public void macroExpandingToSingleNode_doesNotCauseIdCollision() throws Exception { + CelMacro tcMacro = + CelMacro.newGlobalMacro( + "tc", 0, (factory, target, args) -> Optional.of(factory.newIntLiteral(0))); + + CelCompiler compiler = + CelCompilerFactory.standardCelCompilerBuilder().addMacros(tcMacro).build(); + + compiler.compile("tc() == 0"); + } +} diff --git a/common/src/test/java/dev/cel/common/ast/CelExprFactoryTest.java b/common/src/test/java/dev/cel/common/ast/CelExprFactoryTest.java index f3d77ea9e..0ab610cbc 100644 --- a/common/src/test/java/dev/cel/common/ast/CelExprFactoryTest.java +++ b/common/src/test/java/dev/cel/common/ast/CelExprFactoryTest.java @@ -37,4 +37,30 @@ public void nextExprId_startingDefaultIsOne() { assertThat(exprFactory.nextExprId()).isEqualTo(1L); assertThat(exprFactory.nextExprId()).isEqualTo(2L); } + + @Test + public void maybeDeleteId_deletesLastId() { + CelExprFactory exprFactory = CelExprFactory.newInstance(); + long id1 = exprFactory.nextExprId(); // 1 + assertThat(id1).isEqualTo(1L); + + exprFactory.maybeDeleteId(id1); + + // Should be reused + assertThat(exprFactory.nextExprId()).isEqualTo(1L); + } + + @Test + public void maybeDeleteId_doesNotDeletePreviouslyAllocatedId() { + CelExprFactory exprFactory = CelExprFactory.newInstance(); + long id1 = exprFactory.nextExprId(); // 1 + long id2 = exprFactory.nextExprId(); // 2 + + // Try to delete id1. Since id2 was allocated after, it should NOT delete id1 + // because that would rewind the counter and cause a collision with id2. + exprFactory.maybeDeleteId(id1); + + // Should NOT be reused. Next should be 3. + assertThat(exprFactory.nextExprId()).isEqualTo(3L); + } }