From 739c711ee345c85698df517d2382be91858b9911 Mon Sep 17 00:00:00 2001
From: Ben Webb <ben@salilab.org>
Date: Fri, 18 Oct 2024 16:23:32 -0700
Subject: [PATCH] Handle null features in dumper

If a Feature base object is encountered, rather
than raising an obscure exception, report it.
This can happen if an input file references a
feature by ID but does not define it anywhere.
If checks are disabled, output the feature in
ihm_feature_list with a type of "?".
---
 ihm/dumper.py          |  2 ++
 ihm/restraint.py       |  7 ++++++-
 test/test_dumper.py    | 23 +++++++++++++++++++++++
 test/test_restraint.py |  2 ++
 4 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/ihm/dumper.py b/ihm/dumper.py
index 9a9fa71..9f5ccc7 100644
--- a/ihm/dumper.py
+++ b/ihm/dumper.py
@@ -2176,6 +2176,8 @@ def dump_list(self, writer):
                          ["feature_id", "feature_type", "entity_type",
                           "details"]) as lp:
             for f in self._features_by_id:
+                if self._check and f.type is ihm.unknown:
+                    raise ValueError("Invalid null feature %s" % f)
                 lp.write(feature_id=f._id, feature_type=f.type,
                          entity_type=f._get_entity_type(),
                          details=f.details)
diff --git a/ihm/restraint.py b/ihm/restraint.py
index 9aa4c64..a4e0986 100644
--- a/ihm/restraint.py
+++ b/ihm/restraint.py
@@ -574,7 +574,8 @@ def __init__(self, psi=None, sigma1=None, sigma2=None):
 
 class Feature(object):
     """Base class for selecting parts of the system that a restraint acts on.
-       See :class:`ResidueFeature`, :class:`AtomFeature`,
+       This class should not be used itself; instead,
+       see :class:`ResidueFeature`, :class:`AtomFeature`,
        :class:`NonPolyFeature`, and :class:`PseudoSiteFeature`.
 
        Features are typically assigned to one or more
@@ -582,11 +583,15 @@ class Feature(object):
        :class:`~ihm.restraint.DerivedDistanceRestraint` objects.
     """
     details = None
+    type = ihm.unknown
 
     def _all_entities_or_asyms(self):
         # Get all Entities or AsymUnits referenced by this object
         return []
 
+    def _get_entity_type(self):
+        return ihm.unknown
+
 
 class ResidueFeature(Feature):
     """Selection of one or more residues from the system.
diff --git a/test/test_dumper.py b/test/test_dumper.py
index fafcce5..b794af0 100644
--- a/test/test_dumper.py
+++ b/test/test_dumper.py
@@ -3712,6 +3712,29 @@ def test_feature_dumper_no_residues(self):
         self.assertEqual(len(dumper._features_by_id), 1)
         self.assertRaises(ValueError, _get_dumper_output, dumper, system)
 
+    def test_feature_dumper_base_class(self):
+        """Test FeatureDumper with a Feature base class"""
+        system = ihm.System()
+
+        f = ihm.restraint.Feature()
+        system.orphan_features.append(f)
+
+        dumper = ihm.dumper._FeatureDumper()
+        dumper.finalize(system)  # assign IDs
+        self.assertEqual(len(dumper._features_by_id), 1)
+        self.assertRaises(ValueError, _get_dumper_output, dumper, system)
+        # Should be OK if checks are disabled
+        out = _get_dumper_output(dumper, system, check=False)
+        self.assertEqual(out, """#
+loop_
+_ihm_feature_list.feature_id
+_ihm_feature_list.feature_type
+_ihm_feature_list.entity_type
+_ihm_feature_list.details
+1 ? ? .
+#
+""")
+
     def test_pseudo_site_dumper(self):
         """Test PseudoSiteDumper"""
         system = ihm.System()
diff --git a/test/test_restraint.py b/test/test_restraint.py
index e3adb62..ce18b02 100644
--- a/test/test_restraint.py
+++ b/test/test_restraint.py
@@ -173,6 +173,8 @@ def test_feature(self):
         """Test Feature base class"""
         f = ihm.restraint.Feature()  # does nothing
         self.assertEqual(f._all_entities_or_asyms(), [])
+        self.assertIs(f.type, ihm.unknown)
+        self.assertIs(f._get_entity_type(), ihm.unknown)
 
     def test_residue_feature(self):
         """Test ResidueFeature class"""