Merge pull request #106 from IIIF/99_Ed_error

Handling multiple errors
IIIF · Oct 7, 2020 · 32afb6b · 32afb6b
2 parents c2c6def + ff503d8
commit 32afb6b
Show file tree

Hide file tree

Showing 5 changed files with 78 additions and 19 deletions.
diff --git a/fixtures/3/rights_lang_issues.json b/fixtures/3/rights_lang_issues.json
@@ -0,0 +1 @@
+{"@context":["http://www.w3.org/ns/anno.jsonld","http://iiif.io/api/presentation/3/context.json"],"id":"https://www.rpm-api.io/records/5e78f06706f6190017c039f9?_format=iiif","type":"Manifest","items":[{"id":"https://www.rpm-api.io/records/5e78f06706f6190017c039f9/canvas/0","type":"Canvas","items":[{"id":"https://www.rpm-api.io/records/5e78f06706f6190017c039f9/canvas/0/annotationpage/0","type":"AnnotationPage","items":[{"id":"https://www.rpm-api.io/records/5e78f06706f6190017c039f9/canvas/0/annotation/0","type":"Annotation","motivation":"painting","body":{"id":"https://iiif.rpm-api.io/iiif/2/d300bde0b6954976b3a9534160768aa6/full/max/0/default.jpg","type":"Image","format":"image/jpeg","label":{"@none":["Map of Brighthelmston"]},"service":[{"id":"https://iiif.rpm-api.io/iiif/2/d300bde0b6954976b3a9534160768aa6","type":"ImageService2","profile":"http://iiif.io/api/image/2/level2.json"}]},"target":"https://www.rpm-api.io/records/5e78f06706f6190017c039f9/canvas/0"}]}],"label":{"@none":["Map of Brighthelmston"]},"thumbnail":[{"id":"https://iiif.rpm-api.io/iiif/2/d300bde0b6954976b3a9534160768aa6/full/90,/0/default.jpg","type":"Image"}]}],"label":{"@none":["Map of Brighthelmston"]},"metadata":[{"label":{"@none":["Title"]},"value":{"@none":["Map of Brighthelmston"]}},{"label":{"@none":["Department"]},"value":{"@none":["Fine Art"]}},{"label":{"@none":["Category"]},"value":{"@none":["Coloured Lithograph"]}},{"label":{"@none":["Creator"]},"value":{"@none":["Yeakell; Gardner; Arthur Wallis"]}},{"label":{"@none":["Place Created"]},"value":{"@none":["Brighton"]}},{"label":{"@none":["Date Created"]},"value":{"@none":["1779"]}},{"label":{"@none":["Materials"]},"value":{"@none":["Paper"]}},{"label":{"@none":["Description"]},"value":{"@none":[" \tThis is a coloured version which has been mounted. It shows the Old Town of Brighton and is annotated and shows the gardens at the back of houses and the open spaces and market gardens in some detail. At the top and bottom of the map are two engraved illustrations one of which is of the town from the sea. The references mention all the main buildigs. "]}},{"label":{"@none":["Licence"]},"value":{"@none":["https://creativecommons.org/publicdomain/zero/1.0/"]}},{"label":{"@none":["Accession Id"]},"value":{"@none":["FATMP000096"]}}],"rights":"https://creativecommons.org/publicdomain/zero/1.0/"}
diff --git a/iiif-presentation-validator.py b/iiif-presentation-validator.py
@@ -9,6 +9,7 @@
 from gzip import GzipFile
 from io import BytesIO
 from jsonschema.exceptions import ValidationError, SchemaError
+import traceback    
 
 try:
     # python3
@@ -73,14 +74,17 @@ def check_manifest(self, data, version, url=None, warnings=[]):
                     'received': data,
                     'okay': 0,
                     'error': str(e),
-                    'url': url
+                    'url': url,
+                    'warnings': []
                 }
             except Exception as e:    
+                traceback.print_exc()
                 infojson = {
                     'received': data,
                     'okay': 0,
                     'error': 'Presentation Validator bug: "{}". Please create a <a href="https://github.com/IIIF/presentation-validator/issues">Validator Issue</a>, including a link to the manifest.'.format(e),
-                    'url': url
+                    'url': url,
+                    'warnings': []
                 }
 
         else:

diff --git a/schema/error_processor.py b/schema/error_processor.py
@@ -92,7 +92,7 @@ def diagnoseWhichOneOf(self, error_path, IIIFJsonPath):
                 print(err)
                 raise
 
-            # One of the oneOf possibilities is a reference to anouther part of the schema
+            # One of the oneOf possibilities is a reference to another part of the schema
             # this won't bother the validator but will complicate the diagnoise so replace
             # it with the actual schema json (and copy all references)
             if isinstance(possibility, dict) and "$ref" in possibility:
@@ -116,12 +116,19 @@ def diagnoseWhichOneOf(self, error_path, IIIFJsonPath):
                         # its not another oneOf error
                         if addErrors:
                             # if error is also a oneOf then diagnoise again
-                            if err.absolute_schema_path[-1] == 'oneOf' and err.absolute_schema_path != error_path:
+                            if err.absolute_schema_path[-1] == 'oneOf' and err.absolute_schema_path != error_path and 'rights' not in err.absolute_schema_path:
                                 error_path.append(oneOfIndex) # this is is related to one of the original oneOfs at index oneOfIndex
                                 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
