Merge pull request #98 from IIIF/fix_manifest_error

Fixing issue that causes the 500 error
IIIF · Mar 27, 2020 · 4e98e04 · 4e98e04
2 parents cc92e81 + 52b2c02
commit 4e98e04
Show file tree

Hide file tree

Showing 6 changed files with 106 additions and 31 deletions.
diff --git a/fixtures/3/old_format_label.json b/fixtures/3/old_format_label.json
@@ -0,0 +1,52 @@
+{
+  "@context": [
+    "http://www.w3.org/ns/anno.jsonld",
+    "http://iiif.io/api/presentation/3/context.json"
+  ],
+  "id": "https://example.com/manifest",
+  "label": "Old table label which doesn't have a language",
+  "type": "Manifest",
+  "sequences": [
+    {
+      "id": "https://example.com/sequence",
+      "label": "Current order",
+      "type": "Sequence",
+      "viewingDirection": "left-to-right",
+      "canvases": [
+        {
+          "type": "Canvas",
+          "id": "https://example.com/canvas1",
+          "label": "Image 1",
+          "height": 3820,
+          "width": 5426,
+          "content": [
+            {
+              "type": "AnnotationPage",
+              "id": "https://example.com/anno1",
+              "items": [
+                {
+                  "id": "https://example.com/image1",
+                  "type": "Annotation",
+                  "motivation": "painting",
+                  "target": "https://example.com/canvas1",
+                  "body": {
+                    "type": "Image",
+                    "id": "https://example.com/image/full.jpg",
+                    "format": "image/jpeg",
+                    "height": 3820,
+                    "width": 5426,
+                    "service": {
+                      "@context": "http://iiif.io/api/image/2/context.json",
+                      "id": "https://example.com/image1/api/1",
+                      "profile": "http://iiif.io/api/image/2/level2.json"
+                    }
+                  }
+                }
+              ]
+            }
+          ]
+        }
+      ]
+    }
+  ]
+}
diff --git a/schema/error_processor.py b/schema/error_processor.py
@@ -115,10 +115,10 @@ def diagnoseWhichOneOf(self, error_path, IIIFJsonPath):
                         # if this oneOf possiblity is still relevant add it to the list and check
                         # its not another oneOf error
                         if addErrors:
-                            # if error is also a oneOf then diagnoise aggain
-                            if err.absolute_schema_path[-1] == 'oneOf':
+                            # if error is also a oneOf then diagnoise again
+                            if err.absolute_schema_path[-1] == 'oneOf' and err.absolute_schema_path != error_path:
                                 error_path.append(oneOfIndex) # this is is related to one of the original oneOfs at index oneOfIndex
-                                error_path.append('oneOf') # but we found another oneOf test at this location
+                                error_path.extend(err.absolute_schema_path) # but we found another oneOf test at this location
                                 store_errs.append(self.diagnoseWhichOneOf(error_path, IIIFJsonPath)) # so recursivly discovery real error
                         #print ('would add: {} by addErrors is {}'.format(err.message, addErrors))
                         store_errs.append(err)
@@ -142,7 +142,7 @@ def diagnoseWhichOneOf(self, error_path, IIIFJsonPath):
             path = parse(self.pathToJsonPath(IIIFJsonPath))
             instance = path.find(self.iiif_asset)[0].value
             IIIFJsonPath.append('type')
-            print (IIIFJsonPath)
+            #print (IIIFJsonPath)
             return ValidationError(message='Failed to find out which oneOf test matched your data. This is likely due to an issue with the type and it not being valid value at this level. SchemaPath: {}'.format(self.pathToJsonPath(error_path)),
                                     path=[], schema_path=error_path, schema=self.getSchemaPortion(error_path), instance=instance) 
 
