Skip to content

Warn instead of error when reading values invalid per the NWB schema#830

Draft
ehennestad wants to merge 13 commits into
mainfrom
776-warn-on-reading-invalid-values
Draft

Warn instead of error when reading values invalid per the NWB schema#830
ehennestad wants to merge 13 commits into
mainfrom
776-warn-on-reading-invalid-values

Conversation

@ehennestad

@ehennestad ehennestad commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Fix #776 & #827

⚠️ Depends on — merge these first:

Motivation

Part of #776. When reading an NWB file, a value that deviates from the schema currently causes a hard read error, which can make an otherwise-usable file completely unreadable over a minor issue (for example, a fixed attribute such as ElectricalSeries/data/unit being something other than volts).

This PR makes parsed file reads permissive: a non-conforming value produces a warning and is kept, so the data remains accessible, while constructing or assigning such a value in user code stays strict (an error). Read lenient, write strict.

Changes

A shared helper, types.util.reportSchemaViolation(id, message[, causes]), centralizes the decision: it errors in the strict (construction) context and warns in the read context, appending guidance (the value is kept; correct it and re-export if you maintain the file). The context is the one introduced in #822 (types.util.validationContext), set to read by io.createParsedType. Optional causes are attached with addCause when erroring and appended to the text when warning, so structured errors are preserved.

The following validators now route through it (warn-on-read, error-on-construct):

  • Fixed/constant attribute values — new types.util.checkConstant, emitted by fillReadOnlyValidation; core type classes regenerated.
  • Neurodata typecheckType.
  • ShapevalidateShape / checkDims.
  • Soft-link target typevalidateSoftLink.
  • Constrained-set typecheckConstraint.

