Skip to content

Validate property values on export to prevent writing invalid files#832

Open
ehennestad wants to merge 7 commits into
fix-scalar-datetime-exportfrom
validate-properties-on-export
Open

Validate property values on export to prevent writing invalid files#832
ehennestad wants to merge 7 commits into
fix-scalar-datetime-exportfrom
validate-properties-on-export

Conversation

@ehennestad

@ehennestad ehennestad commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator

⚠️ Depends on — merge these first:

Motivation

MatNWB validates property values in property set method when objects are constructed, but export does not re-validate them before writing to file. This was sufficient while the only way to populate a property was through its (strict) setter.

PR #822 introduced permissive reads for DynamicTable column names, and the broader read-validation work in #776 will extend permissive reads to other schema checks (data type, shape, neurodata type, fixed/constant attribute values). Once a value can enter an object without passing strict validation, nothing stops it from being written back out to a new, non-conforming file.

This PR adds an export-time guard so a value that bypassed strict validation cannot be written to a new file.

Changes

  • Add types.untyped.MetaClass/validateProperties, invoked in export alongside the existing required-property and custom-constraint checks. For each set property that has a generated validate_<prop> method, it re-runs the validator and discards the result, so there are no setter side effects such as type coercion or column-name syncing. If the value is invalid it raises NWB:Export:InvalidPropertyValue, including the property name and file location, and preserves the original validator error as a cause.
  • Empty/unset optional properties are skipped, matching what export writes.
  • Validators run in the default (strict) context here, so a schema violation is an error on export even when the same check is lenient during a read.

Relationship to other work

This is the write-side counterpart to the read-side permissiveness in #776 — read lenient (warn, keep value), write strict (error). It generalizes the pattern already used for DynamicTable (checkConfig via the custom-constraint path) to all property validators. Its user-visible effect lands together with #776, so this is best reviewed and merged alongside that work.

How to test

tests.unit.types.MetaClassValidatePropertiesTest exercises the guard with a small MetaClass test double: an invalid property value raises NWB:Export:InvalidPropertyValue with the property name, location, and original cause; an unset (empty) property is skipped; a valid value passes.

Checklist

  • Have you ensured the PR description clearly describes the problem and solutions?
  • Have you checked to ensure that there aren't other open or previously closed Pull Requests for the same change?
  • If this PR fixes an issue, is the first line of the PR description fix #XX where XX is the issue number?

🤖 Generated with Claude Code

@codecov

codecov Bot commented Jun 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.14286% with 1 line in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (fix-scalar-datetime-export@f953063). Learn more about missing BASE report.

Files with missing lines Patch % Lines
+types/+untyped/MetaClass.m 97.14% 1 Missing ⚠️
Additional details and impacted files
@@                      Coverage Diff                      @@
##             fix-scalar-datetime-export     #832   +/-   ##
=============================================================
  Coverage                              ?   95.32%           
=============================================================
  Files                                 ?      219           
  Lines                                 ?     7682           
  Branches                              ?        0           
=============================================================
  Hits                                  ?     7323           
  Misses                                ?      359           
  Partials                              ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ehennestad ehennestad changed the base branch from fix-read-only-validator to main June 24, 2026 19:52
ehennestad and others added 7 commits June 24, 2026 23:17
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
… test

Co-authored-by: ehennestad <17237719+ehennestad@users.noreply.github.com>
@ehennestad ehennestad force-pushed the validate-properties-on-export branch from 484da14 to a35e1c4 Compare June 24, 2026 21:22
@ehennestad ehennestad changed the base branch from main to fix-scalar-datetime-export June 24, 2026 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants