bids-standard / legacy-validator

Validator for the Brain Imaging Data Structure
https://bids-standard.github.io/legacy-validator/
MIT License
186 stars 111 forks source link

feat: I/O checks for NIfTI and JSON files #2056

Closed effigies closed 3 months ago

effigies commented 3 months ago

This implements standard issues:

Because BIDSFile.text() is intended to mimic the behavior of File.text() and replace unknown characters, it's unsuited to validating JSON, which needs to be actual UTF-8. Further, piping BIDSFile.stream through a TextDecoderStream can reliably cause resource leaks on UTF16 data (https://github.com/denoland/deno/issues/24872). Therefore I've reimplemented reading with readBytes() followed by an all-at-once decoding, which seems to be safer.

The main bit of interest is in passing options to TextDecoderStream to ensure we catch bad unicode. This creates a divergence with the browser implementation, so we would need to decide what the best API is. I could switch to using file.stream directly.

There appears to be a bug in TextDecoderStream (https://github.com/denoland/deno/issues/24872) that prevents us cleaning up properly. So the subtests all pass, but the whole JSON UTF-16 test fails due to a detected leak.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 84.14634% with 13 lines in your changes missing coverage. Please review.

Project coverage is 87.13%. Comparing base (e93fd01) to head (40ef8be).

Files Patch % Lines
bids-validator/src/schema/context.ts 20.00% 12 Missing :warning:
bids-validator/src/files/json.ts 96.29% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2056 +/- ## ========================================== + Coverage 85.75% 87.13% +1.37% ========================================== Files 91 139 +48 Lines 3785 6676 +2891 Branches 1218 1572 +354 ========================================== + Hits 3246 5817 +2571 - Misses 453 769 +316 - Partials 86 90 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.