Skip to content

Add context-aware schema violation reporting#836

Open
ehennestad wants to merge 3 commits into
mainfrom
codex/report-schema-violation-context
Open

Add context-aware schema violation reporting#836
ehennestad wants to merge 3 commits into
mainfrom
codex/report-schema-violation-context

Conversation

@ehennestad

@ehennestad ehennestad commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Motivation

MatNWB needs to report schema validation violations differently depending on when validation runs (PR #830, PR #832). Values read from existing files should remain readable, so read-time schema violations should warn. Values being written should not produce invalid NWB output, so write-time schema violations should always error. During normal in-memory editing, validation should error by default, while specific backwards-compatible validators can opt into warning instead of error (This requirement was added during review in PR #837).

This PR adds matnwb.common.validation.reportSchemaViolation as the shared reporting helper for schema validation failures. It gates warning and error through a validation context with READ, EDIT, and WRITE modes:

  • READ always warns.
  • EDIT errors by default.
  • EDIT can warn when a validator passes WarnInsteadOfError=true.
  • WRITE always errors.

The read context is set while deserializing parsed types, and the write context is set during NwbFile.export. Existing DynamicTable validation paths now use the shared reporter.

How to test the behavior?

runtests('tests.unit.common.validation.ReportSchemaViolationTest')
runtests('tests.unit.io.testCreateParsedType')
runtests('tests.unit.dynamicTableTest')

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 Codex

@ehennestad ehennestad marked this pull request as ready for review June 25, 2026 11:01
@ehennestad ehennestad requested a review from Copilot June 25, 2026 11:01

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 introduces a centralized, context-aware mechanism for reporting schema validation violations so MatNWB can be lenient when reading existing files (warn + keep value) while remaining strict during editing (error by default, optionally warn for backwards-compatible validators) and writing (always error to prevent invalid NWB output).

Changes:

  • Added matnwb.common.validation.reportSchemaViolation plus a new internal validation context (READ/EDIT/WRITE) to gate warning vs. error behavior.
  • Set the validation context during deserialization (io.createParsedType => READ) and export (NwbFile.export => WRITE).
  • Routed DynamicTable schema-violation reporting through the new shared reporter and added unit tests for the new behavior.

Reviewed changes

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

Show a summary per file
File Description
NwbFile.m Sets schema validation context to WRITE during export and restores it afterward.
+io/createParsedType.m Sets schema validation context to READ during parsed type construction and restores it afterward.
+types/+util/+dynamictable/validateUniqueColnames.m Uses the shared reporter for duplicate colname violations.
+types/+util/+dynamictable/syncNamedColumn.m Skips syncing on read using the new read-context helper.
+types/+util/+dynamictable/checkConfig.m Uses the shared reporter for colnames mismatch and references new read-context helper.
+types/+util/validationContext.m Removes the old strict/read context helper.
+tests/+unit/+io/testCreateParsedType.m Updates tests to use the new validation context setter.
+tests/+unit/+common/+validation/ReportSchemaViolationTest.m Adds focused tests for context-aware schema violation reporting behavior.
+matnwb/+common/+validation/reportSchemaViolation.m Implements the shared error/warn reporting helper with optional causes and WarnInsteadOfError.
+matnwb/+common/+validation/isReadContext.m Adds a helper for checking read context.
+matnwb/+common/+validation/+internal/ValidationContext.m Adds the READ/EDIT/WRITE enum.
+matnwb/+common/+validation/+internal/context.m Adds process-local get/set for the validation context.

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

Comment thread +matnwb/+common/+validation/+internal/context.m
@codecov

codecov Bot commented Jun 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.11356% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.13%. Comparing base (6f4e97a) to head (80d8e8f).

Files with missing lines Patch % Lines
+matnwb/+neurodata/AlignedDynamicTableBase.m 85.89% 22 Missing ⚠️
...es/+util/+dynamictable/+internal/getColumnHeight.m 90.00% 2 Missing ⚠️
...nternal/collectConstantPropertiesAcrossHierarchy.m 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #836      +/-   ##
==========================================
- Coverage   95.31%   95.13%   -0.19%     
==========================================
  Files         219      223       +4     
  Lines        7647     7913     +266     
==========================================
+ Hits         7289     7528     +239     
- Misses        358      385      +27     

☔ 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.

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