Skip to content

Commit 79544f2

Browse files
atextoratextor
authored
Merge pull request #350 from bci-oss/bugfix/ESMF-315-improve-cycle-detection
Extend cycle detection to recognize loops after optional properties.
2 parents c912678 + d21f4b7 commit 79544f2

File tree

4 files changed

+161
-88
lines changed

4 files changed

+161
-88
lines changed

‎core/esmf-aspect-model-validator/src/main/java/org/eclipse/esmf/aspectmodel/validation/services/ModelCycleDetector.java‎

Lines changed: 86 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,8 @@
4040
import org.eclipse.esmf.aspectmodel.resolver.services.VersionedModel;
4141
import org.eclipse.esmf.aspectmodel.shacl.violation.ProcessingViolation;
4242
import org.eclipse.esmf.aspectmodel.shacl.violation.Violation;
43-
44-
import org.eclipse.esmf.samm.KnownVersion;
45-
4643
import org.eclipse.esmf.aspectmodel.vocabulary.SAMM;
44+
import org.eclipse.esmf.samm.KnownVersion;
4745

4846
/**
4947
* Cycle detector for SAMM models.
@@ -61,11 +59,14 @@ public class ModelCycleDetector {
6159
static String ERR_CYCLE_DETECTED =
6260
"The Aspect Model contains a cycle which includes following properties: %s. Please remove any cycles that do not allow a finite json payload.";
6361

64-
private final static String prefixes = "prefix samm: <urn:samm:org.eclipse.esmf.samm:meta-model:%s#> \r\n" +
65-
"prefix samm-c: <urn:samm:org.eclipse.esmf.samm:characteristic:%s#> \r\n" +
66-
"prefix rdf: <http://www.w3.org/1999/02/22-rdf-syntax-ns#> \r\n";
62+
private final static String prefixes = """
63+
prefix samm: <urn:samm:org.eclipse.esmf.samm:meta-model:%s#>
64+
prefix samm-c: <urn:samm:org.eclipse.esmf.samm:characteristic:%s#>
65+
prefix rdf: <http://www.w3.org/1999/02/22-rdf-syntax-ns#>
66+
""";
6767

6868
final Set<String> discovered = new LinkedHashSet<>();
69+
final Set<String> discoveredOptionals = new HashSet<>();
6970
final Set<String> finished = new HashSet<>();
7071

7172
private Query query;
@@ -77,6 +78,7 @@ public class ModelCycleDetector {
7778

7879
public List<Violation> validateModel( final VersionedModel versionedModel ) {
7980
discovered.clear();
81+
discoveredOptionals.clear();
8082
finished.clear();
8183
cycleDetectionReport.clear();
8284

@@ -94,19 +96,7 @@ public List<Violation> validateModel( final VersionedModel versionedModel ) {
9496
final Iterator<RDFNode> aspectProperties = properties.getList().iterator();
9597
while ( aspectProperties.hasNext() ) {
9698
final RDFNode propRef = aspectProperties.next();
97-
final Resource property;
98-
if ( propRef.isAnon() ) {
99-
if ( isOptionalProperty( propRef.asResource() ) ) {
100-
continue;
101-
}
102-
property = resolvePropertyReference( propRef.asResource() );
103-
} else {
104-
property = propRef.asResource();
105-
}
106-
final String propertyName = getUniqueName( property );
107-
if ( !discovered.contains( propertyName ) && !finished.contains( propertyName ) ) {
108-
depthFirstTraversal( property, this::reportCycle );
109-
}
99+
depthFirstTraversal( propRef.asResource(), this::reportCycle );
110100
}
111101
}
112102
}
@@ -115,56 +105,76 @@ public List<Violation> validateModel( final VersionedModel versionedModel ) {
115105
}
116106

117107
private void depthFirstTraversal( final Resource currentProperty, final BiConsumer<String, Set<String>> cycleHandler ) {
118-
final String currentPropertyName = getUniqueName( currentProperty );
119-
discovered.add( currentPropertyName );
120-
121-
final List<NextHopProperty> nextHopProperties = getDirectlyReachableProperties( model, currentProperty );
122-
123-
// samm-c:Either makes the task somewhat more complicated - we need to know the status of both branches (left/right)
124-
// to be able to decide whether there really is a cycle or not
125-
if ( reachedViaEither( nextHopProperties ) ) {
126-
final EitherCycleDetector leftBranch = new EitherCycleDetector( currentPropertyName, this::reportCycle );
127-
final EitherCycleDetector rightBranch = new EitherCycleDetector( currentPropertyName, this::reportCycle );
128-
nextHopProperties.stream().filter( property -> property.eitherStatus == 1 )
129-
.forEach( property -> investigateProperty( property.propertyNode, leftBranch::collectCycles ) );
130-
nextHopProperties.stream().filter( property -> property.eitherStatus == 2 )
131-
.forEach( property -> investigateProperty( property.propertyNode, rightBranch::collectCycles ) );
132-
if ( leftBranch.hasBreakableCycles() && rightBranch.hasBreakableCycles() ) {
133-
// the cycles found are breakable, but they are present in both branches, resulting in an overall unbreakable cycle
134-
leftBranch.reportCycles( this::reportCycle );
135-
rightBranch.reportCycles( this::reportCycle );
108+
final Resource resolvedProperty = currentProperty.isAnon() ? resolvePropertyReference( currentProperty.asResource() ) : currentProperty.asResource();
109+
final String currentPropertyName = getUniqueName( resolvedProperty );
110+
if ( finished.contains( currentPropertyName ) ) {
111+
return;
112+
}
113+
final boolean isOptional = isOptionalProperty( currentProperty );
114+
if ( isOptional ) {
115+
discoveredOptionals.add( currentPropertyName );
116+
}
117+
118+
if ( discovered.contains( currentPropertyName ) ) {
119+
// found a back edge -> cycle detected; report it as such only if not broken by an optional property
120+
if ( !optionalPropertyAtOrBelowBackEdge( currentPropertyName ) ) {
121+
cycleHandler.accept( currentPropertyName, discovered );
122+
}
123+
} else {
124+
discovered.add( currentPropertyName );
125+
126+
final List<NextHopProperty> nextHopProperties = getDirectlyReachableProperties( model, resolvedProperty );
127+
128+
// samm-c:Either makes the task somewhat more complicated - we need to know the status of both branches (left/right)
129+
// to be able to decide whether there really is a cycle or not
130+
if ( reachedViaEither( nextHopProperties ) ) {
131+
final EitherCycleDetector leftBranch = new EitherCycleDetector( currentPropertyName, this::reportCycle );
132+
final EitherCycleDetector rightBranch = new EitherCycleDetector( currentPropertyName, this::reportCycle );
133+
nextHopProperties.stream().filter( property -> property.eitherStatus == 1 )
134+
.forEach( property -> depthFirstTraversal( property.propertyNode, leftBranch::collectCycles ) );
135+
nextHopProperties.stream().filter( property -> property.eitherStatus == 2 )
136+
.forEach( property -> depthFirstTraversal( property.propertyNode, rightBranch::collectCycles ) );
137+
if ( leftBranch.hasBreakableCycles() && rightBranch.hasBreakableCycles() ) {
138+
// the cycles found are breakable, but they are present in both branches, resulting in an overall unbreakable cycle
139+
leftBranch.reportCycles( this::reportCycle );
140+
rightBranch.reportCycles( this::reportCycle );
141+
}
142+
} else { // "normal" path
143+
nextHopProperties.forEach( property -> depthFirstTraversal( property.propertyNode, cycleHandler ) );
136144
}
137-
} else { // "normal" path
138-
nextHopProperties.forEach( property -> investigateProperty( property.propertyNode, cycleHandler ) );
145+
146+
discovered.remove( currentPropertyName );
147+
finished.add( currentPropertyName );
139148
}
140149

141-
discovered.remove( currentPropertyName );
142-
finished.add( currentPropertyName );
150+
if ( isOptional ) {
151+
discoveredOptionals.remove( currentPropertyName );
152+
}
143153
}
144154

145155
private boolean reachedViaEither( final List<NextHopProperty> nextHopProperties ) {
146156
return nextHopProperties.stream().anyMatch( property -> property.eitherStatus > 0 );
147157
}
148158

149-
private void investigateProperty( Resource propertyNode, final BiConsumer<String, Set<String>> cycleHandler ) {
150-
if ( propertyNode.isAnon() ) {
151-
// [ samm:property :propName ; samm:optional value ; ]
152-
if ( isOptionalProperty( propertyNode ) ) {
153-
// presence of samm:optional = true; no need to continue on this path, the potential cycle would be broken by the optional property anyway
154-
return;
159+
private boolean optionalPropertyAtOrBelowBackEdge( final String backEdge ) {
160+
if ( discoveredOptionals.contains( backEdge ) ) {
161+
return true;
162+
}
163+
final Iterator<String> path = discovered.iterator();
164+
// first find the back edge property within the current path
165+
while ( path.hasNext() ) {
166+
final String currentNode = path.next();
167+
if ( currentNode.equals( backEdge ) ) {
168+
break;
155169
}
156-
157-
propertyNode = resolvePropertyReference( propertyNode );
158170
}
159-
160-
final String propertyName = getUniqueName( propertyNode );
161-
162-
if ( discovered.contains( propertyName ) ) {
163-
// found a back edge -> cycle detected
164-
cycleHandler.accept( propertyName, discovered );
165-
} else if ( !finished.contains( propertyName ) ) {
166-
depthFirstTraversal( propertyNode, cycleHandler );
171+
// look for an optional property at or below the back edge property
172+
while ( path.hasNext() ) {
173+
if ( discoveredOptionals.contains( path.next() ) ) {
174+
return true;
175+
}
167176
}
177+
return false;
168178
}
169179

170180
private Resource resolvePropertyReference( final Resource propertyNode ) {
@@ -212,24 +222,25 @@ private void reportCycle( final String cyclePath ) {
212222

213223
private void initializeQuery( final KnownVersion metaModelVersion ) {
214224
final String currentVersionPrefixes = String.format( prefixes, metaModelVersion.toVersionString(), metaModelVersion.toVersionString() );
215-
final String queryString = String.format(
216-
"%s select ?reachableProperty ?viaEither"
217-
+ " where {"
218-
+ " {"
219-
+ " ?currentProperty samm:characteristic/samm-c:baseCharacteristic*/samm-c:left/samm:dataType/samm:properties/rdf:rest*/rdf:first ?reachableProperty"
220-
+ " bind (1 as ?viaEither)"
221-
+ " }"
222-
+ " union"
223-
+ " {"
224-
+ " ?currentProperty samm:characteristic/samm-c:baseCharacteristic*/samm-c:right/samm:dataType/samm:properties/rdf:rest*/rdf:first ?reachableProperty"
225-
+ " bind (2 as ?viaEither)"
226-
+ " }"
227-
+ " union"
228-
+ " {"
229-
+ " ?currentProperty samm:characteristic/samm-c:baseCharacteristic*/samm:dataType/samm:properties/rdf:rest*/rdf:first ?reachableProperty"
230-
+ " bind (0 as ?viaEither)"
231-
+ " }"
232-
+ "}",
225+
final String queryString = String.format( """
226+
%s select ?reachableProperty ?viaEither
227+
where {
228+
{
229+
?currentProperty samm:characteristic/samm-c:baseCharacteristic*/samm-c:left/samm:dataType/samm:properties/rdf:rest*/rdf:first ?reachableProperty
230+
bind (1 as ?viaEither)
231+
}
232+
union
233+
{
234+
?currentProperty samm:characteristic/samm-c:baseCharacteristic*/samm-c:right/samm:dataType/samm:properties/rdf:rest*/rdf:first ?reachableProperty
235+
bind (2 as ?viaEither)
236+
}
237+
union
238+
{
239+
?currentProperty samm:characteristic/samm-c:baseCharacteristic*/samm:dataType/samm:properties/rdf:rest*/rdf:first ?reachableProperty
240+
bind (0 as ?viaEither)
241+
}
242+
}
243+
""",
233244
currentVersionPrefixes );
234245
query = QueryFactory.create( queryString );
235246
}

