Repository: NeurodataWithoutBorders/pynwb

Issue:
NWBNamespaceBuilder does not validate spec it is building/writing.
NWBNamespaceBuilder allows construction of specs that can't be loaded due to redefining types already defined in the spec. The problem isn't discovered until load time.

Example:

    from pynwb.spec import NWBGroupSpec, NWBNamespaceBuilder

    spec1 = NWBGroupSpec("This is my new group 1",
                         "Group1",
                         neurodata_type_inc="NWBDataInterface",
                         neurodata_type_def="Group1")

    spec2 = NWBGroupSpec("This is my new group 2",
                         "Group2",
                         groups=[spec1],  # oops, this will look like it writes a valid spec, but fails on read
                         neurodata_type_inc="NWBDataInterface",
                         neurodata_type_def="Group2")

    ext_source = "myext.yaml"
    ns_builder = NWBNamespaceBuilder("Example namespace", "example")

    ns_builder.add_spec(ext_source, spec1)
    ns_builder.add_spec(ext_source, spec2)

    ns_path = "example.namespace.yaml"
    ns_builder.export(ns_path)

Fails on load:

    from pynwb import load_namespaces
    namespace_path = 'example.namespace.yaml'
    load_namespaces(namespace_path)

Tested in python 3.6 in ubuntu using latest dev commit.

