From 6f1cd4b6c5dbd86fc4f6b8e640ccc39a0c43c394 Mon Sep 17 00:00:00 2001 From: ota42y Date: Sun, 23 May 2021 12:26:01 +0900 Subject: [PATCH] request unpacker refactoring --- lib/committee/drivers/open_api_2/driver.rb | 1 - lib/committee/request_unpacker.rb | 67 ++++---- .../schema_validator/hyper_schema.rb | 35 ++-- lib/committee/schema_validator/open_api_3.rb | 25 ++- .../data/openapi2/petstore-expanded-form.json | 116 +++++++++++++ test/data/openapi3/normal.yaml | 7 +- .../request_validation_open_api_3_test.rb | 15 ++ test/middleware/request_validation_test.rb | 12 ++ test/request_unpacker_test.rb | 158 ++++-------------- test/test_helper.rb | 12 ++ 10 files changed, 257 insertions(+), 191 deletions(-) create mode 100644 test/data/openapi2/petstore-expanded-form.json diff --git a/lib/committee/drivers/open_api_2/driver.rb b/lib/committee/drivers/open_api_2/driver.rb index 40d8f45f..a7ef0a70 100644 --- a/lib/committee/drivers/open_api_2/driver.rb +++ b/lib/committee/drivers/open_api_2/driver.rb @@ -156,7 +156,6 @@ def parse_routes!(data, schema, store) methods.each do |method, link_data| method = method.upcase - link = Link.new link.enc_type = schema.consumes link.href = href diff --git a/lib/committee/request_unpacker.rb b/lib/committee/request_unpacker.rb index a2df1900..fb17fc0f 100644 --- a/lib/committee/request_unpacker.rb +++ b/lib/committee/request_unpacker.rb @@ -27,61 +27,64 @@ def indifferent_params(object) end end - def initialize(request, options={}) - @request = request - + def initialize(options={}) @allow_form_params = options[:allow_form_params] @allow_get_body = options[:allow_get_body] @allow_query_params = options[:allow_query_params] - @coerce_form_params = options[:coerce_form_params] @optimistic_json = options[:optimistic_json] - @schema_validator = options[:schema_validator] end - def call + # reutrn params and is_form_params + def unpack_request_params(request) # if Content-Type is empty or JSON, and there was a request body, try to # interpret it as JSON - params = if !@request.media_type || @request.media_type =~ %r{application/(?:.*\+)?json} - parse_json + params = if !request.media_type || request.media_type =~ %r{application/(?:.*\+)?json} + parse_json(request) elsif @optimistic_json begin - parse_json + parse_json(request) rescue JSON::ParserError nil end end - params = if params - params - elsif @allow_form_params && %w[application/x-www-form-urlencoded multipart/form-data].include?(@request.media_type) + return [params, false] if params + + if @allow_form_params && %w[application/x-www-form-urlencoded multipart/form-data].include?(request.media_type) # Actually, POST means anything in the request body, could be from # PUT or PATCH too. Silly Rack. - p = @request.POST + return [request.POST, true] if request.POST + end - @schema_validator.coerce_form_params(p) if @coerce_form_params + [{}, false] + end - p - else - {} - end + def unpack_query_params(request) + @allow_query_params ? self.class.indifferent_params(request.GET) : {} + end - if @allow_query_params - [self.class.indifferent_params(@request.GET).merge(params), headers] - else - [params, headers] + def unpack_headers(request) + env = request.env + base = env.keys.grep(/HTTP_/).inject({}) do |headers, key| + headerized_key = key.gsub(/^HTTP_/, '').gsub(/_/, '-') + headers[headerized_key] = env[key] + headers end + + base['Content-Type'] = env['CONTENT_TYPE'] if env['CONTENT_TYPE'] + base end private - def parse_json - return nil if @request.request_method == "GET" && !@allow_get_body + def parse_json(request) + return nil if request.request_method == "GET" && !@allow_get_body - body = @request.body.read + body = request.body.read # if request body is empty, we just have empty params return nil if body.length == 0 - @request.body.rewind + request.body.rewind hash = JSON.parse(body) # We want a hash specifically. '42', 42, and [42] will all be # decoded properly, but we can't use them here. @@ -91,17 +94,5 @@ def parse_json end self.class.indifferent_params(hash) end - - def headers - env = @request.env - base = env.keys.grep(/HTTP_/).inject({}) do |headers, key| - headerized_key = key.gsub(/^HTTP_/, '').gsub(/_/, '-') - headers[headerized_key] = env[key] - headers - end - - base['Content-Type'] = env['CONTENT_TYPE'] if env['CONTENT_TYPE'] - base - end end end diff --git a/lib/committee/schema_validator/hyper_schema.rb b/lib/committee/schema_validator/hyper_schema.rb index 72b92a60..c2d2a75b 100644 --- a/lib/committee/schema_validator/hyper_schema.rb +++ b/lib/committee/schema_validator/hyper_schema.rb @@ -50,12 +50,6 @@ def link_exist? !link.nil? end - def coerce_form_params(parameter) - return unless link_exist? - return unless link.schema - Committee::SchemaValidator::HyperSchema::StringParamsCoercer.new(parameter, link.schema).call! - end - private def coerce_path_params @@ -73,15 +67,26 @@ def coerce_query_params(request) end def request_unpack(request) - request.env[validator_option.params_key], request.env[validator_option.headers_key] = Committee::RequestUnpacker.new( - request, - allow_form_params: validator_option.allow_form_params, - allow_get_body: validator_option.allow_get_body, - allow_query_params: validator_option.allow_query_params, - coerce_form_params: validator_option.coerce_form_params, - optimistic_json: validator_option.optimistic_json, - schema_validator: self - ).call + unpacker = Committee::RequestUnpacker.new( + allow_form_params: validator_option.allow_form_params, + allow_get_body: validator_option.allow_get_body, + allow_query_params: validator_option.allow_query_params, + optimistic_json: validator_option.optimistic_json, + ) + + query_param = unpacker.unpack_query_params(request) + request_param, is_form_params = unpacker.unpack_request_params(request) + + coerce_form_params(request_param) if validator_option.coerce_form_params && is_form_params + request.env[validator_option.params_key] = query_param.merge(request_param) + + request.env[validator_option.headers_key] = unpacker.unpack_headers(request) + end + + def coerce_form_params(parameter) + return unless link_exist? + return unless link.schema + Committee::SchemaValidator::HyperSchema::StringParamsCoercer.new(parameter, link.schema).call! end def request_schema_validation(request) diff --git a/lib/committee/schema_validator/open_api_3.rb b/lib/committee/schema_validator/open_api_3.rb index fd73f6a2..a2a2bcad 100644 --- a/lib/committee/schema_validator/open_api_3.rb +++ b/lib/committee/schema_validator/open_api_3.rb @@ -50,10 +50,6 @@ def link_exist? !@operation_object.nil? end - def coerce_form_params(_parameter) - # Empty because request_schema_validation checks and coerces - end - private attr_reader :validator_option @@ -74,15 +70,18 @@ def header(request) end def request_unpack(request) - request.env[validator_option.params_key], request.env[validator_option.headers_key] = Committee::RequestUnpacker.new( - request, - allow_form_params: validator_option.allow_form_params, - allow_get_body: validator_option.allow_get_body, - allow_query_params: validator_option.allow_query_params, - coerce_form_params: validator_option.coerce_form_params, - optimistic_json: validator_option.optimistic_json, - schema_validator: self - ).call + unpacker = Committee::RequestUnpacker.new( + allow_form_params: validator_option.allow_form_params, + allow_get_body: validator_option.allow_get_body, + allow_query_params: validator_option.allow_query_params, + optimistic_json: validator_option.optimistic_json, + ) + + query_param = unpacker.unpack_query_params(request) + request_param, is_form_params = unpacker.unpack_request_params(request) + request.env[validator_option.params_key] = query_param.merge(request_param) + + request.env[validator_option.headers_key] = unpacker.unpack_headers(request) end def copy_coerced_data_to_query_hash(request) diff --git a/test/data/openapi2/petstore-expanded-form.json b/test/data/openapi2/petstore-expanded-form.json new file mode 100644 index 00000000..53b99a07 --- /dev/null +++ b/test/data/openapi2/petstore-expanded-form.json @@ -0,0 +1,116 @@ +{ + "swagger": "2.0", + "info": { + "version": "1.0.0", + "title": "Swagger Petstore", + "description": "A sample API that uses a petstore as an example to demonstrate features in the swagger-2.0 specification", + "termsOfService": "http://swagger.io/terms/", + "contact": { + "name": "Swagger API Team", + "email": "foo@example.com", + "url": "http://madskristensen.net" + }, + "license": { + "name": "MIT", + "url": "http://github.com/gruntjs/grunt/blob/master/LICENSE-MIT" + } + }, + "host": "petstore.swagger.io", + "basePath": "/api", + "schemes": [ + "http" + ], + "consumes": [ + "application/x-www-form-urlencoded" + ], + "produces": [ + "application/x-www-form-urlencoded" + ], + "paths": { + "/pets": { + "post": { + "description": "Creates a new pet in the store. Duplicates are allowed", + "operationId": "addPet", + "parameters": [ + { + "name": "pet", + "in": "body", + "description": "Pet to add to the store", + "required": true, + "schema": { + "$ref": "#/definitions/NewPet" + } + } + ], + "responses": { + "200": { + "description": "pet response", + "schema": { + "$ref": "#/definitions/Pet" + } + }, + "default": { + "description": "unexpected error", + "schema": { + "$ref": "#/definitions/Error" + } + } + } + } + } + }, + "definitions": { + "Pet": { + "type": "object", + "allOf": [ + { + "$ref": "#/definitions/NewPet" + }, + { + "required": [ + "id" + ], + "properties": { + "id": { + "type": "integer", + "format": "int64" + } + } + } + ] + }, + "NewPet": { + "type": "object", + "required": [ + "name" + ], + "properties": { + "name": { + "type": "string" + }, + "tag": { + "type": "string" + }, + "age": { + "type": "integer" + } + } + }, + "Error": { + "type": "object", + "required": [ + "code", + "message" + ], + "properties": { + "code": { + "type": "integer", + "format": "int32" + }, + "message": { + "type": "string" + } + } + } + } +} \ No newline at end of file diff --git a/test/data/openapi3/normal.yaml b/test/data/openapi3/normal.yaml index f0c5c0ac..42f77540 100644 --- a/test/data/openapi3/normal.yaml +++ b/test/data/openapi3/normal.yaml @@ -313,7 +313,12 @@ paths: enum: - 1.0 - 2.1 - + application/x-www-form-urlencoded: + schema: + type: object + properties: + integer: + type: integer responses: '200': description: success diff --git a/test/middleware/request_validation_open_api_3_test.rb b/test/middleware/request_validation_open_api_3_test.rb index cb339318..fad6c118 100644 --- a/test/middleware/request_validation_open_api_3_test.rb +++ b/test/middleware/request_validation_open_api_3_test.rb @@ -426,6 +426,21 @@ def app assert_equal 204, last_response.status end + it "unpacker test" do + @app = new_rack_app_with_lambda(lambda do |env| + assert_equal env['committee.params']['integer'], 42 + assert_equal env['committee.params'][:integer], 42 + # overwrite by request body... + assert_equal env['rack.request.query_hash']['integer'], 42 + # assert_equal env['rack.request.query_hash'][:integer], 42 + [204, {}, []] + end, schema: open_api_3_schema, raise: true) + + header "Content-Type", "application/x-www-form-urlencoded" + post '/validate?integer=21', "integer=42" + assert_equal 204, last_response.status + end + it "OpenAPI3 raise not support method" do @app = new_rack_app(schema: open_api_3_schema) diff --git a/test/middleware/request_validation_test.rb b/test/middleware/request_validation_test.rb index cf31b8a5..26b0d32c 100644 --- a/test/middleware/request_validation_test.rb +++ b/test/middleware/request_validation_test.rb @@ -435,6 +435,18 @@ def app assert_equal 200, last_response.status end + it "aacorce form params" do + check_parameter = lambda { |env| + assert_equal 3, env['committee.params']['age'] + [200, {}, []] + } + + @app = new_rack_app_with_lambda(check_parameter, schema: open_api_2_form_schema, raise: true, allow_form_params: true, coerce_form_params: true) + header "Content-Type", "application/x-www-form-urlencoded" + post "/api/pets", "age=3&name=ab" + assert_equal 200, last_response.status + end + it "detects an invalid request for OpenAPI" do @app = new_rack_app(schema: open_api_2_schema) get "/api/pets?limit=foo", nil, { "HTTP_AUTH_TOKEN" => "xxx" } diff --git a/test/request_unpacker_test.rb b/test/request_unpacker_test.rb index 78a7a46a..e51d08a3 100644 --- a/test/request_unpacker_test.rb +++ b/test/request_unpacker_test.rb @@ -9,8 +9,8 @@ "rack.input" => StringIO.new('{"x":"y"}'), } request = Rack::Request.new(env) - params, _ = Committee::RequestUnpacker.new(request).call - assert_equal({ "x" => "y" }, params) + unpacker = Committee::RequestUnpacker.new + assert_equal([{ "x" => "y" }, false], unpacker.unpack_request_params(request)) end it "unpacks JSON on Content-Type: application/vnd.api+json" do @@ -19,8 +19,8 @@ "rack.input" => StringIO.new('{"x":"y"}'), } request = Rack::Request.new(env) - params, _ = Committee::RequestUnpacker.new(request).call - assert_equal({ "x" => "y" }, params) + unpacker = Committee::RequestUnpacker.new + assert_equal([{ "x" => "y" }, false], unpacker.unpack_request_params(request)) end it "unpacks JSON on no Content-Type" do @@ -28,8 +28,8 @@ "rack.input" => StringIO.new('{"x":"y"}'), } request = Rack::Request.new(env) - params, _ = Committee::RequestUnpacker.new(request).call - assert_equal({ "x" => "y" }, params) + unpacker = Committee::RequestUnpacker.new + assert_equal([{ "x" => "y" }, false], unpacker.unpack_request_params(request)) end it "doesn't unpack JSON on application/x-ndjson" do @@ -38,8 +38,8 @@ "rack.input" => StringIO.new('{"x":"y"}\n{"a":"b"}'), } request = Rack::Request.new(env) - params, _ = Committee::RequestUnpacker.new(request).call - assert_equal({}, params) + unpacker = Committee::RequestUnpacker.new + assert_equal([{}, false], unpacker.unpack_request_params(request)) end it "doesn't unpack JSON under other Content-Types" do @@ -49,8 +49,8 @@ "rack.input" => StringIO.new('{"x":"y"}'), } request = Rack::Request.new(env) - params, _ = Committee::RequestUnpacker.new(request).call - assert_equal({}, params) + unpacker = Committee::RequestUnpacker.new + assert_equal([{}, false], unpacker.unpack_request_params(request)) end end @@ -61,8 +61,8 @@ "rack.input" => StringIO.new('{"x":"y"}'), } request = Rack::Request.new(env) - params, _ = Committee::RequestUnpacker.new(request, optimistic_json: true).call - assert_equal({ "x" => "y" }, params) + unpacker = Committee::RequestUnpacker.new(optimistic_json: true) + assert_equal([{ "x" => "y" }, false], unpacker.unpack_request_params(request)) end end @@ -73,8 +73,8 @@ "rack.input" => StringIO.new('x=y&foo=42'), } request = Rack::Request.new(env) - params, _ = Committee::RequestUnpacker.new(request, optimistic_json: true).call - assert_equal({}, params) + unpacker = Committee::RequestUnpacker.new(optimistic_json: true) + assert_equal([{}, false], unpacker.unpack_request_params(request)) end end @@ -84,8 +84,8 @@ "rack.input" => StringIO.new(""), } request = Rack::Request.new(env) - params, _ = Committee::RequestUnpacker.new(request).call - assert_equal({}, params) + unpacker = Committee::RequestUnpacker.new + assert_equal([{}, false], unpacker.unpack_request_params(request)) end it "doesn't unpack form params" do @@ -95,8 +95,8 @@ "rack.input" => StringIO.new("x=y"), } request = Rack::Request.new(env) - params, _ = Committee::RequestUnpacker.new(request).call - assert_equal({}, params) + unpacker = Committee::RequestUnpacker.new + assert_equal([{}, false], unpacker.unpack_request_params(request)) end end @@ -107,96 +107,8 @@ "rack.input" => StringIO.new("x=y"), } request = Rack::Request.new(env) - params, _ = Committee::RequestUnpacker.new(request, allow_form_params: true).call - assert_equal({ "x" => "y" }, params) - end - end - - it "coerces form params with coerce_form_params and a schema" do - %w[application/x-www-form-urlencoded multipart/form-data].each do |content_type| - env = { - "CONTENT_TYPE" => content_type, - "rack.input" => StringIO.new("x=1"), - } - request = Rack::Request.new(env) - - options = {} - # TODO: delete when 5.0.0 released because default value changed - options[:parse_response_by_content_type] = false - router = hyper_schema.build_router(options) - validator = router.build_schema_validator(request) - - schema = JsonSchema::Schema.new - schema.properties = { "x" => JsonSchema::Schema.new } - schema.properties["x"].type = ["integer"] - - link_class = Struct.new(:schema) - link_object = link_class.new(schema) - - validator.instance_variable_set(:@link, link_object) - - params, _ = Committee::RequestUnpacker.new( - request, - allow_form_params: true, - coerce_form_params: true, - schema_validator: validator, - ).call - assert_equal({ "x" => 1 }, params) - end - end - - it "coerces form params with coerce_form_params and an OpenAPI3 schema" do - %w[application/x-www-form-urlencoded multipart/form-data].each do |content_type| - env = { - "CONTENT_TYPE" => content_type, - "rack.input" => StringIO.new("limit=20"), - "PATH_INFO" => "/characters", - "SCRIPT_NAME" => "", - "REQUEST_METHOD" => "GET", - } - request = Rack::Request.new(env) - - options = {} - # TODO: delete when 5.0.0 released because default value changed - options[:parse_response_by_content_type] = false - router = open_api_3_schema.build_router(options) - validator = router.build_schema_validator(request) - - params, _ = Committee::RequestUnpacker.new( - request, - allow_form_params: true, - coerce_form_params: true, - schema_validator: validator, - ).call - # openapi3 not support coerce in request unpacker - assert_equal({ "limit" => '20' }, params) - end - end - - it "coerces error params with coerce_form_params and a OpenAPI3 schema" do - %w[application/x-www-form-urlencoded multipart/form-data].each do |content_type| - env = { - "CONTENT_TYPE" => content_type, - "rack.input" => StringIO.new("limit=twenty"), - "PATH_INFO" => "/characters", - "SCRIPT_NAME" => "", - "REQUEST_METHOD" => "GET", - } - request = Rack::Request.new(env) - - options = {} - # TODO: delete when 5.0.0 released because default value changed - options[:parse_response_by_content_type] = false - router = open_api_3_schema.build_router(options) - validator = router.build_schema_validator(request) - - params, _ = Committee::RequestUnpacker.new( - request, - allow_form_params: true, - coerce_form_params: true, - schema_validator: validator, - ).call - assert_equal({ "limit" => "twenty" }, params) + unpacker = Committee::RequestUnpacker.new(allow_form_params: true) + assert_equal([{ "x" => "y" }, true], unpacker.unpack_request_params(request)) end end @@ -208,8 +120,8 @@ "QUERY_STRING" => "a=b" } request = Rack::Request.new(env) - params, _ = Committee::RequestUnpacker.new(request, allow_form_params: true, allow_query_params: true).call - assert_equal({ "x" => "y", "a" => "b" }, params) + unpacker = Committee::RequestUnpacker.new(allow_form_params: true, allow_query_params: true) + assert_equal([ { "x" => "y"}, true], unpacker.unpack_request_params(request)) end end @@ -219,8 +131,8 @@ "QUERY_STRING" => "a=b" } request = Rack::Request.new(env) - params, _ = Committee::RequestUnpacker.new(request, allow_query_params: true).call - assert_equal({ "a" => "b" }, params) + unpacker = Committee::RequestUnpacker.new(allow_query_params: true) + assert_equal({ "a" => "b" }, unpacker.unpack_query_params(request)) end it "errors if JSON is not an object" do @@ -230,7 +142,7 @@ } request = Rack::Request.new(env) assert_raises(Committee::BadRequest) do - Committee::RequestUnpacker.new(request).call + Committee::RequestUnpacker.new.unpack_request_params(request) end end @@ -240,8 +152,8 @@ "rack.input" => StringIO.new('{"x":"y"}'), } request = Rack::Request.new(env) - params, _ = Committee::RequestUnpacker.new(request).call - assert_equal({}, params) + unpacker = Committee::RequestUnpacker.new + assert_equal([{}, false], unpacker.unpack_request_params(request)) end # this is mostly here for line coverage @@ -250,8 +162,8 @@ "rack.input" => StringIO.new('{"x":[]}'), } request = Rack::Request.new(env) - params, _ = Committee::RequestUnpacker.new(request).call - assert_equal({ "x" => [] }, params) + unpacker = Committee::RequestUnpacker.new + assert_equal([{ "x" => [] }, false], unpacker.unpack_request_params(request)) end it "unpacks http header" do @@ -260,8 +172,8 @@ "rack.input" => StringIO.new(""), } request = Rack::Request.new(env) - _, headers = Committee::RequestUnpacker.new(request, { allow_header_params: true }).call - assert_equal({ "FOO-BAR" => "some header value" }, headers) + unpacker = Committee::RequestUnpacker.new({ allow_header_params: true }) + assert_equal({ "FOO-BAR" => "some header value" }, unpacker.unpack_headers(request)) end it "includes request body when`use_get_body` is true" do @@ -271,8 +183,8 @@ "QUERY_STRING"=>"data=value&x=aaa", } request = Rack::Request.new(env) - params, _ = Committee::RequestUnpacker.new(request, { allow_query_params: true, allow_get_body: true }).call - assert_equal({ 'data' => 'value', 'x' => 1, 'y' => 2 }, params) + unpacker = Committee::RequestUnpacker.new({ allow_query_params: true, allow_get_body: true }) + assert_equal([{ 'x' => 1, 'y' => 2 }, false], unpacker.unpack_request_params(request)) end it "doesn't include request body when `use_get_body` is false" do @@ -282,7 +194,7 @@ "QUERY_STRING"=>"data=value&x=aaa", } request = Rack::Request.new(env) - params, _ = Committee::RequestUnpacker.new(request, { allow_query_params: true, use_get_body: false }).call - assert_equal({ 'data' => 'value', 'x' => 'aaa' }, params) + unpacker = Committee::RequestUnpacker.new({ allow_query_params: true, use_get_body: false }) + assert_equal({ 'data' => 'value', 'x' => 'aaa' }, unpacker.unpack_query_params(request)) end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 52d71d68..cd81cabb 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -55,6 +55,10 @@ def open_api_2_schema @open_api_2_schema ||= Committee::Drivers.load_from_file(open_api_2_schema_path) end +def open_api_2_form_schema + @open_api_2_form_schema ||= Committee::Drivers.load_from_file(open_api_2_form_schema_path) +end + def open_api_3_schema @open_api_3_schema ||= Committee::Drivers.load_from_file(open_api_3_schema_path) end @@ -73,6 +77,10 @@ def open_api_2_data JSON.parse(File.read(open_api_2_schema_path)) end +def open_api_2_form_data + JSON.parse(File.read(open_api_2_form_schema_path)) +end + def open_api_3_data YAML.load_file(open_api_3_schema_path) end @@ -85,6 +93,10 @@ def open_api_2_schema_path "./test/data/openapi2/petstore-expanded.json" end +def open_api_2_form_schema_path + "./test/data/openapi2/petstore-expanded-form.json" +end + def open_api_3_schema_path "./test/data/openapi3/normal.yaml" end