[#323] Validation of $ref property values should show error on unexpe… #342

ghillairet · 2017-05-31T14:04:38Z

…cted object type

Probably needs more tests.

…cted object type

tfesenko · 2017-05-31T14:28:14Z

@ghillairet ,
This approach does not work for arrays:

I used this spec:

openapi: "3.0.0"
info:
  version: 1.0.0
  title: Swagger Petstore
  license:
    name: MIT
    
paths: {}
                
components:
  schemas:
  
    MyType2:
      type: array
      items:
        $ref: "#/components/schemas/MyType1"
    MyType1:
      required:
        - property
      properties:
        property:
          type: integer
          format: int64

tfesenko · 2017-05-31T14:29:56Z

com.reprezen.swagedit.core/src/com/reprezen/swagedit/core/validation/Validator.java

+
+            if (valueNode != null && type.getPointer() != null && valueNode.getType() != null
+                    && !type.getPointer().equals(valueNode.getType().getPointer())) {
+                errors.add(error(nodeValue, IMarker.SEVERITY_ERROR, Messages.error_invalid_reference_type));


It would be really helpful to provide what type is actually expected. It looks like valueNode.getType().getPointer() can provide this info

ghillairet · 2017-05-31T14:33:08Z

Ok I'll fix the array and add the type in the error.

…cted object type

…into task/323

tfesenko · 2017-06-02T14:38:54Z

com.reprezen.swagedit.core/src/com/reprezen/swagedit/core/validation/Validator.java

+            TypeDefinition type = node.getType();
+
+            AbstractNode nodeValue = node.get(JsonReference.PROPERTY);
+            AbstractNode valueNode = model.find((String) nodeValue.asValue().getValue());


It only works for local references. It throws and exception for external references:

From the same workspace (local external reference via relative path)

Or, remove (accessed via HTTP/HTTPS/any other protocol)

Because it throws an exception which is never processed, the exception is propagated to the top level com.reprezen.swagedit.core.validation.Validator.validate(JsonDocument, URI) and it means that validation errors are not processed at all.

tfesenko · 2017-06-02T14:48:51Z

@ghillairet , this code works well with references to the elements from the same file.

However, I found a severe problem with JSON pointers which are not starting with a "/". So, if such pointer is present in a spec, validation does NOT work at all. You can type anything and it will be accepted by the validator, see my comment #342 (comment)
Examples of such pointers:

VALID REF: References to the elements from the same workspace, e.g. $ref: "uber_schemas.yaml#/components/schemas/PriceEstimate". https://modelsolv.atlassian.net/secure/attachment/33000/Multi-file%20Uber%20%28OAS3%29.zip has good examples.
VALID REF: References to remote resources, e.g. $ref: "https://raw.githubusercontent.com/RepreZen/KaiZen-OpenAPI-Editor/master/com.reprezen.swagedit.openapi3.tests/resources/spec_examples/v3.0/uber.yaml#/components/schemas/PriceEstimate"
INVALID REF: just invalid pointer, like in my JUnit test: $ref: "INVALID". Yes, it is invalid, but it should show a warning, instead of breaking validation.

All these cases are caused by the same line of code. But we should test them (preferably with automated tests) separately after fixing it.
Exception:

java.lang.IllegalArgumentException: Invalid input: JSON Pointer expression must start with '/': "./components/uber_path_products.yaml#/paths/~1products2"
	at com.fasterxml.jackson.core.JsonPointer.compile(JsonPointer.java:122)
	at com.fasterxml.jackson.core.JsonPointer.valueOf(JsonPointer.java:131)
	at com.reprezen.swagedit.core.model.Model.find(Model.java:222)
	at com.reprezen.swagedit.core.validation.Validator.checkReferenceType(Validator.java:305)
	at com.reprezen.swagedit.core.validation.Validator.validateModel(Validator.java:175)
	at com.reprezen.swagedit.core.validation.Validator.validate(Validator.java:118)
	at com.reprezen.swagedit.core.validation.Validator.validate(Validator.java:100)

tfesenko · 2017-06-02T14:55:52Z

I added more tests for invalid refs, so the old tests + new tests cover these variations of invalid refs:

not a pointer
points to a top level element which is not a JSON
pointer to a nested element inside a non-JSON file
points to a JSON of invalid type (covered by initial set of tests)

The tests check that these scenarios don't break the validation

… show error on unexpected object type

…cted object type

ghillairet · 2017-06-08T12:45:39Z

@tfesenko I updated this PR to make your tests pass. The validation of remote refs as also been fixed, and support for links operationId, operationRef and securitySchemes (see #308 , #306 and #347)

ghillairet · 2017-06-08T12:47:25Z

There is still an issue with validation of refs inside the same workspace/project, I'm trying to fix that now.

…cted object type Disable type validation of externals refs.

…cted object type

tfesenko · 2017-06-08T21:10:25Z

com.reprezen.swagedit.core/src/com/reprezen/swagedit/core/validation/Validator.java

            }
        }
        return errors;
    }