Internal invariants (for example checkType's unrecognized-expected-type assert) deliberately remain hard errors in both contexts.

Scope

This is the read-side complement to the export-side guard in #829 (writing stays strict). It does not yet cover checkDtype / correctType (the type-coercion family) — those need a careful per-assert classification and will follow in a separate PR. So this is part of #776, not a full resolution.

Notes

  • nwbReadTest/readFileWithInvalidTimeSeriesTimestampsShape asserted the old "reading a bad shape fails" behavior; it now asserts the read succeeds with a warning and keeps the value.
  • New unit tests: CheckConstantTest, CheckTypeTest, CheckConstraintTest, ValidateSoftLinkTest.

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements “read lenient, write strict” behavior for NWB schema validation: when parsing an NWB file, schema violations that previously caused hard read failures now emit warnings and keep the non-conforming values to preserve data accessibility, while construction-time validation remains strict (errors).

Changes:

  • Add types.util.reportSchemaViolation(...) to centralize schema-violation reporting as warning-on-read vs error-on-construct, optionally preserving causes.
  • Route multiple validators through the new helper (type checks, shape checks, soft-link target type checks, constrained-set checks), and introduce types.util.checkConstant for schema-fixed attribute validation (regenerated core type validators).
  • Update and add unit tests to reflect permissive read behavior (including new validator-focused test classes).

Reviewed changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
+types/+util/reportSchemaViolation.m New helper to warn-on-read / error-on-construct for schema violations (with optional causes).
+types/+util/checkConstant.m New helper to validate schema-fixed (constant) values while keeping invalid read-time values.
+types/+util/checkType.m Switch wrong-type reporting to reportSchemaViolation for read permissiveness.
+types/+util/validateShape.m Convert shape failures to warn-on-read via reportSchemaViolation, preserving causes in strict mode.
+types/+util/validateSoftLink.m Convert soft-link declared target-type mismatch to warn-on-read via reportSchemaViolation.
+types/+util/checkConstraint.m Convert constrained-set type mismatch to warn-on-read via reportSchemaViolation.
+file/fillValidators.m Update generator to emit constant-value validators via types.util.checkConstant.
+types/+core/*.m (multiple) Regenerated validators to use types.util.checkConstant for fixed schema values.
+tests/+unit/nwbReadTest.m Update read test to expect warning+successful read (instead of error) for invalid timestamps shape.
+tests/+unit/+types/+validators/CheckConstantTest.m New unit tests for constant-value validation behavior in strict vs read contexts.
+tests/+unit/+types/+validators/CheckTypeTest.m New unit tests for strict vs read-context type checking behavior.
+tests/+unit/+types/+validators/CheckConstraintTest.m New unit tests for strict vs read-context constrained-set type behavior.
+tests/+unit/+types/+validators/ValidateSoftLinkTest.m New unit tests for strict vs read-context soft-link target-type behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread +types/+util/reportSchemaViolation.m Outdated
Comment on lines +13 to +37
arguments
errorId (1,1) string
message (1,1) string
causes (1,:) MException = MException.empty(1, 0)
end

if strcmp(types.util.validationContext(), 'read')
readGuidance = ['The value read from the file is kept so the file ' ...
'remains readable. If you maintain this file, consider ' ...
'correcting it and re-exporting.'];

fullMessage = message;
for iCause = 1:numel(causes)
fullMessage = fullMessage + " " + string(causes(iCause).message);
end
fullMessage = fullMessage + " " + readGuidance;

warning(errorId, '%s', fullMessage)
else
exception = MException(errorId, '%s', message);
for iCause = 1:numel(causes)
exception = exception.addCause(causes(iCause));
end
throw(exception)
end
@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.50746% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 95.30%. Comparing base (94e13bc) to head (5c1ae5a).

Files with missing lines Patch % Lines
+types/+util/checkConstant.m 94.73% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #830      +/-   ##
==========================================
- Coverage   95.31%   95.30%   -0.01%     
==========================================
  Files         218      220       +2     
  Lines        7634     7668      +34     
==========================================
+ Hits         7276     7308      +32     
- Misses        358      360       +2     

☔ 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 force-pushed the 776-warn-on-reading-invalid-values branch from ae75126 to 5c1ae5a Compare June 19, 2026 13:25
@ehennestad ehennestad marked this pull request as draft June 20, 2026 06:20
@ehennestad ehennestad force-pushed the 776-warn-on-reading-invalid-values branch from 5c1ae5a to d62b1b3 Compare June 25, 2026 13:24
ehennestad and others added 7 commits June 25, 2026 15:26
Read-only validator now corrects data type if needed.
Read-only validator now works for values that are floats or non-scalar (via using mat2str)
Introduce types.util.reportSchemaViolation to centralize the choice between a strict error and a read-context warning, and to append consistent guidance (the value is kept, how to correct it) when reading. checkConstant delegates to it with a single neutral message, keeping the class doc hyperlink. This is the shared mechanism the remaining read validators will use.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Route checkType and validateShape through reportSchemaViolation so a wrong neurodata type or non-conforming shape produces a warning and keeps the value when reading a file, while construction stays strict. checkType's unknown-expected-type assert remains a hard error in both contexts, since it indicates an internal problem rather than a non-conforming file.

reportSchemaViolation now accepts an optional array of cause exceptions: it attaches them with addCause when erroring and appends their messages when warning, preserving validateShape's structured strict error.

Update the nwbReadTest case that asserted the old read-fails-on-bad-shape behavior, and add CheckTypeTest.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…reading

Route validateSoftLink (a soft link declaring the wrong target type) and checkConstraint (a value whose type matches none of the allowed types) through reportSchemaViolation. Both now warn and keep the value when reading a file, while still erroring on construction.

Add ValidateSoftLinkTest and CheckConstraintTest.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@ehennestad ehennestad force-pushed the 776-warn-on-reading-invalid-values branch from b5a87a5 to 76ce128 Compare June 25, 2026 13:40
ehennestad and others added 3 commits June 25, 2026 20:52
checkDtype gains a Mode option: 'strict' throws so callers can probe candidate dtypes, while the default 'report' mode wraps the strict path and routes genuine schema-value violations through reportSchemaViolation, keeping the original value. A non-conforming dtype is therefore a warning on read instead of a failed read. checkConstraint probes each allowed type in strict mode and reports one violation when none match.

Add CheckDtypeTest and extend CheckConstraintTest.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Distinguish structural defects from recoverable deviations. A non-text colnames cannot be iterated or used to look up columns, so it is a hard error in every context (normalizeColnames). Inconsistent column heights and an id count that does not match the table height are recoverable: the values stay inspectable on the in-memory object, so checkConfig reports them as warnings on read instead of erroring. Materialized columns missing from colnames also warn on read.

Add DynamicTableCheckConfigTest.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
An invalid property value (one that cannot be converted to the schema dtype) is now read permissively: createParsedType warns and keeps the value instead of throwing TypeCreationFailed. Flip testCreateTypeWithInvalidPropertyValue to expect the warning. Belongs with the checkDtype read-validation change (3024a92).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.

[Feature]: Warning instead of error when reading values that are not valid according to the NWB schema

2 participants