Merge pull request #7 from bids-standard/bep031_review_updates

Bep031 review updates

bids-validator-web/package.json

@@ -5,6 +5,7 @@
   "main": "index.js",
   "license": "MIT",
   "dependencies": {
+    "@babel/runtime": "^7.16.7",
     "bootstrap": "^4.3.0",
     "bowser": "^1.0.0",
     "next": "^11.1.2",

bids-validator/bin/bids-validator

@@ -8,6 +8,7 @@ function entry(cli) {
 try {
   // Test if there's a development tree to run
   require.resolve('../cli.js')
+  process.env.ESBUILD_MAX_BUFFER = 64 * 1024 * 1024
   // For dev, use esbuild-runner
   require('esbuild-runner/register')
   const { default: cli } = require('../cli.js')

bids-validator/package.json

@@ -58,7 +58,9 @@
     "stream-browserify": "^3.0.0",
     "table": "^5.2.3",
     "yaml": "^1.10.2",
-    "yargs": "^16.2.0"
+    "yargs": "^16.2.0",
+    "exifreader": "^4.1.0",
+    "xml2js": "^0.4.23"
   },
   "devDependencies": {
     "adm-zip": "",

bids-validator/tests/tsv.spec.js

@@ -685,4 +685,30 @@ describe('TSV', function() {
     let issues = validate.TSV.validateContRec([physio_file], {})
     assert(issues.length === 1 && issues[0].code === 133)
   })
+
+  // samples checks -----------------------------------------------------------
+
+  const samplesFile = {
+    name: 'samples.tsv',
+    relativePath: '/samples.tsv',
+  }
+
+  it('should return errors for each missing mandatory header in samples.tsv', () => {
+    const tsv = 'wrong_col\nsome_data\n'
+    validate.TSV.TSV(samplesFile, tsv, [], function(issues) {
+      expect(issues.length).toBe(3)
+      const codes = issues.map(x => x.code)
+      expect(codes.includes(216)).toBe(true)
+      expect(codes.includes(217)).toBe(true)
+      expect(codes.includes(218)).toBe(true)
+    })
+  })
+
+  it('should return an error for invalid sample_type samples.tsv', () => {
+    const tsv = 'sample_type\nbad\n'
+    validate.TSV.TSV(samplesFile, tsv, [], function(issues) {
+      const codes = issues.map(x => x.code)
+      expect(codes.includes(219)).toBe(true)
+    })
+  })
 })

bids-validator/utils/files/index.js

@@ -15,7 +15,6 @@ import illegalCharacterTest from './illegalCharacterTest'
 import sessions from './sessions'
 import remoteFiles from './remoteFiles'
 import getFileStats from './getFileStats'
-import generateMergedSidecarDictWithPath from './generateMergedSidecarDictWithPath'
 
 // public API ---------------------------------------------------------------------
 
@@ -28,7 +27,6 @@ export default {
   readOMEFile,
   readNiftiHeader,
   generateMergedSidecarDict,
-  generateMergedSidecarDictWithPath,
   potentialLocations,
   getBFileContent,
   collectDirectorySize,

bids-validator/utils/files/readFile.js

@@ -25,7 +25,11 @@ const checkEncoding = (file, data, cb) => {
 }
 
 /**
- * Read
+ * readFile
+ * @param {object | File} file - nodeJS fs file or browser File
+ * @param {boolean} annexed - is the file currently annexed?
+ * @param {string} dir - path to directory containing dataset. Only used if
+ *   annexed is true.
  *
  * A helper method for reading file contents.
  * Takes a file object and a callback and calls
@@ -39,12 +43,12 @@ const checkEncoding = (file, data, cb) => {
 function readFile(file, annexed, dir) {
   return new Promise((resolve, reject) => {
     if (isNode) {
-      testFile(file, annexed, dir, function (issue, stats, remoteBuffer) {
+      testFile(file, annexed, dir, function(issue, stats, remoteBuffer) {
         if (issue) {
           return reject(issue)
         }
         if (!remoteBuffer) {
-          fs.readFile(file.path, function (err, data) {
+          fs.readFile(file.path, function(err, data) {
             if (err) {
               return reject(err)
             }

bids-validator/validators/bids/fullTest.js

@@ -148,6 +148,7 @@ const fullTest = (fileList, options, annexed, dir, schema, callback) => {
           files,
           jsonContentsDict,
         )
+
         self.issues = self.issues
           .concat(samplesIssues)
           .concat(jsonAndFieldIssues)

bids-validator/validators/microscopy/__tests__/checkJSONAndField.spec.js

@@ -0,0 +1,51 @@
+import { assert } from 'chai'
+import checkJSONAndField from '../checkJSONAndField'
+
+describe('checkJSONAndField()', () => {
+  const emptyJsonContentsDict = {
+    'test.json': {},
+  }
+  it('returns no issues with empty arguments', () => {
+    const issues = checkJSONAndField({}, {})
+    expect(issues.length).toBe(0)
+  })
+
+  it('returns issue 225 with no json for ome files', () => {
+    const files = {
+      ome: [{ relativePath: 'test.ome.tif' }],
+    }
+    const issues = checkJSONAndField(files, emptyJsonContentsDict)
+    expect(issues.length).toBe(1)
+    expect(issues[0].code).toBe(225)
+  })
+
+  it('returns issue 225 with no json for tif files', () => {
+    const files = {
+      tif: [{ relativePath: 'test.tif' }],
+    }
+    const issues = checkJSONAndField(files, emptyJsonContentsDict)
+    expect(issues.length).toBe(1)
+    expect(issues[0].code).toBe(225)
+  })
+
+  it('returns issue 225 with no json for png files', () => {
+    const files = {
+      png: [{ relativePath: 'test.png' }],
+    }
+    const issues = checkJSONAndField(files, emptyJsonContentsDict)
+    expect(issues.length).toBe(1)
+    expect(issues[0].code).toBe(225)
+  })
+
+  it('returns warning 223 if chunk entity present but missing metadata', () => {
+    const files = {
+      ome: [{ relativePath: '/test_chunk-01.ome.tif' }],
+    }
+    const jsonContentsDict = {
+      '/test_chunk-01.json': { testKey: 'testValue' },
+    }
+    const issues = checkJSONAndField(files, jsonContentsDict)
+    expect(issues.length).toBe(1)
+    expect(issues[0].code).toBe(223)
+  })
+})

bids-validator/validators/microscopy/__tests__/checkSample.spec.js

@@ -0,0 +1,19 @@
+import checkSamples from '../checkSamples'
+describe('checkSamples()', () => {
+  it('returns issue 214 when no samples.tsv is present', () => {
+    const fileList = {
+      '0': { relativePath: '/test.tsv' },
+    }
+    const issues = checkSamples(fileList)
+    expect(issues.length).toBe(1)
+    expect(issues[0].code).toBe(214)
+  })
+
+  it('doesnt return issue 214 when samples.tsv is present', () => {
+    const fileList = {
+      '0': { relativePath: '/samples.tsv' },
+    }
+    const issues = checkSamples(fileList)
+    expect(issues.length).toBe(0)
+  })
+})

bids-validator/validators/microscopy/__tests__/validate.spec.js

@@ -0,0 +1,99 @@
+import path from 'path'
+
+import readDir from '../../../utils/files/readDir'
+import validate from '../validate'
+
+const dataDir = path.join(__dirname, '/data')
+
+const jsonContent = {
+  Manufacturer: 'Miltenyi Biotec',
+  ManufacturersModelName: 'UltraMicroscope II',
+  BodyPart: 'CSPINE',
+  SampleEnvironment: 'ex vivo',
+  SampleFixation: '4% paraformaldehyde, 2% glutaraldehyde',
+  SampleStaining: 'Luxol fast blue',
+  PixelSize: [1, 1, 1],
+  PixelSizeUnits: 'um',
+  Immersion: 'Oil',
+  NumericalAperture: 1.4,
+  Magnification: 40,
+  ChunkTransformationMatrix: [
+    [1, 0, 0, 0],
+    [0, 2, 0, 0],
+    [0, 0, 1, 0],
+    [0, 0, 0, 1],
+  ],
+  ChunkTransformationMatrixAxis: ['X', 'Y', 'Z'],
+}
+
+describe('validate', () => {
+  it('returns error 227 with extension/id mismatch', () => {
+    const fileName = 'btif_id.ome.tif'
+    const files = [
+      {
+        name: fileName,
+        relativePath: `/bids-validator/validators/microscopy/__tests__/data/${fileName}`,
+        path: path.join(dataDir, fileName),
+      },
+    ]
+
+    expect.assertions(3)
+    return validate(files, {}).then(issues => {
+      expect(issues.length).toBe(2)
+      expect(issues[0].code).toBe(227)
+      expect(issues[1].code).toBe(226)
+    })
+  })
+
+  it('returns error 227 with incorrect id in magic number', () => {
+    const fileName = 'invalid_id.ome.tif'
+    const files = [
+      {
+        name: fileName,
+        relativePath: `/bids-validator/validators/microscopy/__tests__/data/${fileName}`,
+        path: path.join(dataDir, fileName),
+      },
+    ]
+    expect.assertions(2)
+    return validate(files, {}).then(issues => {
+      expect(issues.length).toBe(1)
+      expect(issues[0].code).toBe(227)
+    })
+  })
+
+  it('returns error 227 with tif id and btf extension', () => {
+    const fileName = 'tif_id.ome.btf'
+    const files = [
+      {
+        name: fileName,
+        relativePath: `/bids-validator/validators/microscopy/__tests__/data/${fileName}`,
+        path: path.join(dataDir, fileName),
+      },
+    ]
+
+    expect.assertions(2)
+    return validate(files, {}).then(issues => {
+      expect(issues.length).toBe(1)
+      expect(issues[0].code).toBe(227)
+    })
+  })
+
+  it('validates with valid data', () => {
+    const fileName = 'valid.ome.tif'
+    const relativePath = `/bids-validator/validators/microscopy/__tests__/data/${fileName}`
+    const files = [
+      {
+        name: fileName,
+        relativePath: relativePath,
+        path: path.join(dataDir, fileName),
+      },
+    ]
+    const jsonContentDict = {}
+    jsonContentDict[relativePath.replace('.ome.tif', '.json')] = jsonContent
+
+    expect.assertions(1)
+    return validate(files, jsonContentDict).then(issues => {
+      expect(issues.length).toBe(0)
+    })
+  })
+})

bids-validator/validators/microscopy/checkJSONAndField.js

@@ -9,7 +9,6 @@ const checkJSONAndField = (files, jsonContentsDict) => {
         .replace('.tif', '')
         .replace('.btf', '')
         .replace('.ome', '.json')
-
       issues = issues.concat(
         ifJsonExist(file, possibleJsonPath, jsonContentsDict),
       )
@@ -40,8 +39,15 @@ const checkJSONAndField = (files, jsonContentsDict) => {
 
 const ifJsonExist = (file, possibleJsonPath, jsonContentsDict) => {
   let potentialSidecars = utils.files.potentialLocations(possibleJsonPath)
+  const chunkRegex = new RegExp('_chunk-[0-9]+')
+
+  const jsonChunkFiles = potentialSidecars.filter(
+    name => jsonContentsDict.hasOwnProperty(name) && chunkRegex.exec(name),
+  )
+  const chunkPresent =
+    jsonChunkFiles.length || chunkRegex.exec(file.relativePath)
 
-  const mergedDictionary = utils.files.generateMergedSidecarDictWithPath(
+  const mergedDictionary = utils.files.generateMergedSidecarDict(
     potentialSidecars,
     jsonContentsDict,
   )
@@ -54,29 +60,24 @@ const ifJsonExist = (file, possibleJsonPath, jsonContentsDict) => {
         code: 225,
       }),
     ]
-  } else {
+  }
+
+  if (chunkPresent) {
     return checkMatrixField(file, mergedDictionary)
   }
+
+  return []
 }
 
 const checkMatrixField = (file, mergedDictionary) => {
   let issues = []
-  let regex = new RegExp('_chunk-[0-9]+')
-  let jsonPath = mergedDictionary.sidecarName
-
-  // ChunkTransformationMatrix is RECOMMENDED if <chunk-index> is used in filenames
-  if (regex.exec(file.relativePath) || regex.exec(jsonPath)) {
-    if (!mergedDictionary.hasOwnProperty('ChunkTransformationMatrix')) {
-      issues.push(
-        new Issue({
-          file: {
-            path: jsonPath,
-            relativePath: jsonPath,
-          },
-          code: 223,
-        }),
-      )
-    }
+  if (!mergedDictionary.hasOwnProperty('ChunkTransformationMatrix')) {
+    issues.push(
+      new Issue({
+        file: file,
+        code: 223,
+      }),
+    )
   }
   return issues
 }

bids-validator/validators/microscopy/validate.js

@@ -55,6 +55,15 @@ const validate = (files, jsonContentsDict) => {
                 }),
               )
               resolve()
+            } else {
+              issues.push(
+                new Issue({
+                  code: 227,
+                  file: file,
+                  evidence: `3rd byte of file does not identify file as tiff.`,
+                }),
+              )
+              resolve()
             }
           })
         }),