+    protected void executeModelValidation(Model model, AbstractNode node, Set<SwaggerError> errors) {


I suggest validateNode as method name

tfesenko · 2017-06-08T21:11:13Z

...edit.openapi3/src/com/reprezen/swagedit/openapi3/assist/contexts/OperationIdContextType.java

-                nodes.add(node);
-            }
-        }
+        final List<AbstractNode> nodes = model.findByType(operationPointer);


tfesenko · 2017-06-08T21:13:24Z

com.reprezen.swagedit.tests/src/com/reprezen/swagedit/validation/ValidatorTest.xtend

+
+		document.set(content)
+		val errors = validator.validate(document, null as URI)		
+		assertEquals(0, errors.size())


tfesenko

Passed code review and QA

[#323] Validation of $ref property values should show error on unexpe…

fac4cf3

…cted object type

ghillairet requested a review from tfesenko May 31, 2017 14:04

ghillairet assigned tfesenko May 31, 2017

ghillairet mentioned this pull request May 31, 2017

OAS3: Validation of $ref property values should show error on unexpected object type #323

Closed

tfesenko reviewed May 31, 2017

View reviewed changes

Merge branch 'master' into task/323

6bb0df9

tfesenko mentioned this pull request May 31, 2017

Tests: SchemaValidationTest can only be run as JUnit Plugin Test, make it a regular JUnit test #343

Closed

ghillairet and others added 7 commits June 1, 2017 11:08

Merge branch 'master' into task/323

a71bbbc

Merge branch 'master' into task/323

13c013b

[#323] Validation of $ref property values should show error on unexpe…

fa90d32

…cted object type

Merge branch 'master' into task/323

7e69150

Merge branch 'task/323' of github.com:RepreZen/KaiZen-OpenAPI-Editor …

da3b970

…into task/323

Merge branch 'master' into task/323

9ffd914

Added a test for invalid pointer in a reference

5061b5a

tfesenko reviewed Jun 2, 2017

View reviewed changes

More tests for invalid references

eef2528

tfesenko mentioned this pull request Jun 2, 2017

OpenAPI v3: Validation of *remote* $ref property values #345

Closed

ghillairet added 2 commits June 7, 2017 16:59

[#345] OpenAPI v3: Validation of *remote* $ref property values should…

f26d00c

… show error on unexpected object type

[#323] Validation of $ref property values should show error on unexpe…

19968da

…cted object type

ghillairet and others added 4 commits June 8, 2017 14:49

Merge branch 'master' into task/323

c151348

[#323] Validation of $ref property values should show error on unexpe…

235a556

…cted object type Disable type validation of externals refs.

[#323] Validation of $ref property values should show error on unexpe…

ee3d787

…cted object type

Merge branch 'master' into task/323

c4dfcd1

tfesenko reviewed Jun 8, 2017

View reviewed changes

tfesenko approved these changes Jun 8, 2017

View reviewed changes

tfesenko added 2 commits June 8, 2017 18:11

#353 - disable test checks depending on fix for #353

5d87d3a

#323 - change severity for unexpected ref type from error to warning

df06378

tfesenko merged commit b923a58 into master Jun 8, 2017

tfesenko deleted the task/323 branch June 8, 2017 22:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#323] Validation of $ref property values should show error on unexpe… #342

[#323] Validation of $ref property values should show error on unexpe… #342

ghillairet commented May 31, 2017

tfesenko commented May 31, 2017

tfesenko May 31, 2017

ghillairet commented May 31, 2017

tfesenko Jun 2, 2017

tfesenko commented Jun 2, 2017

tfesenko commented Jun 2, 2017 •

edited

Loading

ghillairet commented Jun 8, 2017

ghillairet commented Jun 8, 2017

tfesenko Jun 8, 2017

tfesenko Jun 8, 2017

tfesenko Jun 8, 2017

tfesenko left a comment

[#323] Validation of $ref property values should show error on unexpe… #342

[#323] Validation of $ref property values should show error on unexpe… #342

Conversation

ghillairet commented May 31, 2017

tfesenko commented May 31, 2017

tfesenko May 31, 2017

Choose a reason for hiding this comment

ghillairet commented May 31, 2017

tfesenko Jun 2, 2017

Choose a reason for hiding this comment

tfesenko commented Jun 2, 2017

tfesenko commented Jun 2, 2017 • edited Loading

ghillairet commented Jun 8, 2017

ghillairet commented Jun 8, 2017

tfesenko Jun 8, 2017

Choose a reason for hiding this comment

tfesenko Jun 8, 2017

Choose a reason for hiding this comment

tfesenko Jun 8, 2017

Choose a reason for hiding this comment

tfesenko left a comment

Choose a reason for hiding this comment

tfesenko commented Jun 2, 2017 •

edited

Loading