Skip to content

Commit 471ecca

Browse files
Christopher-Chianellitriceo
authored andcommitted
fix: correctly identify variable access as INDIRECT if accessed after a fact
The path "fact.variable" was incorrectly identified as "NO_PARENT" instead of "INDIRECT". This can cause an unsuitable graph structure to be used, causing score corruption.
1 parent d9cc89a commit 471ecca

File tree

5 files changed

+16
-9
lines changed

5 files changed

+16
-9
lines changed

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/DefaultShadowVariableSessionFactory.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ yield buildSingleDirectionalParentGraph(solutionDescriptor,
6161
graphStructureAndDirection,
6262
entities);
6363
}
64-
case ARBITRARY_SINGLE_ENTITY_SINGLE_DIRECTIONAL_PARENT_TYPE ->
64+
case ARBITRARY_SINGLE_ENTITY_AT_MOST_ONE_DIRECTIONAL_PARENT_TYPE ->
6565
buildArbitrarySingleEntityGraph(solutionDescriptor, variableReferenceGraphBuilder, entities, graphCreator);
6666
case NO_DYNAMIC_EDGES, ARBITRARY ->
6767
buildArbitraryGraph(solutionDescriptor, variableReferenceGraphBuilder, entities, graphCreator);

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/GraphStructure.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ public enum GraphStructure {
4242
* entity that uses declarative shadow variables with all directional
4343
* parents being the same type.
4444
*/
45-
ARBITRARY_SINGLE_ENTITY_SINGLE_DIRECTIONAL_PARENT_TYPE,
45+
ARBITRARY_SINGLE_ENTITY_AT_MOST_ONE_DIRECTIONAL_PARENT_TYPE,
4646

4747
/**
4848
* A graph structure that accepts all graphs.
@@ -73,7 +73,7 @@ public static <Solution_> GraphStructureAndDirection determineGraphStructure(
7373
.distinct().count() > 1;
7474

7575
final var arbitraryGraphStructure = new GraphStructureAndDirection(
76-
multipleDeclarativeEntityClasses ? ARBITRARY : ARBITRARY_SINGLE_ENTITY_SINGLE_DIRECTIONAL_PARENT_TYPE,
76+
multipleDeclarativeEntityClasses ? ARBITRARY : ARBITRARY_SINGLE_ENTITY_AT_MOST_ONE_DIRECTIONAL_PARENT_TYPE,
7777
null, null);
7878

7979
var rootVariableSources = declarativeShadowVariableDescriptors.stream()

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/RootVariableSource.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,7 @@ public static <Entity_, Value_> RootVariableSource<Entity_, Value_> from(
192192
assertIsValidVariableReference(rootEntityClass, variablePath, variableSourceReference);
193193
}
194194

195-
if (parentVariableType != ParentVariableType.GROUP && variableSourceReferences.size() == 1) {
195+
if (!parentVariableType.isIndirect() && variableSourceReferences.size() == 1) {
196196
// No variables are accessed from the parent, so there no
197197
// parent variable.
198198
parentVariableType = ParentVariableType.NO_PARENT;

core/src/test/java/ai/timefold/solver/core/impl/domain/variable/declarative/GraphStructureTest.java

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,8 @@
11
package ai.timefold.solver.core.impl.domain.variable.declarative;
22

33
import static ai.timefold.solver.core.impl.domain.variable.declarative.GraphStructure.ARBITRARY;
4-
import static ai.timefold.solver.core.impl.domain.variable.declarative.GraphStructure.ARBITRARY_SINGLE_ENTITY_SINGLE_DIRECTIONAL_PARENT_TYPE;
4+
import static ai.timefold.solver.core.impl.domain.variable.declarative.GraphStructure.ARBITRARY_SINGLE_ENTITY_AT_MOST_ONE_DIRECTIONAL_PARENT_TYPE;
55
import static ai.timefold.solver.core.impl.domain.variable.declarative.GraphStructure.EMPTY;
6-
import static ai.timefold.solver.core.impl.domain.variable.declarative.GraphStructure.NO_DYNAMIC_EDGES;
76
import static ai.timefold.solver.core.impl.domain.variable.declarative.GraphStructure.SINGLE_DIRECTIONAL_PARENT;
87
import static org.assertj.core.api.Assertions.assertThat;
98

@@ -53,7 +52,7 @@ void simpleChainedStructure() {
5352
var entity = new TestdataChainedSimpleVarValue();
5453
assertThat(GraphStructure.determineGraphStructure(
5554
TestdataChainedSimpleVarSolution.buildSolutionDescriptor(), entity))
56-
.hasFieldOrPropertyWithValue("structure", ARBITRARY_SINGLE_ENTITY_SINGLE_DIRECTIONAL_PARENT_TYPE);
55+
.hasFieldOrPropertyWithValue("structure", ARBITRARY_SINGLE_ENTITY_AT_MOST_ONE_DIRECTIONAL_PARENT_TYPE);
5756
}
5857

5958
@Test
@@ -94,15 +93,15 @@ void concurrentValuesStructureWithGroups() {
9493
assertThat(GraphStructure.determineGraphStructure(
9594
TestdataConcurrentSolution.buildSolutionDescriptor(),
9695
value1, value2))
97-
.hasFieldOrPropertyWithValue("structure", ARBITRARY_SINGLE_ENTITY_SINGLE_DIRECTIONAL_PARENT_TYPE);
96+
.hasFieldOrPropertyWithValue("structure", ARBITRARY_SINGLE_ENTITY_AT_MOST_ONE_DIRECTIONAL_PARENT_TYPE);
9897
}
9998

10099
@Test
101100
void followerStructure() {
102101
var entity = new TestdataFollowerEntity();
103102
assertThat(GraphStructure.determineGraphStructure(
104103
TestdataFollowerSolution.buildSolutionDescriptor(), entity))
105-
.hasFieldOrPropertyWithValue("structure", NO_DYNAMIC_EDGES);
104+
.hasFieldOrPropertyWithValue("structure", ARBITRARY_SINGLE_ENTITY_AT_MOST_ONE_DIRECTIONAL_PARENT_TYPE);
106105
}
107106

108107
@Test

core/src/test/java/ai/timefold/solver/core/impl/domain/variable/declarative/RootVariableSourceTest.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ void pathUsingBuiltinShadow() {
7878

7979
assertThat(rootVariableSource.rootEntity()).isEqualTo(TestdataInvalidDeclarativeValue.class);
8080
assertThat(rootVariableSource.variableSourceReferences()).hasSize(1);
81+
assertThat(rootVariableSource.parentVariableType()).isEqualTo(ParentVariableType.NO_PARENT);
8182
var source = rootVariableSource.variableSourceReferences().get(0);
8283

8384
assertEmptyChainToVariableEntity(source);
@@ -112,6 +113,7 @@ void pathUsingDeclarativeShadow() {
112113

113114
assertThat(rootVariableSource.rootEntity()).isEqualTo(TestdataInvalidDeclarativeValue.class);
114115
assertThat(rootVariableSource.variableSourceReferences()).hasSize(1);
116+
assertThat(rootVariableSource.parentVariableType()).isEqualTo(ParentVariableType.NO_PARENT);
115117
var source = rootVariableSource.variableSourceReferences().get(0);
116118

117119
assertEmptyChainToVariableEntity(source);
@@ -145,6 +147,7 @@ void pathUsingDeclarativeShadowAfterGroup() {
145147

146148
assertThat(rootVariableSource.rootEntity()).isEqualTo(TestdataInvalidDeclarativeValue.class);
147149
assertThat(rootVariableSource.variableSourceReferences()).hasSize(1);
150+
assertThat(rootVariableSource.parentVariableType()).isEqualTo(ParentVariableType.GROUP);
148151
var source = rootVariableSource.variableSourceReferences().get(0);
149152

150153
assertEmptyChainToVariableEntity(source);
@@ -183,6 +186,7 @@ void pathUsingBuiltinShadowAfterGroup() {
183186

184187
assertThat(rootVariableSource.rootEntity()).isEqualTo(TestdataInvalidDeclarativeValue.class);
185188
assertThat(rootVariableSource.variableSourceReferences()).hasSize(1);
189+
assertThat(rootVariableSource.parentVariableType()).isEqualTo(ParentVariableType.GROUP);
186190
var source = rootVariableSource.variableSourceReferences().get(0);
187191

188192
assertEmptyChainToVariableEntity(source);
@@ -221,6 +225,7 @@ void pathUsingDeclarativeShadowAfterGroupAfterFact() {
221225

222226
assertThat(rootVariableSource.rootEntity()).isEqualTo(TestdataInvalidDeclarativeValue.class);
223227
assertThat(rootVariableSource.variableSourceReferences()).hasSize(1);
228+
assertThat(rootVariableSource.parentVariableType()).isEqualTo(ParentVariableType.GROUP);
224229
var source = rootVariableSource.variableSourceReferences().get(0);
225230

226231
assertEmptyChainToVariableEntity(source);
@@ -261,6 +266,7 @@ void pathUsingDeclarativeShadowAfterBuiltinShadow() {
261266

262267
assertThat(rootVariableSource.rootEntity()).isEqualTo(TestdataInvalidDeclarativeValue.class);
263268
assertThat(rootVariableSource.variableSourceReferences()).hasSize(2);
269+
assertThat(rootVariableSource.parentVariableType()).isEqualTo(ParentVariableType.PREVIOUS);
264270
var previousSource = rootVariableSource.variableSourceReferences().get(0);
265271

266272
assertEmptyChainToVariableEntity(previousSource);
@@ -305,6 +311,7 @@ void pathUsingBuiltinShadowAfterFact() {
305311

306312
assertThat(rootVariableSource.rootEntity()).isEqualTo(TestdataInvalidDeclarativeValue.class);
307313
assertThat(rootVariableSource.variableSourceReferences()).hasSize(1);
314+
assertThat(rootVariableSource.parentVariableType()).isEqualTo(ParentVariableType.INDIRECT);
308315
var previousSource = rootVariableSource.variableSourceReferences().get(0);
309316

310317
assertChainToVariableEntity(previousSource, "fact");
@@ -344,6 +351,7 @@ void pathUsingDeclarativeShadowAfterBuiltinShadowAfterGroup() {
344351

345352
assertThat(rootVariableSource.rootEntity()).isEqualTo(TestdataInvalidDeclarativeValue.class);
346353
assertThat(rootVariableSource.variableSourceReferences()).hasSize(2);
354+
assertThat(rootVariableSource.parentVariableType()).isEqualTo(ParentVariableType.GROUP);
347355
var previousSource = rootVariableSource.variableSourceReferences().get(0);
348356

349357
assertEmptyChainToVariableEntity(previousSource);

0 commit comments

Comments
 (0)