Perform a strict check on object properties #33

Ladas · 2019-05-17T13:21:37Z

Perform a strict check on properties

We need to throw a validation error if property being send is
not defined. If additionalProperties are defined, we skip the
validation for now, with TODO added.

In a case of composing schema using allOf, the property needs
to be defined at least in 1 of te allOf items, as stated in:

https://swagger.io/docs/specification/data-models/oneof-anyof-allof-not/#allof

Depends on:

Add support for discriminator #32 (affecting the spec here, so commits of this PR added as dependency and will be rebased when merged)

ota42y · 2019-05-20T08:18:25Z

lib/openapi_parser/schema_validators/all_of_validator.rb

@@ -7,6 +7,9 @@ class AllOfValidator < Base
    def coerce_and_validate(value, schema)
      # if any schema return error, it's not valida all of value
      schema.all_of.each do |s|
+        # We need to store the reference to all of, so we can perform strict check on allowed properties
+        s.parent_all_of = schema.all_of


I think Schema object should be immutable.
And schema's parent isn't always the same (e.g, when we use reference, one object have many parent)
so it becomes a problem when multi-threading.

NotExistPropertyDefinition error have property name which not defined in s.
I think this validator validate all schemas in all_of section and collect not defined keys.
And returns error if there is property name not included in all the definitions.

This is sample code

remain_keys = value.keys schema.all_of.each do |s| _coerced, errs = validatable.validate_schema(value, s, early_return=false) errors_key = get_errors_key(errs) remain_keys = remain_keys & errors_key end raise 'not exist key' if remain_keys

But this sample code dosen't work because validate_schema method return only one error.
So I think we should add early_return option which return all validate errors.

right, doing it here with schema.all_of.each would solve only a part of the issue. Because there is a normal object validation, which changes if the object is under allOf https://github.com/ota42y/openapi_parser/pull/33/files#diff-0662b153c9d300d215ae97087f9fa776R18

So I can do the validation here, but I will still need to pass something to this nested validation, so we can skip it at least. So maybe passing another argument to validatable.validate_schema(value, s) ?

so I moved the parent_all_of to argument fde88c0 and now trying to figure out how to move it up and aggregate, but still it won;t be perfect, because it can throw also OpenAPIParser::NotExistRequiredKey and we can't aggregate 2 different errors into one.

@ota42y ok, it's done

We need to throw a validation error if property being send is not defined. If additionalProperties are defined, we skip the validation for now, with TODO added. In a case of composing schema using allOf, the property needs to be defined at least in 1 of te allOf items, as stated in: https://swagger.io/docs/specification/data-models/oneof-anyof-allof-not/#allof

Aggregating either for the whole allOf block or the object.

ota42y

Thank you very much! I think almost good!
I have some questions, so I want to know more about it

lib/openapi_parser/schema_validators/object_validator.rb

lib/openapi_parser/schema_validators/all_of_validator.rb

Supporting attribute that can be nil or any of the refs and adding specs for the validation.

Ladas · 2019-05-27T15:18:07Z

@ota42y last 2 commits implement the 2 requests you had :-)

The past format of nullable was not validatable, changing according to new validations and specs in ota42y/openapi_parser#33

ota42y

Almost ok!
Just one simple fix please.

ota42y · 2019-05-28T11:32:28Z

lib/openapi_parser/schema_validators/object_validator.rb

      return [nil, OpenAPIParser::NotExistRequiredKey.new(required_set.to_a, schema.object_reference)] unless required_set.empty?

      value.merge!(coerced_values.to_h) if @coerce_value

      [value, nil]
    end
+
+    def validate_using_additional_properties(schema, name, v)
+      # TODO: we need to perform a validation based on schema.additional_properties here


I think we don't need implement this method in this PR.
Because this PR is for adding strict check feature and we can add this feature later.

So I will merge this and release a new version.
(If you have a problem, please tell me)

ok, removing the method for now, but keeping the todo, so we know where to touch (and we can search for 'additionalProperties' string)

ota42y · 2019-05-28T11:36:22Z

lib/openapi_parser/schemas/base.rb

@@ -6,6 +6,9 @@ class Base

    attr_reader :parent, :raw_schema, :object_reference, :root

+    # Parent allOf reference, if schema is nested in allOf
+    attr_accessor :parent_all_of


