588 Add check on file export to ensure all required properties are fi…

…lled out (#600)

* Update metaclass to instrospect required properties

* Minor fixes to MetaClass wrt checking required properties

* Display classname in warning for missing required properties

* Add custom constraint checks (Issue #588 )

* Update multipleConstrainedTest, add required dataset in type

* Update linkTest to pass required property check

* Update anonTest to pass required property check

* Add explanations for custom constraints

* Add unittest to test changes made in this PR

* Hardcode exceptions for warnIfMissingRequiredProperties

+ Fix syntax error (missing end) in nwbExportTest

* Fix test for special case dataset starting_time in TimeSeries

* Change test workflow to upload test results also if tests fails

* Fix more issues with test workflow

+file/fillClass.m

@@ -129,6 +129,11 @@
         '%% VALIDATORS' validatorFcns...
         '%% EXPORT' exporterFcns}, newline);
 
+    customConstraintStr = file.fillCustomConstraint(name);
+    if ~isempty(customConstraintStr)
+        methodBody = strjoin({methodBody, '%% CUSTOM CONSTRAINTS', customConstraintStr}, newline);
+    end
+
     if strcmp(name, 'DynamicTable')
         methodBody = strjoin({methodBody, '%% TABLE METHODS', file.fillDynamicTableMethods()}, newline);
     end

+file/fillCustomConstraint.m

@@ -0,0 +1,38 @@
+function customConstraintStr = fillCustomConstraint(nwbType)
+% fillCustomConstraint - Create functions to check custom constraints 
+%   These are constraints that can not be inferred from the nwb-schema
+%
+%   Reference: https://github.com/NeurodataWithoutBorders/matnwb/issues/588
+
+    switch nwbType
+
+        case "TimeSeries"
+            % Add method to validate constraint that either timestamps or 
+            % starting_time must be present
+            customConstraintStr = sprintf( [...
+                'function checkCustomConstraint(obj)\n', ...
+                '    assert(~isempty(obj.timestamps) || ~isempty(obj.starting_time), ...\n', ...
+                '        "''timestamps'' or ''starting_time'' must be specified")\n', ...
+                '    if ~isempty(obj.starting_time)\n', ...
+                '        assert(~isempty(obj.starting_time_rate), ...\n', ...
+                '            "''starting_time_rate'' must be specified when ''starting_time'' is specified")\n', ...
+                '    end\n', ...
+                'end'] );
+
+
+        case "ImageSeries"
+            % If external_file is set, it does not make sense to fill out the
+            % data property. However, data is a required property, so this 
+            % method will add a nan-array to the data property so that it passes 
+            % the requirement check on file export.
+            customConstraintStr = sprintf( [...
+                'function checkCustomConstraint(obj)\n', ...
+                '    if ~isempty(obj.external_file) && isempty(obj.data), ...\n', ...
+                '        obj.data = nan(1,1,2);\n', ...
+                '    end\n', ...
+                'end'] );
+
+        otherwise
+            customConstraintStr = '';
+    end
+end

+tests/+unit/anonTest.m

@@ -17,7 +17,7 @@ function setup(testCase)
 end
 
 function testAnonDataset(testCase)