‎core/esmf-aspect-model-validator/src/test/java/org/eclipse/esmf/aspectmodel/validation/services/AspectModelValidatorTest.java‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,7 @@
1313

1414
package org.eclipse.esmf.aspectmodel.validation.services;
1515

16-
import static org.assertj.core.api.Assertions.assertThat;
17-
import static org.assertj.core.api.Assertions.assertThatCode;
16+
import static org.assertj.core.api.Assertions.*;
1817

1918
import java.util.ArrayList;
2019
import java.util.Arrays;
@@ -35,19 +34,18 @@
3534
import org.eclipse.esmf.aspectmodel.shacl.violation.SparqlConstraintViolation;
3635
import org.eclipse.esmf.aspectmodel.shacl.violation.Violation;
3736
import org.eclipse.esmf.aspectmodel.vocabulary.SAMM;
38-
import org.junit.jupiter.params.ParameterizedTest;
39-
import org.junit.jupiter.params.provider.Arguments;
40-
import org.junit.jupiter.params.provider.EnumSource;
41-
import org.junit.jupiter.params.provider.MethodSource;
42-
4337
import org.eclipse.esmf.samm.KnownVersion;
44-
4538
import org.eclipse.esmf.test.InvalidTestAspect;
4639
import org.eclipse.esmf.test.MetaModelVersions;
4740
import org.eclipse.esmf.test.TestAspect;
4841
import org.eclipse.esmf.test.TestModel;
4942
import org.eclipse.esmf.test.TestProperty;
5043
import org.eclipse.esmf.test.TestResources;
44+
import org.junit.jupiter.params.ParameterizedTest;
45+
import org.junit.jupiter.params.provider.Arguments;
46+
import org.junit.jupiter.params.provider.EnumSource;
47+
import org.junit.jupiter.params.provider.MethodSource;
48+
5149
import io.vavr.control.Try;
5250

