Skip to content

Commit

Permalink
support schemas validation in additionalProperties + remove additiona…
Browse files Browse the repository at this point in the history
…lProperties logic from allof
  • Loading branch information
epaillous committed May 2, 2024
1 parent a430896 commit 7969494
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 33 deletions.
17 changes: 0 additions & 17 deletions lib/openapi_parser/schema_validator/all_of_validator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@ def coerce_and_validate(value, schema, **keyword_args)
end

# if any schema return error, it's not valida all of value
remaining_keys = value.kind_of?(Hash) ? value.keys : []
nested_additional_properties = false
schema.all_of.each do |s|
# We need to store the reference to all of, so we can perform strict check on allowed properties
_coerced, err = validatable.validate_schema(
Expand All @@ -20,24 +18,9 @@ def coerce_and_validate(value, schema, **keyword_args)
:parent_all_of => true,
parent_discriminator_schemas: keyword_args[:parent_discriminator_schemas]
)

if s.type == "object"
remaining_keys -= (s.properties || {}).keys
nested_additional_properties = true if s.additional_properties
else
# If this is not allOf having array of objects inside, but e.g. having another anyOf/oneOf nested
remaining_keys.clear
end

return [nil, err] if err
end

# If there are nested additionalProperites, we allow not defined extra properties and lean on the specific
# additionalProperties validation
if !nested_additional_properties && !remaining_keys.empty?
return [nil, OpenAPIParser::NotExistPropertyDefinition.new(remaining_keys, schema.object_reference)]
end

[value, nil]
end
end
Expand Down
8 changes: 3 additions & 5 deletions lib/openapi_parser/schema_validator/object_validator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ def coerce_and_validate(value, schema, parent_all_of: false, parent_discriminato
coerced, err = if s
remaining_keys.delete(name)
validatable.validate_schema(v, s)
elsif schema.additional_properties != true && schema.additional_properties != false
validatable.validate_schema(v, schema.additional_properties)
else
# TODO: we need to perform a validation based on schema.additional_properties here, if
# additionalProperties are defined
[v, nil]
end

Expand All @@ -41,9 +41,7 @@ def coerce_and_validate(value, schema, parent_all_of: false, parent_discriminato

remaining_keys.delete(discriminator_property_name) if discriminator_property_name

if !remaining_keys.empty? && !parent_all_of && !schema.additional_properties
# If object is nested in all of, the validation is already done in allOf validator. Or if
# additionalProperties are defined, we will validate using that
if !remaining_keys.empty? && !schema.additional_properties
return [nil, OpenAPIParser::NotExistPropertyDefinition.new(remaining_keys, schema.object_reference)]
end
return [nil, OpenAPIParser::NotExistRequiredKey.new(required_set.to_a, schema.object_reference)] unless required_set.empty?
Expand Down
4 changes: 2 additions & 2 deletions spec/data/petstore-with-discriminator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,6 @@ components:
fire_range:
type: integer
format: int64
additionalProperties: false
Hydra:
allOf:
- $ref: '#/components/schemas/DragonBody'
Expand All @@ -222,6 +221,8 @@ components:
head_count:
type: integer
format: int64
mass:
type: integer
additionalProperties:
type: string
DragonBody:
Expand All @@ -233,4 +234,3 @@ components:
type: string
mass:
type: integer
additionalProperties: false
18 changes: 9 additions & 9 deletions spec/openapi_parser/schema_validator/all_of_validator_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,22 +24,20 @@
request_operation.validate_request_body(content_type, body)
end

it 'fails when sending unknown properties' do
it 'works when sending unknown properties' do
body = {
"baskets" => [
{
"name" => "dragon",
"mass" => 10,
"fire_range" => 20,
"speed" => 20
"speed" => 20,
"color" => 'gold'
}
]
}

expect { request_operation.validate_request_body(content_type, body) }.to raise_error do |e|
expect(e).to be_kind_of(OpenAPIParser::NotExistPropertyDefinition)
expect(e.message).to end_with("does not define properties: speed")
end
request_operation.validate_request_body(content_type, body)
end

it 'fails when missing required property' do
Expand Down Expand Up @@ -88,7 +86,7 @@
request_operation.validate_request_body(content_type, body)
end

it 'fails when sending unknown properties of correct type based on additionalProperties' do
it 'fails when sending unknown properties of incorrect type based on additionalProperties' do
body = {
"baskets" => [
{
Expand All @@ -100,8 +98,10 @@
]
}

# TODO for now we don't validate on additionalProperites, but this should fail on speed have to be string
request_operation.validate_request_body(content_type, body)
expect { request_operation.validate_request_body(content_type, body) }.to raise_error do |e|
expect(e).to be_kind_of(OpenAPIParser::ValidateError)
expect(e.message).to end_with("expected string, but received Integer: 20")
end
end

it 'fails when missing required property' do
Expand Down

1 comment on commit 7969494

@geemus
Copy link
Collaborator

@geemus geemus commented on 7969494 Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epaillous Unfortunately this was causing some issues for me in my usage of the library (a number of things that used to work started breaking), so I reverted it for the time being. Since I don't see a related pull request I didn't feel like I had enough context to fully understand what was being addressed here, so if you could provide some details and/or examples that would be great. I don't want to break what you need either, so I think we may need to find some middle ground that supports both use cases. Thanks!

Please sign in to comment.