I think this code is unnecessary, so please delete it.

ah, right, I forgot it there

ota42y

Thank you very much for adding great features!!!

ota42y · 2019-05-28T11:54:52Z

lib/openapi_parser/schema_validators/object_validator.rb

@@ -17,8 +17,9 @@ def coerce_and_validate(value, schema, parent_all_of: false)
                         remaining_keys.delete(name)
                         validatable.validate_schema(v, s)
                       else
-                         # Property is not defined, try validate using additionalProperties
-                         validate_using_additional_properties(schema, name, v)
+                         # TODO: we need to perform a validation based on schema.additional_properties here, if


Ladas · 2019-05-28T11:57:33Z

@ota42y thank you for the great reviews :-)

ota42y · 2019-06-01T12:35:53Z

I just released v.0.3.0 👍

ota42y self-requested a review May 20, 2019 06:33

ota42y requested changes May 20, 2019

View reviewed changes

Ladas added 3 commits May 20, 2019 15:56

Adjust existing specs for strict properites validation

7a4f947

Add spec for object properties

da388f8

Ladas force-pushed the perform_a_strict_check_on_object_properties branch from 4851245 to da388f8 Compare May 20, 2019 13:56

Ladas added 2 commits May 21, 2019 14:54

Pass parent_all_of as keyword argument

50875db

Aggregate NotExistPropertyDefinition exception

7615724

Aggregating either for the whole allOf block or the object.

Ladas force-pushed the perform_a_strict_check_on_object_properties branch from fde88c0 to 7615724 Compare May 21, 2019 17:39

Ladas added 2 commits May 21, 2019 20:19

Allow keyword args for anyOf and oneOf

a0e9f7b

Do validation of extra keys only for objects nested in allOf

cb0aabc

Ladas changed the title ~~[WIP] Perform a strict check on object properties~~ Perform a strict check on object properties May 21, 2019

Ladas mentioned this pull request May 21, 2019

Validation of request parameters/body according to openapi v3 json RedHatInsights/insights-api-common-rails#67

Closed

ota42y requested changes May 25, 2019

View reviewed changes

lib/openapi_parser/schema_validators/object_validator.rb Outdated Show resolved Hide resolved

lib/openapi_parser/schema_validators/all_of_validator.rb Show resolved Hide resolved

Ladas added 2 commits May 27, 2019 15:52

Move conditions near NotExistPropertyDefinition

adb04f7

Support nullable alternative attribute

f75efa0

Supporting attribute that can be nil or any of the refs and adding specs for the validation.

Ladas added a commit to Ladas/topological_inventory-ingress_api that referenced this pull request May 28, 2019

Change format of nullable

ed43c96

The past format of nullable was not validatable, changing according to new validations and specs in ota42y/openapi_parser#33

ota42y requested changes May 28, 2019

View reviewed changes

Ladas added 3 commits May 28, 2019 13:44

Remove unused attr_accessor

6728b99

Remove method for validating additionalProperties for now

d06b0e6

Add class checks to spec

40c7665

ota42y approved these changes May 28, 2019

View reviewed changes

ota42y merged commit d210f77 into ota42y:master May 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perform a strict check on object properties #33

Perform a strict check on object properties #33

Ladas commented May 17, 2019 •

edited

Loading

ota42y May 20, 2019

ota42y May 20, 2019

ota42y May 20, 2019

Ladas May 20, 2019

Ladas May 20, 2019

Ladas May 21, 2019

ota42y left a comment

Ladas commented May 27, 2019

ota42y left a comment

ota42y May 28, 2019

Ladas May 28, 2019 •

edited

Loading

ota42y May 28, 2019

Ladas May 28, 2019

Ladas May 28, 2019

ota42y left a comment

ota42y May 28, 2019

Ladas commented May 28, 2019

ota42y commented Jun 1, 2019

Perform a strict check on object properties #33

Perform a strict check on object properties #33

Conversation

Ladas commented May 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ota42y left a comment

Choose a reason for hiding this comment

Ladas commented May 27, 2019

ota42y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladas May 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ota42y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladas commented May 28, 2019

ota42y commented Jun 1, 2019

Ladas commented May 17, 2019 •

edited

Loading

Ladas May 28, 2019 •

edited

Loading