Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to permit validation when schema and data are empty #417

Merged
merged 4 commits into from
May 13, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/committee/schema_validator/hyper_schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def response_validate(status, headers, response, _test_method = false)
data = JSON.parse(full_body) if parse_to_json
end

Committee::SchemaValidator::HyperSchema::ResponseValidator.new(link, validate_success_only: validator_option.validate_success_only).call(status, headers, data)
Committee::SchemaValidator::HyperSchema::ResponseValidator.new(link, validate_success_only: validator_option.validate_success_only, permit_blank_structures: validator_option.permit_blank_structures).call(status, headers, data)
end

def link_exist?
Expand Down
18 changes: 14 additions & 4 deletions lib/committee/schema_validator/hyper_schema/response_validator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@ module Committee
module SchemaValidator
class HyperSchema
class ResponseValidator
attr_reader :validate_success_only
attr_reader :validate_success_only, :permit_blank_structures

def initialize(link, options = {})
@link = link
@validate_success_only = options[:validate_success_only]
@permit_blank_structures = options[:permit_blank_structures]

@validator = JsonSchema::Validator.new(target_schema(link))
end
Expand Down Expand Up @@ -39,9 +40,18 @@ def call(status, headers, data)
return if data == nil
end

if Committee::Middleware::ResponseValidation.validate?(status, validate_success_only) && !@validator.validate(data)
errors = JsonSchema::SchemaError.aggregate(@validator.errors).join("\n")
raise InvalidResponse, "Invalid response.\n\n#{errors}"
if permit_blank_structures && @link.is_a?(Committee::Drivers::OpenAPI2::Link) && !@link.target_schema
return if data.nil?
end

begin
if Committee::Middleware::ResponseValidation.validate?(status, validate_success_only) && !@validator.validate(data)
errors = JsonSchema::SchemaError.aggregate(@validator.errors).join("\n")
raise InvalidResponse, "Invalid response.\n\n#{errors}"
end
rescue => e
raise InvalidResponse, "Invalid response.\n\nschema is undefined" if /undefined method .all_of. for nil/ =~ e.message
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ruby 3.2.2

NoMethodError: undefined method `all_of' for nil:NilClass

ruby 3.3.0

NoMethodError: undefined method `all_of' for nil

ruby head

NoMethodError: undefined method 'all_of' for nil

raise e
end
end

Expand Down
4 changes: 3 additions & 1 deletion lib/committee/schema_validator/option.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ class Option
:optimistic_json,
:validate_success_only,
:parse_response_by_content_type,
:parameter_overwite_by_rails_rule
:parameter_overwite_by_rails_rule,
:permit_blank_structures

# Non-boolean options:
attr_reader :headers_key,
Expand Down Expand Up @@ -46,6 +47,7 @@ def initialize(options, schema, schema_type)
@optimistic_json = options.fetch(:optimistic_json, false)
@parse_response_by_content_type = options.fetch(:parse_response_by_content_type, true)
@parameter_overwite_by_rails_rule = options.fetch(:parameter_overwite_by_rails_rule, true)
@permit_blank_structures = options.fetch(:permit_blank_structures, false)
Copy link
Member

Choose a reason for hiding this comment

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

nits) As for the name of the parameter, how about allow_blank_structures to match the name of the other parameter if you want to allow it?

@allow_form_params = options.fetch(:allow_form_params, true)
@allow_query_params = options.fetch(:allow_query_params, true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you.
I have renamed permit_blank_structures to allow_blank_structures.

ref: 6de7583

Copy link
Member

Choose a reason for hiding this comment

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

Could you add a description of this parameter to the README?

https://github.com/interagent/committee?tab=readme-ov-file#configuration-options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a note to the README : )

[memo]

In OpenAPI 3, the following becomes nil, so the validation of empty response succeeds.
Therefore, this option is expected to be valid only for Hyper-schema and OpenAPI 2.


# Boolean options and have a different value by default
@allow_get_body = options.fetch(:allow_get_body, schema.driver.default_allow_get_body)
Expand Down
11 changes: 11 additions & 0 deletions test/data/openapi2/petstore-expanded.json
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,17 @@
}
}
}
},
"/pets/cat": {
"get": {
"description": "Returns pets which are cats",
"operationId": "find pets which are cats",
"responses": {
"200": {
"description": "empty schema"
Comment on lines +192 to +194
Copy link
Member

Choose a reason for hiding this comment

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

imo) 204 No Content I think the response body is empty as a specification. So why not make the test data follow the common use case?

Suggested change
"responses": {
"200": {
"description": "empty schema"
"responses": {
"204": {
"description": "The resource was deleted successfully."

Refs: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/204#compatibility_notes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my recent experiences, I've noticed the following scenarios:

  1. status_code=200 && empty response
    => This PR addresses this case. While it's technically correct to use status_code=204, there are instances where APIs are operated with 200, and ideally, I'd like to accommodate this case as well.

  2. status_code=204 && empty response
    => While it's being skipped here for 204, it seemed like a better approach to modify it to skip within Committee::SchemaValidator::HyperSchema::ResponseValidator. If it's not an inconvenience, I'm considering addressing this in a separate PR. Could you please share your thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for your detailed explanation!
I understood the intent and thought it make sense. So I agree with your opinion.

}
}
}
}
},
"definitions": {
Expand Down
23 changes: 23 additions & 0 deletions test/middleware/response_validation_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,29 @@ def app
assert_equal 200, last_response.status
end

it "passes through a valid response for OpenAPI when data=nil, target_schema=empty, permit_blank_structures=true" do
@app = new_rack_app("null", {},
permit_blank_structures: true, schema: open_api_2_schema)
get "/api/pets/cat"
assert_equal 200, last_response.status
end

it "invalid responses for OpenAPI when data=nil, target_schema=empty, permit_blank_structures=false" do
@app = new_rack_app("null", {},
permit_blank_structures: false, schema: open_api_2_schema)
get "/api/pets/cat"
assert_equal 500, last_response.status
assert_match(/Invalid response/i, last_response.body)
end

it "passes through a valid response for OpenAPI when data=nil, target_schema=present, permit_blank_structures=true" do
@app = new_rack_app("null", {},
permit_blank_structures: true, schema: open_api_2_schema)
get "/api/pets/dog"
assert_equal 500, last_response.status
assert_match(/nil is not an array/i, last_response.body)
end

it "detects an invalid response for OpenAPI" do
@app = new_rack_app("{_}", {}, schema: open_api_2_schema)
get "/api/pets"
Expand Down