- [X] Have you ensured the feature or change was not already [reported](https://github.com/NeurodataWithoutBorders/pynwb/issues/) ?
- [X] Have you included a brief and descriptive title?
- [X] Have you included a clear description of the problem you are trying to solve?
- [X] Have you included a minimal code snippet that reproduces the issue you are encountering?


Discussion:

Patch:
diff --git a/src/pynwb/form/spec/namespace.py b/src/pynwb/form/spec/namespace.py
index b152adef..bf7388ad 100644
--- a/src/pynwb/form/spec/namespace.py
+++ b/src/pynwb/form/spec/namespace.py
@@ -337,6 +337,40 @@ class NamespaceCatalog(object):
         for subspec_dict in it:
             self.__resolve_includes(subspec_dict, catalog)
 
+    def __load_namespace(self, namespace, reader, types_key, resolve=True):
+        ns_name = namespace['name']
+        if ns_name in self.__namespaces:
+            raise KeyError("namespace '%s' already exists" % ns_name)
+        catalog = SpecCatalog()
+        included_types = dict()
+        for s in namespace['schema']:
+            if 'source' in s:
+                # read specs from file
+                dtypes = None
+                if types_key in s:
+                    dtypes = set(s[types_key])
+                self.__load_spec_file(reader, s['source'], catalog, dtypes=dtypes, resolve=resolve)
+                self.__included_sources.setdefault(ns_name, list()).append(s['source'])
+            elif 'namespace' in s:
+                # load specs from namespace
+                try:
+                    inc_ns = self.get_namespace(s['namespace'])
+                except KeyError:
+                    raise ValueError("Could not load namespace '%s'" % s['namespace'])
+                if types_key in s:
+                    types = s[types_key]
+                else:
+                    types = inc_ns.get_registered_types()
+                for ndt in types:
+                    spec = inc_ns.get_spec(ndt)
+                    spec_file = inc_ns.catalog.get_spec_source_file(ndt)
+                    catalog.register_spec(spec, spec_file)
+                included_types[s['namespace']] = tuple(types)
+        # construct namespace
+        self.add_namespace(ns_name,
+                           self.__spec_namespace_cls.build_namespace(catalog=catalog, **namespace))
+        return included_types
+
     @docval({'name': 'namespace_path', 'type': str, 'doc': 'the path to the file containing the namespaces(s) to load'},
             {'name': 'resolve',
              'type': bool,
@@ -361,35 +395,11 @@ class NamespaceCatalog(object):
             return ret
         namespaces = reader.read_namespace(namespace_path)
         types_key = self.__spec_namespace_cls.types_key()
+        for ns in namespaces:
+            if ns['name'] in self.__namespaces:
+                raise KeyError("namespace '%s' already exists" % ns['name'])
         # now load specs into namespace
         for ns in namespaces:
-            catalog = SpecCatalog()
-            included_types = dict()
-            for s in ns['schema']:
-                if 'source' in s:
-                    # read specs from file
-                    dtypes = None
-                    if types_key in s:
-                        dtypes = set(s[types_key])
-                    self.__load_spec_file(reader, s['source'], catalog, dtypes=dtypes, resolve=resolve)
-                    self.__included_sources.setdefault(ns['name'], list()).append(s['source'])
-                elif 'namespace' in s:
-                    # load specs from namespace
-                    try:
-                        inc_ns = self.get_namespace(s['namespace'])
-                    except KeyError:
-                        raise ValueError("Could not load namespace '%s'" % s['namespace'])
-                    if types_key in s:
-                        types = s[types_key]
-                    else:
-                        types = inc_ns.get_registered_types()
-                    for ndt in types:
-                        spec = inc_ns.get_spec(ndt)
-                        spec_file = inc_ns.catalog.get_spec_source_file(ndt)
-                        catalog.register_spec(spec, spec_file)
-                    included_types[s['namespace']] = tuple(types)
-            ret[ns['name']] = included_types
-            # construct namespace
-            self.add_namespace(ns['name'], self.__spec_namespace_cls.build_namespace(catalog=catalog, **ns))
+            ret[ns['name']] = self.__load_namespace(ns, reader, types_key, resolve=resolve)
         self.__included_specs[namespace_path] = ret
         return ret
diff --git a/src/pynwb/form/spec/write.py b/src/pynwb/form/spec/write.py
index 850a697b..d477c282 100644
--- a/src/pynwb/form/spec/write.py
+++ b/src/pynwb/form/spec/write.py
@@ -8,6 +8,7 @@ from abc import ABCMeta, abstractmethod
 
 from .namespace import SpecNamespace
 from .spec import GroupSpec, DatasetSpec
+from .catalog import SpecCatalog
 
 from ..utils import docval, getargs, popargs
 
@@ -59,6 +60,7 @@ class NamespaceBuilder(object):
         self.__ns_args = copy.deepcopy(kwargs)
         self.__namespaces = OrderedDict()
         self.__sources = OrderedDict()
+        self.__catalog = SpecCatalog()
         self.__dt_key = ns_cls.types_key()
 
     @docval({'name': 'source', 'type': str, 'doc': 'the path to write the spec to'},
@@ -66,6 +68,7 @@ class NamespaceBuilder(object):
     def add_spec(self, **kwargs):
         ''' Add a Spec to the namespace '''
         source, spec = getargs('source', 'spec', kwargs)
+        self.__catalog.auto_register(spec, source)
         self.add_source(source)
         self.__sources[source].setdefault(self.__dt_key, list()).append(spec)
 


Test patch:
diff --git a/tests/unit/pynwb_tests/test_extension.py b/tests/unit/pynwb_tests/test_extension.py
index 0e6e72ee..b425db55 100644
--- a/tests/unit/pynwb_tests/test_extension.py
+++ b/tests/unit/pynwb_tests/test_extension.py
@@ -72,3 +72,28 @@ class TestCatchDupNS(unittest.TestCase):
         load_namespaces(os.path.join(self.tempdir, self.ns_path1))
         with self.assertRaises(KeyError):
             load_namespaces(os.path.join(self.tempdir, self.ns_path2))
+
+
+class TestCatchDuplicateSpec(unittest.TestCase):
+
+    def setUp(self):
+        self.ext_source = 'fake_extension3.yaml'
+
+    def tearDown(self):
+        pass
+
+    def test_catch_duplicate_spec(self):
+        spec1 = NWBGroupSpec("This is my new group 1",
+                             "Group1",
+                             neurodata_type_inc="NWBDataInterface",
+                             neurodata_type_def="Group1")
+        spec2 = NWBGroupSpec("This is my new group 2",
+                             "Group2",
+                             groups=[spec1],
+                             neurodata_type_inc="NWBDataInterface",
+                             neurodata_type_def="Group2")
+        ns_builder = NWBNamespaceBuilder("Example namespace",
+                                         "pynwb_test_ext")
+        ns_builder.add_spec(self.ext_source, spec1)
+        with self.assertRaises(ValueError):
+            ns_builder.add_spec(self.ext_source, spec2)