5351
public class AspectModelValidatorTest extends MetaModelVersions {
@@ -261,15 +259,16 @@ public void testValidationWithMultipleAspects( final KnownVersion metaModelVersi
261259
void testCycleDetection( final KnownVersion metaModelVersion ) {
262260
final Try<VersionedModel> versionedModel = TestResources.getModel( TestAspect.MODEL_WITH_CYCLES, metaModelVersion );
263261
final List<Violation> report = service.get( metaModelVersion ).validateModel( versionedModel );
264-
assertThat( report.size() ).isEqualTo( 6 );
262+
assertThat( report.size() ).isEqualTo( 7 );
265263
assertThat( report ).containsAll( cycles(
266264
":a -> :b -> :a",
267265
":e -> :f -> :g -> :e",
268266
":h -> :h",
269267
":h -> :i -> :h",
270268
":l -> :l",
271269
// TimeSeries are handled differently between v1 and v2 meta models.
272-
metaModelVersion.isOlderThan( KnownVersion.SAMM_2_0_0 ) ? ":n -> :refinedValue -> :n" : ":n -> :NTimeSeriesEntity|samm-e:value -> :n" ) );
270+
metaModelVersion.isOlderThan( KnownVersion.SAMM_2_0_0 ) ? ":n -> :refinedValue -> :n" : ":n -> :NTimeSeriesEntity|samm-e:value -> :n",
271+
":p -> :q -> :r -> :q" ) );
273272
}
274273