@@ -213,6 +213,11 @@ def parse(self, error_path, schemaEl, iiif_asset, IIIFJsonPath, parent=None, jso
         if len(error_path) <= 0:
             return True
 
+        if isinstance(schemaEl, dict) and "$ref" in schemaEl:
+            #print ('Found ref, trying to resolve: {}'.format(schemaEl['$ref']))
+            return self.parse(error_path, self.resolver.resolve(schemaEl['$ref'])[1], iiif_asset, IIIFJsonPath, parent, jsonPath)
+
+
         pathEl = error_path.pop(0)
         # Check current type to see if its a match
         if pathEl == 'type' and parent == 'properties':
@@ -227,7 +232,7 @@ def parse(self, error_path, schemaEl, iiif_asset, IIIFJsonPath, parent=None, jso
                         value.append(option['pattern'])
                     else:
                         value.append(option['const'])
-                print ('Using values: {}'.format(value))
+                #print ('Using values: {}'.format(value))
             if not self.isTypeMatch(jsonPath + '.type', iiif_asset, value, IIIFJsonPath):
                 return False
         # Check child type to see if its a match        
@@ -294,7 +299,7 @@ def isTypeMatch(self, iiifPath, iiif_asset, schemaType, IIIFJsonPath):
         #print ('Found type {} and schemaType {}'.format(typeValue, schemaType))
         if isinstance(schemaType, list):
             for typeOption in schemaType:
-                print ('Testing {} = {}'.format(typeOption, typeValue)) 
+                #print ('Testing {} = {}'.format(typeOption, typeValue)) 
                 if re.match(typeOption, typeValue):
                     return True
             return False        

diff --git a/schema/iiif_3_0.json b/schema/iiif_3_0.json
@@ -62,6 +62,23 @@
         "duration": {
             "type": "number",
             "minimum": 0
+        },
+        "external": {
+            "type": "array",
+            "items": {
+                "allOf": [
+                    { "$ref": "#/types/class" },
+                    {
+                        "type": "object",
+                        "properties": {
+                            "format": { "$ref": "#/types/format" },
+                            "profile": {
+                                "type": "string"
+                            }
+                        }
+                    }
+                ]
+            }
         }
     },
 
@@ -92,21 +109,7 @@
             }
         },
         "seeAlso": {
-            "type": "array",
-            "items": {
-                "allOf": [
-                    { "$ref": "#/types/class" },
-                    {
-                        "type": "object",
-                        "properties": {
-                            "format": { "$ref": "#/types/format" },
-                            "profile": {
-                                "type": "string"
-                            }
-                        }
-                    }
-                ]
-            }
+            "$ref": "#/types/external"
         },
         "partOf": {
             "type": "array",
@@ -424,6 +427,7 @@
                 { "$ref": "#/types/class" },
                 {
                     "type": "object",
+                    "additionalProperties": false,
                     "properties": {
                         "@context": {
                             "oneOf": [
@@ -441,22 +445,23 @@
                                 }
                             ]
                         },
+                        "id": {},
+                        "label": {},
                         "type": {
                             "type": "string",
                             "pattern": "^Manifest"
                         },
                         "metadata": { "$ref": "#/classes/metadata" },
                         "summary": { "$ref": "#/types/lngString" },
                         "requiredStatement": { "$ref": "#/types/keyValueString" },
+                        "rendering": { "$ref": "#/types/external" },
+                        "service": { "$ref": "#/classes/service" },
+                        "viewingDirection": { "$ref": "#/classes/viewingDirection" },
                         "rights": { "$ref": "#/classes/rights" },
-                        "height": {
-                            "type": "integer"
-                        },
-                        "width": {
-                            "type": "integer"
-                        },
-                        "duration": {
-                            "$ref": "#/types/duration"
+                        "start": {},
+                        "logo": {
+                            "type": "array",
+                            "items": { "$ref": "#/classes/resource" }
                         },
                         "navDate": { "$ref": "#/classes/navDate" },
                         "provider": { "$ref": "#/classes/provider" },

diff --git a/schema/schemavalidator.py b/schema/schemavalidator.py
@@ -58,12 +58,15 @@ def validate(data, version, url):
         # if a oneOf possibility is of type Collection but we have passed a Manifest
         # then its safe to ignore the validation error.
         for err in errors:
+            #print (err)
             if errorParser.isValid(list(err.absolute_schema_path), list(err.absolute_path)):
                 # if it is valid we want a good error message so diagnose which oneOf is 
                 # relevant for the error we've found.
                 if err.absolute_schema_path[-1] == 'oneOf':
                     err = errorParser.diagnoseWhichOneOf(list(err.absolute_schema_path), list(err.absolute_path))
                 relevantErrors.append(err)
+            #else:
+            #    print ('Dismissing schema: {} path: {}'.format(err.absolute_schema_path, err.absolute_path))
             i += 1
         errors = relevantErrors
         errorCount = 1

diff --git a/setup.py b/setup.py
@@ -54,7 +54,8 @@ def run(self):
         ':python_version>="3.0"': ["Pillow>=3.2.0"],
         ':python_version<"3.0"': [
             "Pillow==6.2.2",
-            "zipp==1.2.0"
+            "zipp==1.2.0",
+            "configparser==4.0.2"
         ],
     },
     test_suite="tests",

diff --git a/tests/test_validator.py b/tests/test_validator.py
@@ -247,10 +247,19 @@ def test_version3errors(self):
         response = self.helperRunValidation(v, filename)
         self.helperTestValidationErrors(filename, response, errorPaths)
 
+        filename = 'fixtures/3/old_format_label.json'
+        errorPaths = [
+            '/label',
+            '/label/'
+          #  '/' Validator doesn't pick up extra sequence when it comes across the label error first
+        ]
+        response = self.helperRunValidation(v, filename)
+        self.helperTestValidationErrors(filename, response, errorPaths)
+
 
     def helperTestValidationErrors(self, filename, response, errorPaths):       
         self.assertEqual(response['okay'], 0, 'Expected {} to fail validation but it past.'.format(filename))
-        self.assertEqual(len(response['errorList']), len(errorPaths), 'Expected {} validation errors but found {} for file {}'.format(len(errorPaths), len(response['errorList']), filename))
+        self.assertEqual(len(response['errorList']), len(errorPaths), 'Expected {} validation errors but found {} for file {}\n{}'.format(len(errorPaths), len(response['errorList']), filename, response['errorList']))
 
         for error in response['errorList']:
             foundPath = False