Skip to content

Fix order-dependent deletion for superset value lists (#260) #263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fym-rgb merged 1 commit into from
Aug 29, 2026
Merged

Fix order-dependent deletion for superset value lists (#260) #263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 31 additions & 40 deletions src/main/software/amazon/event/ruler/GenericMachine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,8 @@ public void addPatternRule(final T name, final Map<String, List<Patterns>> namev
* The eventual result will be same as one time deleteRule call with r1 {a, [1,2]}.
* So, caller is expected to save its rule expression if want to entirely remove the rule unless deliberately
* want to remove partial rule from rule name.
* Values within a key are order-independent; values that reach no rule are skipped while later values are
* still considered.
*
* @param name ARN of the rule
* @param namevals names and values which make up the rule
Expand Down Expand Up @@ -309,22 +311,13 @@ private Set<SubRuleContext> deleteStep(final NameState state,
: byteMachine.findAllPatterns(pattern);
}

if (nextNameStates.size() <= 1) {
NameState nextNameState = nextNameStates.isEmpty() ? null : nextNameStates.iterator().next();
if (nextNameState != null && deleteStepForNameState(state, key, keyIndex, keys, patterns, ruleName,
deletedKeys, candidateSubRuleIds, pattern, nextNameState, deletedSubRuleIds, nextNameStates)) {
return deletedSubRuleIds;
}
} else {
Set<SubRuleContext> survivingCandidates = new HashSet<>();
for (NameState nextNameState : nextNameStates) {
Set<SubRuleContext> incoming = new HashSet<>(candidateSubRuleIds);
deleteStepForNameState(state, key, keyIndex, keys, patterns, ruleName,
deletedKeys, incoming, pattern, nextNameState, deletedSubRuleIds, nextNameStates);
survivingCandidates.addAll(incoming);
}
candidateSubRuleIds.clear();
candidateSubRuleIds.addAll(survivingCandidates);
// Values within a key have OR semantics. Each value/NameState branch starts with the same candidates
// inherited from the preceding keys.
for (NameState nextNameState : nextNameStates) {
final Set<SubRuleContext> branchCandidateSubRuleIds = new HashSet<>(candidateSubRuleIds);
deleteStepForNameState(state, key, keyIndex, keys, patterns, ruleName,
deletedKeys, branchCandidateSubRuleIds, pattern, nextNameState, deletedSubRuleIds,
nextNameStates);
}
}

Expand All @@ -333,18 +326,18 @@ private Set<SubRuleContext> deleteStep(final NameState state,

// Deletes a single pattern via a single NameState. The teardown guard considers all NameStates the pattern can
// lead to (nextNameStates), so shared wildcard transitions survive while any other NameState still uses them.
private boolean deleteStepForNameState(final NameState state,
final String key,
final int keyIndex,
final List<String> keys,
final Map<String, List<Patterns>> patterns,
final T ruleName,
final List<String> deletedKeys,
final Set<SubRuleContext> candidateSubRuleIds,
final Patterns pattern,
final NameState nextNameState,
final Set<SubRuleContext> deletedSubRuleIds,
final Set<NameState> nextNameStates) {
private void deleteStepForNameState(final NameState state,
final String key,
final int keyIndex,
final List<String> keys,
final Map<String, List<Patterns>> patterns,
final T ruleName,
final List<String> deletedKeys,
final Set<SubRuleContext> candidateSubRuleIds,
final Patterns pattern,
final NameState nextNameState,
final Set<SubRuleContext> deletedSubRuleIds,
final Set<NameState> nextNameStates) {
// If this was the last step, then reaching the last state means the rule matched, and we should delete
// the rule from the next NameState.
final int nextKeyIndex = keyIndex + 1;
Expand All @@ -354,13 +347,12 @@ private boolean deleteStepForNameState(final NameState state,
Set<SubRuleContext> nextNameStateSubRuleIds = isTerminal ?
nextNameState.getTerminalSubRuleIdsForPattern(pattern) :
nextNameState.getNonTerminalSubRuleIdsForPattern(pattern);
// If no sub-rule IDs are found for next NameState, then we have no candidates, and will return below
// without further recursion through the keys.
if (nextNameStateSubRuleIds == null) {
candidateSubRuleIds.clear();
// If candidate set is empty, we are at first NameState, so initialize to next NameState's sub-rule IDs.
// When initializing, ensure that sub-rule IDs match the provided rule name for deletion.
} else if (candidateSubRuleIds.isEmpty()) {
return;
}
// Only the first key initializes candidates. An empty set at a later key means the preceding key
// constraints found no matching sub-rule and must not be reset.
if (keyIndex == 0) {
for (SubRuleContext nextNameStateSubRuleId : nextNameStateSubRuleIds) {
if (Objects.equals(ruleName,
nextNameStateSubRuleId.getRuleName())) {
Expand All @@ -372,6 +364,10 @@ private boolean deleteStepForNameState(final NameState state,
candidateSubRuleIds.retainAll(nextNameStateSubRuleIds);
}

if (candidateSubRuleIds.isEmpty()) {
return;
}

if (isTerminal) {
for (SubRuleContext candidateSubRuleId : candidateSubRuleIds) {
if (nextNameState.deleteSubRule(
Expand All @@ -387,11 +383,8 @@ private boolean deleteStepForNameState(final NameState state,
}
}
} else {
if (candidateSubRuleIds.isEmpty()) {
return true;
}
deletedSubRuleIds.addAll(deleteStep(nextNameState, keys, nextKeyIndex, patterns, ruleName,
deletedKeys, new HashSet<>(candidateSubRuleIds)));
deletedKeys, candidateSubRuleIds));

for (SubRuleContext deletedSubRuleId : deletedSubRuleIds) {
nextNameState.deleteSubRule(deletedSubRuleId.getRuleName(),
Expand All @@ -405,8 +398,6 @@ private boolean deleteStepForNameState(final NameState state,
state.removeNextNameState(key);
}
}

return false;
}

private boolean noNameStateContainsPattern(final Set<NameState> nameStates, final Patterns pattern) {
Expand Down
110 changes: 110 additions & 0 deletions src/test/software/amazon/event/ruler/MachineTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,6 +547,116 @@ public void testIssue255WildcardDeleteRuleOrderIndependent() throws Exception {
}
}

@Test
public void testIssue260SupersetExactDeleteIsOrderIndependent() throws Exception {
for (String deleteRule : asList(
"{\"x\":[\"a\",\"b\"],\"y\":[\"e\"]}",
"{\"x\":[\"b\",\"a\"],\"y\":[\"e\"]}")) {
Machine machine = Machine.builder().build();
machine.addRule("r1", "{\"x\":[\"b\"],\"y\":[\"e\"]}");
machine.addRule("r3", "{\"x\":[\"a\"],\"y\":[\"e\"]}");

machine.deleteRule("r1", deleteRule);

assertFalse(machine.rulesForJSONEvent("{\"x\":\"b\",\"y\":\"e\"}").contains("r1"));
assertEquals(Collections.singletonList("r3"),
machine.rulesForJSONEvent("{\"x\":\"a\",\"y\":\"e\"}"));
}
}

@Test
public void testIssue260SupersetWildcardDeleteIsOrderIndependent() throws Exception {
for (String deleteRule : asList(
"{\"x\":[{\"wildcard\":\"*foo*\"},{\"wildcard\":\"*bar*\"}],\"y\":[\"e\"]}",
"{\"x\":[{\"wildcard\":\"*bar*\"},{\"wildcard\":\"*foo*\"}],\"y\":[\"e\"]}")) {
Machine machine = Machine.builder().build();
String barRule = "{\"x\":[{\"wildcard\":\"*bar*\"}],\"y\":[\"e\"]}";
String fooRule = "{\"x\":[{\"wildcard\":\"*foo*\"}],\"y\":[\"e\"]}";
machine.addRule("r1", barRule);
machine.addRule("r2", barRule);
machine.addRule("r3", fooRule);

machine.deleteRule("r1", deleteRule);

assertEquals(Collections.singleton("r2"),
new HashSet<>(machine.rulesForJSONEvent("{\"x\":\"a-bar-b\",\"y\":\"e\"}")));
assertEquals(Collections.singletonList("r3"),
machine.rulesForJSONEvent("{\"x\":\"a-foo-b\",\"y\":\"e\"}"));
}
}

@Test
public void testIssue260SupersetDeletePreservesPriorKeyCandidates() throws Exception {
// Reuse funnels later-key values into one shared NameState, making foreign patterns findable there.
Machine machine = Machine.builder().withAdditionalNameStateReuse(true).build();
String firstRule = "{\"a\":[\"one\"],\"b\":[\"left\"],\"c\":[\"left-end\"]}";
String secondRule = "{\"a\":[\"two\"],\"b\":[\"right\"],\"c\":[\"right-end\"]}";
String otherRule = "{\"a\":[\"one\"],\"b\":[\"missing\"],\"c\":[\"other-end\"]}";
machine.addRule("r1", firstRule);
machine.addRule("r1", secondRule);
machine.addRule("other", otherRule);

machine.deleteRule("r1",
"{\"a\":[\"one\"],\"b\":[\"missing\",\"right\"],\"c\":[\"right-end\"]}");

assertTrue(machine.rulesForJSONEvent(
"{\"a\":\"one\",\"b\":\"left\",\"c\":\"left-end\"}").contains("r1"));
assertTrue(machine.rulesForJSONEvent(
"{\"a\":\"two\",\"b\":\"right\",\"c\":\"right-end\"}").contains("r1"));
assertTrue(machine.rulesForJSONEvent(
"{\"a\":\"one\",\"b\":\"missing\",\"c\":\"other-end\"}").contains("other"));
}

@Test
public void testIssue260TerminalSupersetDoesNotResetPriorKeyCandidates() throws Exception {
// Reuse funnels later-key values into one shared NameState, making foreign patterns findable there.
Machine machine = Machine.builder().withAdditionalNameStateReuse(true).build();
machine.addRule("r1", "{\"a\":[\"one\"],\"b\":[\"left\"]}");
machine.addRule("r1", "{\"a\":[\"two\"],\"b\":[\"right\"]}");
machine.addRule("other", "{\"a\":[\"one\"],\"b\":[\"missing\"]}");

machine.deleteRule("r1", "{\"a\":[\"one\"],\"b\":[\"missing\",\"right\"]}");

assertTrue(machine.rulesForJSONEvent("{\"a\":\"one\",\"b\":\"left\"}").contains("r1"));
assertTrue(machine.rulesForJSONEvent("{\"a\":\"two\",\"b\":\"right\"}").contains("r1"));
assertTrue(machine.rulesForJSONEvent("{\"a\":\"one\",\"b\":\"missing\"}").contains("other"));
}

@Test
public void testIssue260LaterKeySupersetDeleteContinuesPastEmptyBranch() throws Exception {
for (String deleteRule : asList(
"{\"a\":[\"one\"],\"b\":[\"missing\",\"right\"]}",
"{\"a\":[\"one\"],\"b\":[\"right\",\"missing\"]}")) {
// Reuse funnels later-key values into one NameState, exposing the branch-sharing edge case.
Machine machine = Machine.builder().withAdditionalNameStateReuse(true).build();
String targetRule = "{\"a\":[\"one\"],\"b\":[\"right\"]}";
String otherRule = "{\"a\":[\"one\"],\"b\":[\"missing\"]}";
machine.addRule("r1", targetRule);
machine.addRule("other", otherRule);

machine.deleteRule("r1", deleteRule);

assertTrue(machine.rulesForJSONEvent("{\"a\":\"one\",\"b\":\"right\"}").isEmpty());
assertEquals(Collections.singletonList("other"),
machine.rulesForJSONEvent("{\"a\":\"one\",\"b\":\"missing\"}"));
machine.deleteRule("other", otherRule);
assertTrue(machine.isEmpty());
}
}

@Test
public void testIssue260SupersetDeletesIndependentSameNameSubRules() throws Exception {
Machine machine = Machine.builder().build();
machine.addRule("r1", "{\"x\":[\"a\"]}");
machine.addRule("r1", "{\"x\":[\"b\"]}");

machine.deleteRule("r1", "{\"x\":[\"a\",\"b\"]}");

assertTrue(machine.rulesForJSONEvent("{\"x\":\"a\"}").isEmpty());
assertTrue(machine.rulesForJSONEvent("{\"x\":\"b\"}").isEmpty());
assertTrue(machine.isEmpty());
}

@Test
public void testCityLotsProblemLines() throws Exception {

Expand Down
Loading