275274
@ParameterizedTest

‎core/esmf-test-aspect-models/src/main/resources/valid/samm_1_0_0/org.eclipse.esmf.test/1.0.0/ModelWithCycles.ttl‎

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717

1818
:ModelWithCycles a samm:Aspect ;
1919
samm:name "ModelWithCycles" ;
20-
samm:properties ( :a :e :h :l :n ) ;
20+
samm:properties ( :a :e :h :l :n [ samm:property :p ; samm:optional true ; ] ) ;
2121
samm:operations ( ) .
2222

2323
#####################################
@@ -173,9 +173,48 @@
173173
samm:dataType :OEntity.
174174

175175
:OEntity a samm:Entity;
176-
samm:name "MEntity" ;
176+
samm:name "OEntity" ;
177177
samm:properties ( :n ) .
178178

179+
######################################################
180+
# optional property *before* a cycle does not break it
181+
######################################################
182+
:p a samm:Property ;
183+
samm:name "p" ;
184+
samm:characteristic :pCharacteristic .
185+
186+
:pCharacteristic a samm:Characteristic;
187+
samm:name "pCharacteristic" ;
188+
samm:dataType :PEntity.
189+
190+
:PEntity a samm:Entity;
191+
samm:name "PEntity" ;
192+
samm:properties ( :q ) .
193+
194+
:q a samm:Property ;
195+
samm:name "q" ;
196+
samm:characteristic :qCharacteristic .
197+
198+
:qCharacteristic a samm:Characteristic;
199+
samm:name "qCharacteristic" ;
200+
samm:dataType :QEntity.
201+
202+
:QEntity a samm:Entity;
203+
samm:name "QEntity" ;
204+
samm:properties ( :r ) .
205+
206+
:r a samm:Property ;
207+
samm:name "rEntity" ;
208+
samm:characteristic :rCharacteristic .
209+
210+
:rCharacteristic a samm:Characteristic;
211+
samm:name "rCharacteristic" ;
212+
samm:dataType :REntity.
213+
214+
:REntity a samm:Entity;
215+
samm:name "REntity" ;
216+
samm:properties ( :q ) .
217+
179218

180219

181220

‎core/esmf-test-aspect-models/src/main/resources/valid/samm_2_0_0/org.eclipse.esmf.test/1.0.0/ModelWithCycles.ttl‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
@prefix xsd: <http://www.w3.org/2001/XMLSchema#> .
1717

1818
:ModelWithCycles a samm:Aspect ;
19-
samm:properties ( :a :e :h :l :n ) .
19+
samm:properties ( :a :e :h :l :n [ samm:property :p ; samm:optional true ; ] ) .
2020

2121
#####################################
2222
# direct cycle between two properties
@@ -138,8 +138,32 @@
138138
:OEntity a samm:Entity;
139139
samm:properties ( :n ) .
140140

141+
######################################################
142+
# optional property *before* a cycle does not break it
143+
######################################################
144+
:p a samm:Property ;
145+
samm:characteristic :pCharacteristic .
141146

147+
:pCharacteristic a samm:Characteristic;
148+
samm:dataType :PEntity.
142149

150+
:PEntity a samm:Entity;
151+
samm:properties ( :q ) .
143152

153+
:q a samm:Property ;
154+
samm:characteristic :qCharacteristic .
144155

156+
:qCharacteristic a samm:Characteristic;
157+
samm:dataType :QEntity.
145158

159+
:QEntity a samm:Entity;
160+
samm:properties ( :r ) .
161+
162+
:r a samm:Property ;
163+
samm:characteristic :rCharacteristic .
164+
165+
:rCharacteristic a samm:Characteristic;
166+
samm:dataType :REntity.
167+
168+
:REntity a samm:Entity;
169+
samm:properties ( :q ) .

0 commit comments

Comments
 (0)