+                                result = (self.diagnoseWhichOneOf(error_path, IIIFJsonPath)) # so recursivly discovery real error
+
+                                if isinstance(result, ValidationError):    
+                                    store_errs.append(result)
+                                else:
+                                    store_errs.extend(result)
+
                         #print ('would add: {} by addErrors is {}'.format(err.message, addErrors))
-                        store_errs.append(err)
+                            else:
+                                store_errs.append(err)
                 # if All errors are relevant to the current type add them to the list        
                 if addErrors:
                     valid_errors += store_errs
@@ -131,10 +138,12 @@ def diagnoseWhichOneOf(self, error_path, IIIFJsonPath):
             # this may hide errors as we are only selecting the first one but better to get one to work on and then can re-run
             # Also need to convert back the error paths to the full path
             error_path.reverse()
-            valid_errors[0].absolute_schema_path.extendleft(error_path)
             IIIFJsonPath.reverse()
-            valid_errors[0].absolute_path.extendleft(IIIFJsonPath)
-            return valid_errors[0] 
+            for error in valid_errors:
+                error.absolute_schema_path.extendleft(error_path)
+                error.absolute_path.extendleft(IIIFJsonPath)
+            #    print ('Err {}, path {}'.format(error.message, error.path))
+            return valid_errors
         else:
             # Failed to find the source of the error so most likely its a problem with the type
             # and it didn't match any of the possible oneOf types
@@ -171,10 +180,14 @@ def getSchemaPortion(self, schemaPath):
         """
         schemaEl = self.schema
         for pathPart in schemaPath:
-            if isinstance(schemaEl[pathPart], dict) and "$ref" in schemaEl[pathPart]:
-                schemaEl = self.resolver.resolve(schemaEl[pathPart]['$ref'])[1]
-            else:
-                schemaEl = schemaEl[pathPart]
+            try:
+                if isinstance(schemaEl[pathPart], dict) and "$ref" in schemaEl[pathPart]:
+                    schemaEl = self.resolver.resolve(schemaEl[pathPart]['$ref'])[1]
+                else:
+                    schemaEl = schemaEl[pathPart]
+            except KeyError as error:
+               # print (schemaEl)
+                raise KeyError
 
         return schemaEl    
 
@@ -218,6 +231,7 @@ def parse(self, error_path, schemaEl, iiif_asset, IIIFJsonPath, parent=None, jso
             return self.parse(error_path, self.resolver.resolve(schemaEl['$ref'])[1], iiif_asset, IIIFJsonPath, parent, jsonPath)
 
 
+        #print ("Path: {} ".format(error_path))
         pathEl = error_path.pop(0)
         # Check current type to see if its a match
         if pathEl == 'type' and parent == 'properties':
@@ -242,7 +256,7 @@ def parse(self, error_path, schemaEl, iiif_asset, IIIFJsonPath, parent=None, jso
                 return False
         # This is the case where additionalProperties has falied but need to check
         # if the type didn't match anyway
-        elif pathEl == 'additionalProperties' and 'type' in schemaEl['properties'] and 'pattern' in schemaEl['properties']['type']:
+        elif pathEl == 'additionalProperties' and 'properties' in schemaEl and 'type' in schemaEl['properties'] and 'pattern' in schemaEl['properties']['type']:
             value = schemaEl['properties']['type']['pattern']
             if not self.isTypeMatch(jsonPath + '.type', iiif_asset, value, IIIFJsonPath):
                 return False

diff --git a/schema/schemavalidator.py b/schema/schemavalidator.py
@@ -58,17 +58,26 @@ 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)
+                if isinstance(err, ValidationError):    
+                    relevantErrors.append(err)
+                else:
+                    relevantErrors.extend(err)
             #else:
             #    print ('Dismissing schema: {} path: {}'.format(err.absolute_schema_path, err.absolute_path))
             i += 1
-        errors = relevantErrors
+        # Remove dupes    
+        seen_titles = set()    
+        errors = []
+        for errorDup in relevantErrors:
+            errorPath = errorParser.pathToJsonPath(errorDup.path)
+            if errorPath not in seen_titles:
+                errors.append(errorDup)
+                seen_titles.add(errorPath)
         errorCount = 1
         # Now create some useful messsages to pass on
         for err in errors:

diff --git a/tests/test_validator.py b/tests/test_validator.py
@@ -251,12 +251,43 @@ def test_version3errors(self):
         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 test_lang_rights(self):
+        v = val_mod.Validator()
+
+        filename = 'fixtures/3/rights_lang_issues.json'
+        errorPaths = [
+            '/label',
+            '/items[0]/label[0]',
+            '/items[0]/',
+            '/rights',
+            '/metadata[0]/label/',
+            '/metadata[0]/value/',
+            '/metadata[1]/label/',
+            '/metadata[1]/value/',
+            '/metadata[2]/label/',
+            '/metadata[2]/value/',
+            '/metadata[3]/label/',
+            '/metadata[3]/value/',
+            '/metadata[4]/label/',
+            '/metadata[4]/value/',
+            '/metadata[5]/label/',
+            '/metadata[5]/value/',
+            '/metadata[6]/label/',
+            '/metadata[6]/value/',
+            '/metadata[7]/label/',
+            '/metadata[7]/value/',
+            '/metadata[8]/label/',
+            '/metadata[8]/value/',
+            '/metadata[9]/label/',
+            '/metadata[9]/value/'
+        ]
+        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))