From 565be812dbe4a01fc06e9e8696d49c366031e7ae Mon Sep 17 00:00:00 2001 From: Alex Skrenchuk Date: Mon, 3 Aug 2026 23:13:42 -0700 Subject: [PATCH 1/2] Fix nil crash in paths_to_root at parentless ancestors traverse_path_to_root gated its empty-parents guard on the `tree` flag, so it only protected the tree/path_to_root caller. The /classes/:cls/paths_to_root endpoint calls paths_to_root with tree=false and no roots, so the traversal recognizes only owl:Thing as a stopping point (append_if_not_there_already, tree_root?). When the ancestor chain reaches a class with no parents under the submission's tree property, goo loads `parents` as [] and marks it loaded (solutions_mapper init_unloaded_attributes), the recursion is entered with an empty array, and parents[0] is nil: NoMethodError: undefined method `id' for nil:NilClass class.rb:701:in `append_if_not_there_already' class.rb:725:in `traverse_path_to_root' concept_tree.rb:64:in `paths_to_root' OWL submissions mostly escape this because owlapi asserts `rdfs:subClassOf owl:Thing` on top-level classes (12 such triples in a BRO_v3.2 parse, none present in the source file) and owl:Thing is the sentinel the traversal already handles. SKOS (skos:broader) and OBO (metadata:treeView) have no equivalent sentinel, so every class with a parent is exposed. Reported in production for NDFRT, RCD and NCBITAXON. Root classes were unaffected because paths_to_root returns [] before traversing, and /tree was unaffected because path_to_root passes tree=true. - make the terminal guard unconditional; a parentless ancestor ends the path - nil-safe .dup on p.parents - the cannot-load-parents fail-safe used `return`, aborting the remaining recursions.each_index iterations and silently truncating the sibling paths built for a multi-parent class; use `next` so only the failing path stops Adds two regression tests, both verified to reproduce the production trace before the change: SKOS via the efo_gwas.skos.owl fixture, and OBO via hp.obo for the metadata:treeView tree property. --- lib/ontologies_linked_data/models/class.rb | 16 ++++++-- test/models/test_class.rb | 48 ++++++++++++++++++++++ test/models/test_skos_submission.rb | 34 +++++++++++++++ 3 files changed, 94 insertions(+), 4 deletions(-) diff --git a/lib/ontologies_linked_data/models/class.rb b/lib/ontologies_linked_data/models/class.rb index 7dec5fad..27e886d1 100644 --- a/lib/ontologies_linked_data/models/class.rb +++ b/lib/ontologies_linked_data/models/class.rb @@ -704,7 +704,14 @@ def append_if_not_there_already(path, r) end def traverse_path_to_root(parents, paths, path_i, tree = false, roots = nil) - return if (tree && parents.length == 0) + # Terminal condition: a class with no parents under the submission's tree + # property is the top of this path, so there is nothing left to append. + # Callers that pass `roots` (path_to_root / #tree) normally stop earlier in + # tree_root? below; callers that do not (the paths_to_root endpoint) rely on + # this guard, because the only other sentinel here is owl:Thing -- which + # owlapi asserts on top-level OWL classes but which never appears under + # skos:broader (SKOS) or metadata:treeView (OBO). + return if parents.nil? || parents.empty? recursions = [path_i] recurse_on_path = [false] @@ -737,13 +744,14 @@ def traverse_path_to_root(parents, paths, path_i, tree = false, roots = nil) end if !p.loaded_attributes.include?(:parents) - # fail safely + # fail safely -- abandon this path only, the sibling paths in this + # frame are unaffected and must still be traversed logger = LinkedData::Parser.logger || Logger.new($stderr) logger.error("Class #{p.id.to_s} from #{p.submission.id} cannot load parents") - return + next end - traverse_path_to_root(p.parents.dup, paths, rec_i, tree=tree, roots=roots) + traverse_path_to_root((p.parents || []).dup, paths, rec_i, tree=tree, roots=roots) end end end diff --git a/test/models/test_class.rb b/test/models/test_class.rb index 8a3e4129..fd611fd6 100644 --- a/test/models/test_class.rb +++ b/test/models/test_class.rb @@ -454,4 +454,52 @@ def test_xml_literal_serialization assert_equal String, xml_comment.class assert_equal comment, xml_comment end + + # Companion to test_bro_paths_to_root, for the OBO tree property. BRO cannot + # cover this: owlapi asserts `rdfs:subClassOf owl:Thing` on top-level OWL + # classes, and the traversal treats owl:Thing as a stop sentinel. OBO + # submissions walk metadata:treeView, where no such sentinel exists, so the + # traversal has to terminate on a genuinely parentless ancestor instead. + # Regressing that recurses into an empty parents array and raises + # "undefined method `id' for nil:NilClass" in append_if_not_there_already. + def test_obo_paths_to_root_terminates_at_parentless_class + if !LinkedData::Models::Ontology.find("HPPATHSTEST").first + submission_parse("HPPATHSTEST", "HP paths_to_root test", "./test/data/ontology_files/hp.obo", 124, + process_rdf: true, index_search: false, + run_metrics: false) + end + os = LinkedData::Models::Ontology.find("HPPATHSTEST").first.latest_submission(status: [:rdf]) + + assert_equal 'http://data.bioontology.org/metadata/treeView', + LinkedData::Models::Class.tree_view_property(os).to_s, + 'expected the OBO tree property, otherwise this duplicates the OWL test' + + # Find a class whose own parent has no parents -- one hop is enough to make + # the traversal walk into the parentless ancestor. + target = nil + LinkedData::Models::Class.in(os).include(parents: [:prefLabel]).page(1, 500).each do |cls| + next if cls.parents.nil? || cls.parents.empty? + + parent = cls.parents.first + parent.bring(:parents) if parent.bring?(:parents) + next unless parent.parents.nil? || parent.parents.empty? + + target = LinkedData::Models::Class.find(cls.id).in(os).first + break + end + refute_nil target, 'expected an OBO class whose parent is parentless' + + paths = target.paths_to_root + refute_empty paths, 'expected at least one path to root' + + paths.each do |path| + refute_includes path, nil, 'path contains a nil node' + assert_equal target.id.to_s, path.last.id.to_s, 'path must end at the requested class' + + root = path.first + root.bring(:parents) if root.bring?(:parents) + assert_empty(root.parents || [], + "path must terminate at a parentless class, got #{root.id}") + end + end end diff --git a/test/models/test_skos_submission.rb b/test/models/test_skos_submission.rb index 601c9565..578e3e53 100644 --- a/test/models/test_skos_submission.rb +++ b/test/models/test_skos_submission.rb @@ -217,6 +217,40 @@ def test_skos_class_tree assert seen_target, 'target class not found within its own tree' end + # Guards the terminal condition of the paths_to_root traversal. The traversal + # must stop when it reaches a class with no parents under the submission's tree + # property; it cannot rely on an owl:Thing sentinel, which never appears under + # skos:broader (or metadata:treeView for OBO), nor on `roots` -- the + # /classes/:cls/paths_to_root endpoint passes none. Regressing this recurses + # into an empty parents array and raises + # "undefined method `id' for nil:NilClass" in append_if_not_there_already. + def test_skos_paths_to_root_terminates_at_parentless_concept + sub = before_suite + roots = sub.roots + refute_empty roots, 'expected SKOS roots' + + # Descend one level from a root so the path is non-trivial (root -> target). + target = nil + roots.each do |r| + LinkedData::Models::Class.in(sub).models([r]).include(children: [:prefLabel]).all + next if r.children.empty? + + target = LinkedData::Models::Class.find(r.children.first.id).in(sub).first + break + end + refute_nil target, 'expected a SKOS root with at least one child' + + paths = target.paths_to_root + refute_empty paths, 'expected at least one path to root' + + root_ids = roots.map { |r| r.id.to_s } + paths.each do |path| + refute_includes path, nil, 'path contains a nil node' + assert_equal target.id.to_s, path.last.id.to_s, 'path must end at the requested class' + assert_includes root_ids, path.first.id.to_s, 'path must start at a submission root' + end + end + # N+1 guard for the class-tree endpoint (SKOS). Same contract as the OWL guard # in test_class.rb: serializing a tree must resolve the submission's languages # once, not once per node (get_languages `bring?` guard, From e8dd8c1526ca8d841ce89a9dc4490ca1e17e7af6 Mon Sep 17 00:00:00 2001 From: Alex Skrenchuk Date: Tue, 4 Aug 2026 01:28:45 -0700 Subject: [PATCH 2/2] Make paths_to_root regression tests backend-independent Both new tests picked their target class by iterating query results and taking the first match, so the chosen class differed per backend -- row order is not guaranteed and 4store, AllegroGraph, GraphDB and Virtuoso each returned a different one. On AllegroGraph the OBO test then failed for two reasons: - the discovery loop treated owl:Thing as a "parentless parent", so it selected a class whose only qualifying parent was the sentinel -- a path that never reaches the empty-parents branch the test is meant to cover - the terminal assertion required path.first to have no parents at all, but a path may legitimately stop on the owl:Thing sentinel, which owlapi asserts on top-level classes - sort candidate ids and pick the lowest, so every backend tests the same class - skip owl:Thing when looking for a parentless parent - assert the terminal node has no *traversable* parents (parents minus owl:Thing), plus a separate assertion that at least one path terminates at a class with no parents at all -- the case that used to raise Verified on all four backends: test_class.rb, test_skos_submission.rb and test_provisional_class.rb pass on 4store, AllegroGraph, GraphDB and Virtuoso, and both tests still reproduce the NoMethodError against the pre-fix traversal. --- test/models/test_class.rb | 41 +++++++++++++++++++++-------- test/models/test_skos_submission.rb | 32 +++++++++++++++------- 2 files changed, 53 insertions(+), 20 deletions(-) diff --git a/test/models/test_class.rb b/test/models/test_class.rb index fd611fd6..3efbe210 100644 --- a/test/models/test_class.rb +++ b/test/models/test_class.rb @@ -474,32 +474,51 @@ def test_obo_paths_to_root_terminates_at_parentless_class LinkedData::Models::Class.tree_view_property(os).to_s, 'expected the OBO tree property, otherwise this duplicates the OWL test' - # Find a class whose own parent has no parents -- one hop is enough to make - # the traversal walk into the parentless ancestor. - target = nil + # Find a class with a parent that itself has no parents -- one hop is enough + # to make the traversal walk into the parentless ancestor. Candidates are + # sorted so the same class is picked on every backend (row order is not + # guaranteed and differs between 4store/AG/GraphDB/Virtuoso). + candidates = [] LinkedData::Models::Class.in(os).include(parents: [:prefLabel]).page(1, 500).each do |cls| next if cls.parents.nil? || cls.parents.empty? - parent = cls.parents.first - parent.bring(:parents) if parent.bring?(:parents) - next unless parent.parents.nil? || parent.parents.empty? + cls.parents.each do |parent| + # owl:Thing has no parents either, but the traversal stops on it via the + # sentinel, so it does not exercise the empty-parents branch. + next if parent.id.to_s['#Thing'] - target = LinkedData::Models::Class.find(cls.id).in(os).first - break + parent.bring(:parents) if parent.bring?(:parents) + next unless parent.parents.nil? || parent.parents.empty? + + candidates << cls.id.to_s + break + end end - refute_nil target, 'expected an OBO class whose parent is parentless' + target_id = candidates.sort.first + refute_nil target_id, 'expected an OBO class whose parent is parentless' + target = LinkedData::Models::Class.find(RDF::URI.new(target_id)).in(os).first paths = target.paths_to_root refute_empty paths, 'expected at least one path to root' + saw_parentless_terminal = false paths.each do |path| refute_includes path, nil, 'path contains a nil node' assert_equal target.id.to_s, path.last.id.to_s, 'path must end at the requested class' root = path.first root.bring(:parents) if root.bring?(:parents) - assert_empty(root.parents || [], - "path must terminate at a parentless class, got #{root.id}") + root_parents = root.parents || [] + saw_parentless_terminal ||= root_parents.empty? + + # A path may also stop on the owl:Thing sentinel, which owlapi asserts on + # top-level classes; what must hold is that nothing traversable is left. + assert_empty root_parents.reject { |p| p.id.to_s['#Thing'] }, + "path must terminate where no parent is left to follow, got #{root.id}" end + + assert saw_parentless_terminal, + 'expected at least one path to terminate at a class with no parents at all -- ' \ + 'that is the case that used to raise' end end diff --git a/test/models/test_skos_submission.rb b/test/models/test_skos_submission.rb index 578e3e53..3b676c94 100644 --- a/test/models/test_skos_submission.rb +++ b/test/models/test_skos_submission.rb @@ -230,25 +230,39 @@ def test_skos_paths_to_root_terminates_at_parentless_concept refute_empty roots, 'expected SKOS roots' # Descend one level from a root so the path is non-trivial (root -> target). - target = nil - roots.each do |r| + # Ids are sorted so the same concept is picked on every backend -- row order + # is not guaranteed and differs between 4store/AG/GraphDB/Virtuoso. + child_ids = [] + roots.sort_by { |r| r.id.to_s }.each do |r| LinkedData::Models::Class.in(sub).models([r]).include(children: [:prefLabel]).all - next if r.children.empty? - - target = LinkedData::Models::Class.find(r.children.first.id).in(sub).first - break + child_ids += r.children.map { |c| c.id.to_s } end - refute_nil target, 'expected a SKOS root with at least one child' + target_id = child_ids.sort.first + refute_nil target_id, 'expected a SKOS root with at least one child' + target = LinkedData::Models::Class.find(RDF::URI.new(target_id)).in(sub).first paths = target.paths_to_root refute_empty paths, 'expected at least one path to root' - root_ids = roots.map { |r| r.id.to_s } + saw_parentless_terminal = false paths.each do |path| refute_includes path, nil, 'path contains a nil node' assert_equal target.id.to_s, path.last.id.to_s, 'path must end at the requested class' - assert_includes root_ids, path.first.id.to_s, 'path must start at a submission root' + + root = path.first + root.bring(:parents) if root.bring?(:parents) + root_parents = root.parents || [] + saw_parentless_terminal ||= root_parents.empty? + + # skos:broader has no owl:Thing sentinel, so the only way to stop is to run + # out of parents; the reject keeps this symmetric with the OBO guard. + assert_empty root_parents.reject { |p| p.id.to_s['#Thing'] }, + "path must terminate where no parent is left to follow, got #{root.id}" end + + assert saw_parentless_terminal, + 'expected at least one path to terminate at a concept with no broader -- ' \ + 'that is the case that used to raise' end # N+1 guard for the class-tree endpoint (SKOS). Same contract as the OWL guard