diff --git a/CHANGELOG.md b/CHANGELOG.md index 970214b5..c49b5296 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,7 @@ Request validation is called automatically for these operations. ### Fixes +- Fixed: `$ref`s nested inside the schema of a parameter or a response header are resolved now, so these values are unpacked and converted as described. Before, only a `$ref` at the top level of the schema was resolved. See #450. - Fixed: The JSON schema of a parameter that uses a `content` field with a `$ref`'d schema is resolved now. - Fixed: Loading a document no longer raises `NoMethodError` when a parameter has neither `schema` nor `content`. - Fixed: Repeated values for a query parameter that describes an object or uses `content` (`?filter=a&filter=b`) raised a `NoMethodError` or `TypeError`. The values are validated against the schema now, which returns an `:invalid_query` failure. diff --git a/lib/openapi_first/builder.rb b/lib/openapi_first/builder.rb index 56181fba..be97b2fe 100644 --- a/lib/openapi_first/builder.rb +++ b/lib/openapi_first/builder.rb @@ -137,7 +137,7 @@ def parse_parameters(parameters) def build_parameters(parameters) parameters.to_a.map do |parameter| - Parameters::Parameter.new(parameter.resolved, schema: parameter_schema_node(parameter)&.resolved) + Parameters::Parameter.new(parameter.resolved, schema: parameter_schema_node(parameter)&.dereferenced) end end @@ -232,7 +232,7 @@ def build_response_headers(headers_object) name:, schema: schema_node.schema(configuration: schemer_configuration), required?: header['required']&.value == true, - resolved_schema: schema_node.resolved + resolved_schema: schema_node.dereferenced ) end result diff --git a/lib/openapi_first/failure.rb b/lib/openapi_first/failure.rb index 894ff0fd..97cfee01 100644 --- a/lib/openapi_first/failure.rb +++ b/lib/openapi_first/failure.rb @@ -49,6 +49,10 @@ def self.new(type, message: nil, errors: nil) alias original_message message private :original_message + def inspect + "#" + end + # A generic error message def message original_message || exception_message diff --git a/lib/openapi_first/ref_resolver.rb b/lib/openapi_first/ref_resolver.rb index 65ebed7d..aa3ff23d 100644 --- a/lib/openapi_first/ref_resolver.rb +++ b/lib/openapi_first/ref_resolver.rb @@ -99,6 +99,10 @@ def resolve_ref(pointer) "file #{File.absolute_path(filepath).inspect}: #{e.message}" raise OpenapiFirst::FileNotFoundError, message end + + private + + def mark(visited) = (visited || []) + [value.object_id] end # @visibility private @@ -106,6 +110,8 @@ class Simple include Resolvable def resolved = value + + def dereferenced(_visited = nil) = value end # @visibility private @@ -124,6 +130,19 @@ def resolved value end + # Returns a plain Hash with all nested $refs resolved. + # A node that is reached again on its own path, as in a recursive schema, + # is returned unresolved to stop the recursion. + # @param visited [Array, nil] Object ids of the nodes on the current path. + def dereferenced(visited = nil) + return value if visited&.include?(value.object_id) + + visited = mark(visited) + return resolve_ref(value['$ref'])&.dereferenced(visited) if value.key?('$ref') + + value.each_key.to_h { |key| [key, self[key]&.dereferenced(visited)] } + end + def [](key) return resolve_ref(@value['$ref'])[key] if !@value.key?(key) && @value.key?('$ref') @@ -201,6 +220,15 @@ def resolved end end end + + # Returns a plain Array with all nested $refs resolved. + # @param visited [Array, nil] Object ids of the nodes on the current path. + def dereferenced(visited = nil) + return value if visited&.include?(value.object_id) + + visited = mark(visited) + value.each_index.map { self[_1]&.dereferenced(visited) } + end end end end diff --git a/spec/data/components/schemas/integers.yaml b/spec/data/components/schemas/integers.yaml new file mode 100644 index 00000000..10351035 --- /dev/null +++ b/spec/data/components/schemas/integers.yaml @@ -0,0 +1,4 @@ +type: array +minItems: 2 +items: + type: integer diff --git a/spec/data/response-header.yaml b/spec/data/response-header.yaml index 57614b31..e66cf5a1 100644 --- a/spec/data/response-header.yaml +++ b/spec/data/response-header.yaml @@ -32,3 +32,12 @@ paths: type: integer X-Authors: $ref: './components/headers/x-authors.yaml' + X-Counts: + schema: + type: array + items: + $ref: '#/components/schemas/count' +components: + schemas: + count: + type: integer diff --git a/spec/middlewares/response_validation/response_header_validation_spec.rb b/spec/middlewares/response_validation/response_header_validation_spec.rb index 24d1a260..1049ebb2 100644 --- a/spec/middlewares/response_validation/response_header_validation_spec.rb +++ b/spec/middlewares/response_validation/response_header_validation_spec.rb @@ -52,6 +52,18 @@ end.to raise_error OpenapiFirst::ResponseInvalidError end + it 'succeeds with a ref nested inside a header schema' do + post '/echo', JSON.generate({ 'X-Counts' => '1,2', 'Location' => '/echos/42' }) + expect(last_response.status).to eq 201 + end + + it 'fails with a ref nested inside a header schema' do + expect do + post '/echo', JSON.generate({ 'X-Counts' => '1,two', 'Location' => '/echos/42' }) + end.to raise_error OpenapiFirst::ResponseInvalidError, + 'Response header is invalid: value at `/X-Counts` is not an integer' + end + it 'fails with a missing header' do expect do post '/echo', JSON.generate({ 'X-Id' => '42' }) diff --git a/spec/ref_resolver_spec.rb b/spec/ref_resolver_spec.rb index 771b071a..f691e1ad 100644 --- a/spec/ref_resolver_spec.rb +++ b/spec/ref_resolver_spec.rb @@ -231,6 +231,71 @@ end end + describe '#dereferenced' do + it 'resolves nested refs' do + doc = resolver.for(contents) + expect(doc.dereferenced).to eq( + 'definitions' => { + 'Thing' => { 'type' => 'object' }, + 'A' => { 'name' => 'A' } + }, + 'hash' => { 'type' => 'object' }, + 'array' => [{ 'name' => 'A' }, { 'name' => 'B' }] + ) + end + + it 'resolves refs nested in a schema' do + contents = { + 'type' => 'object', + 'properties' => { + 'ids' => { '$ref' => '#/$defs/ids' } + }, + '$defs' => { + 'ids' => { 'type' => 'array', 'default' => nil, 'items' => { '$ref' => '#/$defs/id' } }, + 'id' => { 'type' => %w[integer null], 'enum' => [1, nil] } + } + } + doc = resolver.for(contents) + expect(doc.dig('properties', 'ids').dereferenced).to eq( + 'type' => 'array', + 'default' => nil, + 'items' => { 'type' => %w[integer null], 'enum' => [1, nil] } + ) + end + + it 'resolves refs across files' do + doc = resolver.load('./spec/data/query-parameter-validation.yaml') + node = doc.dig('paths', '/search', 'get', 'parameters', 1, 'schema') + expect(node.dereferenced).to eq( + 'type' => 'object', + 'required' => ['name'], + 'properties' => { + 'name' => { 'type' => 'string', 'minLength' => 2 }, + 'other' => { 'type' => 'object' }, + 'id' => { 'type' => 'integer' } + } + ) + end + + it 'keeps the ref of a recursive schema' do + doc = resolver.load('./spec/data/self-referencing.yaml') + node = doc.dig('paths', '/', 'get', 'responses', '200', 'content', 'application/json', 'schema') + expect(node.dereferenced).to eq( + 'type' => 'object', + 'properties' => { + 'foo' => { 'type' => 'string' }, + 'bar' => { + 'type' => 'object', + 'properties' => { + 'foo' => { 'type' => 'string' }, + 'bar' => { '$ref' => '#/components/schemas/MySelfRef' } + } + } + } + ) + end + end + describe '#each' do it 'works across files' do filepath = './spec/data/splitted-train-travel-api/openapi.yaml' diff --git a/spec/test_cases/nullable.yaml b/spec/test_cases/nullable.yaml index 509049bc..1c65c6df 100644 --- a/spec/test_cases/nullable.yaml +++ b/spec/test_cases/nullable.yaml @@ -24,3 +24,33 @@ invalid_response: content_type: application/json body: { "path": ["1", "2", 3] } +- description: nullable value in request body + oad: + openapi: 3.0.2 + paths: + /: + post: + requestBody: + required: true + content: + application/json: + schema: + required: [value] + additionalProperties: false + properties: + value: + type: string + nullable: true + responses: + '200': + description: ok + valid_request: + method: post + uri: '/' + content_type: application/json + body: { "value": null } + invalid_request: + method: post + uri: '/' + content_type: application/json + body: { "foo": "bar" } diff --git a/spec/test_cases/params-with-nested-refs.yaml b/spec/test_cases/params-with-nested-refs.yaml new file mode 100644 index 00000000..60c918d9 --- /dev/null +++ b/spec/test_cases/params-with-nested-refs.yaml @@ -0,0 +1,87 @@ +- description: Query parameter with a $ref'd items schema + oad: + openapi: 3.1.1 + paths: + "/": + get: + parameters: + - name: ids + in: query + schema: + type: array + items: + $ref: '#/components/schemas/id' + responses: + '200': + description: ok + components: + schemas: + id: + type: integer + valid_request: + method: get + uri: '/?ids=1&ids=2' + invalid_request: + method: get + uri: '/?ids=one' + +- description: deepObject query parameter with a $ref inside allOf + oad: + openapi: 3.1.1 + paths: + "/": + get: + parameters: + - name: filter + in: query + style: deepObject + explode: true + schema: + allOf: + - $ref: '#/components/schemas/counter' + responses: + '200': + description: ok + components: + schemas: + counter: + type: object + required: [count] + properties: + count: + type: integer + valid_request: + method: get + uri: '/?filter[count]=5' + invalid_request: + method: get + uri: '/?filter[count]=five' + +- description: Path parameter with a $ref'd property schema + oad: + openapi: 3.1.1 + paths: + "/things/{filter}": + get: + parameters: + - name: filter + in: path + required: true + schema: + type: object + properties: + id: + $ref: '#/components/schemas/id' + responses: + '200': + description: ok + components: + schemas: + id: + type: integer + valid_request: + method: get + uri: '/things/id,42' + invalid_request: + method: get + uri: '/things/id,fortytwo' diff --git a/spec/test_cases/query-param-deepObject-explode.yaml b/spec/test_cases/query-param-deepObject-explode.yaml new file mode 100644 index 00000000..a6c2a859 --- /dev/null +++ b/spec/test_cases/query-param-deepObject-explode.yaml @@ -0,0 +1,26 @@ +- description: deepObject query param with array value + oad: + openapi: 3.1.1 + paths: + /: + get: + operationId: 'foo' + parameters: + - name: filter + in: query + style: deepObject + explode: true + schema: + type: object + properties: + ids: + $ref: '../data/components/schemas/integers.yaml' + responses: + '200': + description: ok + valid_request: + method: 'get' + uri: '/?filter[ids]=1&filter[ids]=2' + invalid_request: + method: 'get' + uri: '/?filter[ids]=1' diff --git a/spec/test_cases_spec.rb b/spec/test_cases_spec.rb index f4be2ead..3f1cf401 100644 --- a/spec/test_cases_spec.rb +++ b/spec/test_cases_spec.rb @@ -6,6 +6,15 @@ RSpec.describe 'request/response validation examples' do include Rack::Test::Methods + def send_request(request_example) + method = request_example.fetch('method') + uri = request_example.fetch('uri') + return send(method, uri) unless request_example['body'] + + send(method, uri, JSON.generate(request_example['body']), + 'CONTENT_TYPE' => request_example.fetch('content_type', 'application/json')) + end + Dir.glob(File.join(__dir__, '/test_cases/*.yaml')).each do |filepath| describe filepath do YAML.load_file(filepath).each do |example| @@ -28,44 +37,76 @@ oad['paths'].keys.first end - let(:test_method) do + let(:request_method) do oad['paths'][test_path].keys.first.upcase end - context 'with valid response' do - let(:response) { example['valid_response'] } + let(:response) do + example['valid_response'] || { 'content_type' => 'application/json', 'body' => {} } + end - it 'passes validation' do - send(test_method.downcase, test_path) + if example['valid_response'] + context 'with valid response' do + let(:response) { example['valid_response'] } - request = Rack::Request.new(last_request.env) - response = Rack::Response.new( - last_response.body, - last_response.status, - last_response.headers - ) + it 'passes validation' do + send(request_method.downcase, test_path) - validated = definition.validate_response(request, response) - expect(validated).to be_valid + request = Rack::Request.new(last_request.env) + response = Rack::Response.new( + last_response.body, + last_response.status, + last_response.headers + ) + + validated = definition.validate_response(request, response) + expect(validated).to be_valid + end end end - context 'with invalid response' do - let(:response) { example['invalid_response'] } + if example['invalid_response'] + context 'with invalid response' do + let(:response) { example['invalid_response'] } + + it 'fails validation' do + send(request_method.downcase, test_path) + + request = Rack::Request.new(last_request.env) + response = Rack::Response.new( + last_response.body, + last_response.status, + last_response.headers + ) - it 'fails validation' do - send(test_method.downcase, test_path) + validated = definition.validate_response(request, response) + expect(validated).not_to be_valid + expect(validated.error).not_to be_nil + end + end + end + + if example['valid_request'] + context 'with valid request' do + it 'passes validation' do + send_request(example['valid_request']) + + validated = definition.validate_request(last_request) + expect(validated.error).to be_nil + expect(validated).to be_valid + end + end + end - request = Rack::Request.new(last_request.env) - response = Rack::Response.new( - last_response.body, - last_response.status, - last_response.headers - ) + if example['invalid_request'] + context 'with invalid request' do + it 'fails validation' do + send_request(example['invalid_request']) - validated = definition.validate_response(request, response) - expect(validated).not_to be_valid - expect(validated.error).not_to be_nil + validated = definition.validate_request(last_request) + expect(validated.error).not_to be_nil + expect(validated).to be_invalid + end end end end