-ag = types.anon.AnonGroup('ad', types.anon.AnonData());
+ag = types.anon.AnonGroup('ad', types.anon.AnonData('data', 0));
 nwbExpected = NwbFile(...
     'identifier', 'ANON',...
     'session_description', 'anonymous class schema testing',...

+tests/+unit/linkTest.m

@@ -65,9 +65,11 @@ function testExternalResolution(testCase)
 expectedData = rand(100,1);
 stubDtr = types.hdmf_common.DynamicTableRegion(...
     'table', types.untyped.ObjectView('/acquisition/es1'),...
+    'data', 1, ...
     'description', 'dtr stub that points to electrical series illegally'); % do not do this at home.
 expected = types.core.ElectricalSeries('data', expectedData,...
     'data_unit', 'volts', ...
+    'timestamps', (1:100)', ...
     'electrodes', stubDtr);
 nwb.acquisition.set('es1', expected);
 nwb.export('test1.nwb');

+tests/+unit/multipleConstrainedTest.m

@@ -20,7 +20,7 @@ function testRoundabout(testCase)
     MultiSet = types.mcs.MultiSetContainer();
     MultiSet.something.set('A', types.mcs.ArbitraryTypeA());
     MultiSet.something.set('B', types.mcs.ArbitraryTypeB());
-    MultiSet.something.set('Data', types.mcs.DatasetType());
+    MultiSet.something.set('Data', types.mcs.DatasetType('data', ones(3,3)));
     nwbExpected = NwbFile(...
         'identifier', 'MCS', ...
         'session_description', 'multiple constrained schema testing', ...

+tests/+unit/nwbExportTest.m

@@ -31,6 +31,26 @@ function setupMethod(testCase)
     end
 
     methods (Test)
+        function testExportNwbFileWithMissingRequiredProperties(testCase)
+            nwb = NwbFile();
+            nwbFilePath = fullfile(testCase.OutputFolder, 'testfile.nwb');
+            testCase.verifyError(@(file, path) nwbExport(nwb, nwbFilePath), ...
+                'NWB:RequiredPropertyMissing')
+        end
+
+        function testExportTimeseriesWithMissingTimestampsAndStartingTime(testCase)
+            time_series = types.core.TimeSeries( ...
+                 'data', linspace(0, 0.4, 50), ...
+                 'description', 'a test series', ...
+                 'data_unit', 'n/a' ...
+             );
+
+             testCase.NwbObject.acquisition.set('time_series', time_series);
+             nwbFilePath = fullfile(testCase.OutputFolder, 'testfile.nwb');
+             testCase.verifyError(@(f, fn) nwbExport(testCase.NwbObject, nwbFilePath), ...
+                 'NWB:CustomConstraintUnfulfilled')
+        end
+
         function testExportDependentAttributeWithMissingParentA(testCase)
             testCase.NwbObject.general_source_script_file_name = 'my_test_script.m';
             nwbFilePath = fullfile(testCase.OutputFolder, 'test_part1.nwb');
@@ -42,22 +62,16 @@ function testExportDependentAttributeWithMissingParentA(testCase)
             testCase.verifyWarningFree(@(f, fn) nwbExport(testCase.NwbObject, nwbFilePath))
         end
 
-        function testExportDependentAttributeWithMissingParentB(testCase)
+        function testExportTimeseriesWithoutStartingTimeRate(testCase)
             time_series = types.core.TimeSeries( ...
                 'data', linspace(0, 0.4, 50), ...
-                'starting_time_rate', 10.0, ...
+                'starting_time', 1, ...
                 'description', 'a test series', ...
                 'data_unit', 'n/a' ...
             );
-
             testCase.NwbObject.acquisition.set('time_series', time_series);
             nwbFilePath = fullfile(testCase.OutputFolder, 'test_part1.nwb');
-            testCase.verifyWarning(@(f, fn) nwbExport(testCase.NwbObject, nwbFilePath), 'NWB:DependentAttributeNotExported')
-
-            % Add value for dataset which attribute depends on and export again
-            time_series.starting_time = 1;
-            nwbFilePath = fullfile(testCase.OutputFolder, 'test_part2.nwb');
-            testCase.verifyWarningFree(@(f, fn) nwbExport(testCase.NwbObject, nwbFilePath))
+            testCase.verifyError(@(f, fn) nwbExport(testCase.NwbObject, nwbFilePath), 'NWB:CustomConstraintUnfulfilled')
         end
     end
 

+types/+core/ImageSeries.m

@@ -186,6 +186,12 @@
             end
         end
     end
+    %% CUSTOM CONSTRAINTS
+    function checkCustomConstraint(obj)
+        if ~isempty(obj.external_file) && isempty(obj.data), ...
+            obj.data = nan(1,1,2);
+        end
+    end
 end
 
 end

+types/+core/TimeSeries.m

@@ -415,6 +415,15 @@
             io.writeAttribute(fid, [fullpath '/timestamps/unit'], obj.timestamps_unit);
         end
     end
+    %% CUSTOM CONSTRAINTS
+    function checkCustomConstraint(obj)
+        assert(~isempty(obj.timestamps) || ~isempty(obj.starting_time), ...
+            "'timestamps' or 'starting_time' must be specified")
+        if ~isempty(obj.starting_time)
+            assert(~isempty(obj.starting_time_rate), ...
+                "'starting_time_rate' must be specified when 'starting_time' is specified")
+        end
+    end
 end
 
 end

+types/+untyped/MetaClass.m

@@ -1,8 +1,12 @@
-classdef MetaClass < handle
+classdef MetaClass < handle & matlab.mixin.CustomDisplay
     properties (Hidden, SetAccess = private)
         metaClass_fullPath;
     end
-
+
+    properties (Constant, Transient, Access = protected)
+        REQUIRED containers.Map = containers.Map
+    end
+
     methods
         function obj = MetaClass(varargin)
         end
@@ -43,6 +47,11 @@
 
     methods
         function refs = export(obj, fid, fullpath, refs)
+            % throwErrorIfCustomConstraintUnfulfilled is intentionally placed 
+            % before throwErrorIfMissingRequiredProps. 
+            % See file.fillCustomConstraint
+            obj.throwErrorIfCustomConstraintUnfulfilled(fullpath)
+            obj.throwErrorIfMissingRequiredProps(fullpath)
             obj.metaClass_fullPath = fullpath;
             %find reference properties
             propnames = properties(obj);
@@ -107,4 +116,100 @@ function warnIfPropertyAttributeNotExported(obj, propName, depPropName, fullpath
             warning(warningId, warningMessage) %#ok<SPWRN>
         end
     end
-end
+
+    methods (Access = protected) % Override matlab.mixin.CustomDisplay
+        function str = getFooter(obj)
+            obj.displayWarningIfMissingRequiredProps();
+            str = '';
+        end
+    end
+
+    methods (Access = protected)
+        function missingRequiredProps = checkRequiredProps(obj)
+            missingRequiredProps = {};
+            requiredProps = obj.getRequiredProperties();
+
+            for i = 1:numel(requiredProps)
+                thisPropName = requiredProps{i};
+                if isempty(obj.(thisPropName))
+                    missingRequiredProps{end+1} = thisPropName; %#ok<AGROW>
+                end
+            end
+        end
+
+        function displayWarningIfMissingRequiredProps(obj)
+            missingRequiredProps = obj.checkRequiredProps();
+
+            % Exception: 'file_create_date' is automatically added by the 
+            % matnwb API on export,  so no need to warn if it is missing.
+            if isa(obj, 'types.core.NWBFile')
+                missingRequiredProps = setdiff(missingRequiredProps, 'file_create_date', 'stable');
+            end
+
+            % Exception: 'id' of DynamicTable is automatically assigned if not 
+            % specified, so no need to warn if it is missing.
+            if isa(obj, 'types.hdmf_common.DynamicTable')
+                missingRequiredProps = setdiff(missingRequiredProps, 'id', 'stable');
+            end
+
+            if ~isempty( missingRequiredProps )
+                warnState = warning('backtrace', 'off');
+                cleanerObj = onCleanup(@(s) warning(warnState));
+
+                propertyListStr = obj.prettyPrintPropertyList(missingRequiredProps);
+                warning('NWB:RequiredPropertyMissing', ...
+                    'The following required properties are missing for instance for type "%s":\n%s', class(obj), propertyListStr)
+            end
+        end
+
+        function throwErrorIfMissingRequiredProps(obj, fullpath)
+            missingRequiredProps = obj.checkRequiredProps();
+            if ~isempty( missingRequiredProps )
+                propertyListStr = obj.prettyPrintPropertyList(missingRequiredProps);
+                error('NWB:RequiredPropertyMissing', ...
+                    'The following required properties are missing for instance for type "%s" at file location "%s":\n%s', class(obj), fullpath, propertyListStr)
+            end
+        end
+
+        function throwErrorIfCustomConstraintUnfulfilled(obj, fullpath)
+            try
+                obj.checkCustomConstraint()
+            catch ME
+                error('NWB:CustomConstraintUnfulfilled', ...
+                    'The following error was caught when exporting type "%s" at file location "%s":\n%s', class(obj), fullpath, ME.message)
+            end
+        end
+    end
+
+    methods
+        function checkCustomConstraint(obj) %#ok<MANU>
+            % This method is meant to be overridden by subclasses that have 
+            % custom constraints that can not be inferred from the nwb schema.
+        end
+    end
+
+    methods (Access = private)
+        function requiredProps = getRequiredProperties(obj)
+
+            % Introspectively retrieve required properties and add to
+            % persistent map.
+
+            if isKey(obj.REQUIRED, class(obj) )
+                requiredProps = obj.REQUIRED( class(obj) );
+            else
+                mc = metaclass(obj);
+                propertyDescription = {mc.PropertyList.Description};
+                isRequired = startsWith(propertyDescription, 'REQUIRED');
+                requiredProps = {mc.PropertyList(isRequired).Name};
+                obj.REQUIRED( class(obj) ) = requiredProps;
+            end
+        end
+    end
+
+    methods (Static, Access = private)
+        function propertyListStr = prettyPrintPropertyList(propertyNames)
+            propertyListStr = compose('    %s', string(propertyNames));
+            propertyListStr = strjoin(propertyListStr, newline);
+        end
+    end
+end

.github/workflows/run_codespell.yml

@@ -12,7 +12,7 @@ on:
     branches:
       - master
   push:
-    branches-ignore:
+    branches:
       - master
 
 jobs:

.github/workflows/run_tests.yml

@@ -18,65 +18,67 @@ jobs:
     name: Run MATLAB tests
     runs-on: ubuntu-latest
     steps:
-      - name: check out repository
+      - name: Check out repository
         uses: actions/checkout@v4
-      - name: install python
+      - name: Install python
         uses: actions/setup-python@v5
         with:
           python-version: '3.10'
-      - name: configure python env
+      - name: Configure python env
         run: |
           python -m pip install -U pip
           pip install -r +tests/requirements.txt
           echo "HDF5_PLUGIN_PATH=$(python -c "import hdf5plugin; print(hdf5plugin.PLUGINS_PATH)")" >> "$GITHUB_ENV"
-      - name: install MATLAB
+      - name: Install MATLAB
         uses: matlab-actions/setup-matlab@v2
         with:
           release: R2024a # this is necessary to test dynamic filters
-      - name: run tests
+      - name: Run tests
         uses: matlab-actions/run-command@v2
         with:
           command: results = assertSuccess(nwbtest); assert(~isempty(results), 'No tests ran');
-      - name: upload JUnit results
+      - name: Upload JUnit results
+        if: always()
         uses: actions/upload-artifact@v4
         with:
           name: test-results
           path: testResults.xml
           retention-days: 1
-      - name: upload coverage results
+      - name: Upload coverage results
+        if: always()
         uses: actions/upload-artifact@v4
         with:
           name: test-coverage
           path: ./coverage.xml
   publish_junit:
-    name: Publish JUnit Test Results
+    name: Publish JUnit test results
     runs-on: ubuntu-latest
-    if: ${{ always() }}
+    if: always()
     needs: [run_tests]
     steps:
-      - name: retrieve result files
+      - name: Retrieve result files
         uses: actions/download-artifact@v4
         with:
           name: test-results
-      - name: publish results
+      - name: Publish test results
         uses: mikepenz/action-junit-report@v4
         with:
           report_paths: 'testResults.xml'
   publish_coverage:
-    name: Publish Cobertura Test Coverage
+    name: Publish Cobertura test coverage
     runs-on: ubuntu-latest
     needs: [run_tests]
     steps:
-      - name: check out repository
+      - name: Check out repository
         uses: actions/checkout@v4
       - name: retrieve code coverage files
         uses: actions/download-artifact@v4
         with:
           name: test-coverage
-      - name: publish on Codecov
+      - name: Publish on coverage results on Codecov
         uses: codecov/codecov-action@v4
         with:
           token: ${{ secrets.CODECOV_TOKEN }}
           files: coverage.xml
           name: codecov-matnwb
-          verbose: true
+          verbose: true

nwbtest.m

@@ -62,7 +62,7 @@
         coverageFile = fullfile(ws, 'coverage.xml');
         [installDir, ~, ~] = fileparts(mfilename('fullpath'));
 
-        ignoreFolders = {'tutorials', '+contrib', '+util', 'external_packages', '+tests'};
+        ignoreFolders = {'tutorials', 'tools', '+contrib', '+util', 'external_packages', '+tests'};
         ignorePaths = {fullfile('+misc', 'generateDocs.m'), [mfilename '.m'], 'nwbClearGenerated.m'};
         mfilePaths = getMfilePaths(installDir, ignoreFolders, ignorePaths);
         if ~verLessThan('matlab', '9.3') && ~isempty(mfilePaths)