diff --git a/CHANGELOG.md b/CHANGELOG.md index 31b437b0d..279d07adf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,8 +6,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] + +### Changed - Sort [`report`] violations by rule name within level [#955] +### Fixed +- Fix subClassOf cycles in related object selection [#979] + ## [1.8.3] - 2021-12-16 ### Changed @@ -294,6 +299,7 @@ First official release of ROBOT! [`template`]: http://robot.obolibrary.org/template [`validate`]: http://robot.obolibrary.org/validate +[#979]: https://github.com/ontodev/robot/pull/979 [#955]: https://github.com/ontodev/robot/pull/955 [#953]: https://github.com/ontodev/robot/pull/953 [#951]: https://github.com/ontodev/robot/pull/951 diff --git a/robot-core/src/main/java/org/obolibrary/robot/RelatedObjectsHelper.java b/robot-core/src/main/java/org/obolibrary/robot/RelatedObjectsHelper.java index 28650abf5..cbed28d43 100644 --- a/robot-core/src/main/java/org/obolibrary/robot/RelatedObjectsHelper.java +++ b/robot-core/src/main/java/org/obolibrary/robot/RelatedObjectsHelper.java @@ -2032,6 +2032,10 @@ private static void selectClassAncestors( for (OWLClassExpression classExpression : EntitySearcher.getSuperClasses(cls, ontology)) { if (!classExpression.isAnonymous()) { OWLClass superClass = classExpression.asOWLClass(); + if (ancestors.contains(superClass)) { + // Subclass of/equivalent class can cause a loop - see #976 + continue; + } ancestors.add(superClass); if (!superClass.isTopEntity()) { selectClassAncestors(ontology, superClass, ancestors); @@ -2055,6 +2059,10 @@ private static void selectClassDescendants( for (OWLClassExpression classExpression : EntitySearcher.getSubClasses(cls, ontology)) { if (!classExpression.isAnonymous()) { OWLClass subClass = classExpression.asOWLClass(); + if (descendants.contains(subClass)) { + // Subclass of/equivalent class can cause a loop - see #976 + continue; + } descendants.add(subClass); if (!EntitySearcher.getSubClasses(subClass, ontology).isEmpty()) { selectClassDescendants(ontology, subClass, descendants); diff --git a/robot-core/src/test/java/org/obolibrary/robot/RelatedObjectsHelperTest.java b/robot-core/src/test/java/org/obolibrary/robot/RelatedObjectsHelperTest.java index 5cc46afc8..e193ea636 100644 --- a/robot-core/src/test/java/org/obolibrary/robot/RelatedObjectsHelperTest.java +++ b/robot-core/src/test/java/org/obolibrary/robot/RelatedObjectsHelperTest.java @@ -4,6 +4,8 @@ import com.google.common.collect.Sets; import java.io.IOException; +import java.util.Arrays; +import java.util.Collections; import java.util.HashSet; import java.util.Set; import org.junit.Test; @@ -151,6 +153,29 @@ public void testGetAnnotationByRegex() throws Exception { assertTrue(annotations.contains(annotation2)); } + /** + * Test selecting ancestors and descendants with a subClassOf cycle. + * + * @throws Exception on any problem + */ + @Test + public void testSelectCycle() throws Exception { + IOHelper ioHelper = new IOHelper(); + OWLOntology patoRole = loadOntology("/pato_role.owl"); + + // Use BFO role as our starting object - then get all ancestors and descendants + Set objects = + new HashSet<>( + Collections.singletonList( + df.getOWLClass(IRI.create("http://purl.obolibrary.org/obo/BFO_0000023")))); + Set relatedObjects = + RelatedObjectsHelper.select( + patoRole, ioHelper, objects, Arrays.asList("ancestors", "descendants")); + + // Check that the select returned all classes in this ontology (all descendants of role) + assert relatedObjects.size() == patoRole.getClassesInSignature().size(); + } + /** @throws Exception */ private OWLOntology getOntology() throws Exception { OWLOntology ontology = manager.createOntology(); diff --git a/robot-core/src/test/resources/pato_role.owl b/robot-core/src/test/resources/pato_role.owl new file mode 100644 index 000000000..57449c0f3 --- /dev/null +++ b/robot-core/src/test/resources/pato_role.owl @@ -0,0 +1,733 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +