Skip to content

Commit

Permalink
Fix a bunch of API validation problems
Browse files Browse the repository at this point in the history
Apparently `nullable` doesn't play well with attribute modifications in
OpenAPI 3.0 (apparently better in 3.1?).

This is evidenced by validations previously passing against newer
versions when it should've been impossible.

OAI/OpenAPI-Specification#1368
  • Loading branch information
elliotcm committed Nov 22, 2022
1 parent 0db5ed3 commit be0c839
Show file tree
Hide file tree
Showing 14 changed files with 49 additions and 37 deletions.
20 changes: 16 additions & 4 deletions app/presenters/api_docs/api_schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,26 @@ class APISchema
attr_reader :schema
delegate :description, :required, to: :schema

def self.null_types(schema)
all_types(schema).select { |type| type['type'].nil? }
end

def self.non_null_types(schema)
all_types(schema).reject { |type| type['type'].nil? }
end

def self.all_types(schema)
(schema['anyOf']&.to_a || []) + (schema['oneOf']&.to_a || []) + (schema['allOf']&.to_a || [])
end

def initialize(schema)
@schema = schema
end

def properties
props = []

schema['allOf']&.each do |schema_nested|
self.class.non_null_types(schema).each do |schema_nested|
schema_nested.properties.each do |property|
props << property
end
Expand Down Expand Up @@ -56,7 +68,7 @@ def required?
end

def nullable?
attributes['nullable'] || attributes['oneOf']&.any? { |ref_type| ref_type['type'].nil? }
attributes['nullable'] || APISchema.null_types(attributes).any?
end

def deprecated?
Expand Down Expand Up @@ -87,8 +99,8 @@ def object_schema_name
linked_schema = attributes['items']
end

if attributes['oneOf'].present?
linked_schema = attributes['oneOf'].find { |ref_type| ref_type['type'].present? }
if (non_null_types = APISchema.non_null_types(attributes)).any?
linked_schema = non_null_types.first
end

location = linked_schema.node_context.source_location.to_s
Expand Down
2 changes: 1 addition & 1 deletion config/vendor_api/draft.yml
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ components:
- "$ref": "#/components/schemas/Month"
end_month:
description: The month the position ended
oneOf: # https://github.com/OAI/OpenAPI-Specification/issues/1368
anyOf:
- type: null
- "$ref": "#/components/schemas/Month"
skills_relevant_to_teaching:
Expand Down
10 changes: 5 additions & 5 deletions config/vendor_api/v1.0.yml
Original file line number Diff line number Diff line change
Expand Up @@ -507,20 +507,20 @@ components:
description: References will only be included once a candidate has accepted an offer and has received a reference. There is no limit to the number of references.
offer:
anyOf:
- type: null
- "$ref": "#/components/schemas/Offer"
nullable: true
withdrawal:
anyOf:
- type: null
- "$ref": "#/components/schemas/Withdrawal"
nullable: true
rejection:
anyOf:
- type: null
- "$ref": "#/components/schemas/Rejection"
nullable: true
hesa_itt_data:
anyOf:
- type: null
- "$ref": "#/components/schemas/HESAITTData"
nullable: true
deprecated: true
description: |
Values to populate this candidate’s HESA Initial Teacher
Expand Down Expand Up @@ -804,7 +804,7 @@ components:
- Completion of professional skills test
course:
description: An optional `course` that you wish to suggest to the candidate.
anyOf:
oneOf:
- "$ref": "#/components/schemas/Course"
Qualification:
type: object
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@

get_api_request "/api/v1.0/applications?since=#{CGI.escape(1.day.ago.iso8601)}"

expect(parsed_response).to be_valid_against_openapi_schema('MultipleApplicationsResponse')
expect(parsed_response).to be_valid_against_openapi_schema('MultipleApplicationsResponse', '1.0')
end

it 'returns a ParameterMissingResponse if the `since` parameter is missing' do
Expand Down
8 changes: 4 additions & 4 deletions spec/requests/vendor_api/v1.0/get_reference_data_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
end

it 'returns a response that is valid according to the OpenAPI schema' do
expect(parsed_response).to be_valid_against_openapi_schema('ListResponse')
expect(parsed_response).to be_valid_against_openapi_schema('ListResponse', '1.0')
end

it 'includes GCSE subjects' do
Expand All @@ -23,7 +23,7 @@
end

it 'returns a response that is valid according to the OpenAPI schema' do
expect(parsed_response).to be_valid_against_openapi_schema('ListResponse')
expect(parsed_response).to be_valid_against_openapi_schema('ListResponse', '1.0')
end

it 'includes appropriate subjects' do
Expand All @@ -37,7 +37,7 @@
end

it 'returns a response that is valid according to the OpenAPI schema' do
expect(parsed_response).to be_valid_against_openapi_schema('ListResponse')
expect(parsed_response).to be_valid_against_openapi_schema('ListResponse', '1.0')
end

it 'includes appropriate grades' do
Expand All @@ -51,7 +51,7 @@
end

it 'returns a response that is valid according to the OpenAPI schema' do
expect(parsed_response).to be_valid_against_openapi_schema('ListResponse')
expect(parsed_response).to be_valid_against_openapi_schema('ListResponse', '1.0')
end

it 'includes appropriate grades' do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

get_api_request "/api/v1.0/applications/#{application_choice.id}"

expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse')
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse', '1.0')
end

it 'returns a NotFoundResponse if the application cannot be found' do
Expand Down
2 changes: 1 addition & 1 deletion spec/requests/vendor_api/v1.0/post_conditions_met_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
post_api_request "/api/v1.0/applications/#{application_choice.id}/confirm-conditions-met"

expect(response).to have_http_status(:ok)
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse')
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse', '1.0')
expect(parsed_response['data']['attributes']['status']).to eq 'recruited'
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
post_api_request "/api/v1.0/applications/#{application_choice.id}/conditions-not-met"

expect(response).to have_http_status(:ok)
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse')
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse', '1.0')
expect(parsed_response['data']['attributes']['status']).to eq 'conditions_not_met'
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
post_api_request "/api/v1.0/applications/#{application_choice.id}/confirm-enrolment"

expect(response).to have_http_status(:ok)
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse')
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse', '1.0')
expect(parsed_response['data']['attributes']['status']).to eq 'recruited'
end

Expand Down
22 changes: 11 additions & 11 deletions spec/requests/vendor_api/v1.0/post_make_an_offer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@
],
},
}
expect(request_body[:data]).to be_valid_against_openapi_schema('MakeOffer')
expect(request_body[:data]).to be_valid_against_openapi_schema('MakeOffer', '1.0')

post_api_request "/api/v1.0/applications/#{application_choice.id}/offer", params: request_body

course_option = application_choice.course_option
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse')
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse', '1.0')
expect(parsed_response['data']['attributes']['status']).to eq('offer')
expect(parsed_response['data']['attributes']['offer']).to eq(
'conditions' => [
Expand Down Expand Up @@ -74,12 +74,12 @@
],
},
}
expect(request_body[:data]).to be_valid_against_openapi_schema('MakeOffer')
expect(request_body[:data]).to be_valid_against_openapi_schema('MakeOffer', '1.0')

post_api_request "/api/v1.0/applications/#{application_choice.id}/offer", params: request_body

course_option = application_choice.course_option
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse')
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse', '1.0')
expect(parsed_response['data']['attributes']['status']).to eq('offer')
expect(parsed_response['data']['attributes']['offer']).to eq(
'conditions' => [
Expand Down Expand Up @@ -109,7 +109,7 @@
},
}

expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse')
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse', '1.0')
expect(parsed_response['data']['attributes']['offer']).to eq(
'conditions' => [],
'course' => course_option_to_course_payload(other_course_option),
Expand All @@ -136,7 +136,7 @@
}

expect(response).to have_http_status(:unprocessable_entity)
expect(parsed_response).to be_valid_against_openapi_schema('UnprocessableEntityResponse')
expect(parsed_response).to be_valid_against_openapi_schema('UnprocessableEntityResponse', '1.0')
expect(parsed_response['errors'].map { |e| e['message'] }).to contain_exactly(
'Course code cannot be blank',
'Study mode cannot be blank',
Expand Down Expand Up @@ -279,7 +279,7 @@
},
}

expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse')
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse', '1.0')
expect(parsed_response['data']['attributes']['offer']).to eq(
'conditions' => [],
'course' => course_option_to_course_payload(other_course_option),
Expand All @@ -303,7 +303,7 @@
},
}

expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse')
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse', '1.0')
expect(parsed_response['data']['attributes']['offer']).to eq(
'conditions' => [],
'course' => course_option_to_course_payload(other_course_option),
Expand All @@ -329,7 +329,7 @@
}

post_api_request "/api/v1.0/applications/#{application_choice.id}/offer", params: request_body
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse')
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse', '1.0')

new_course_option = course_option_for_provider(provider: currently_authenticated_provider)

Expand Down Expand Up @@ -367,7 +367,7 @@
request_body = { data: { conditions: ['DBS Check'] } }
post_api_request "/api/v1.0/applications/#{application_choice.id}/offer", params: request_body

expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse')
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse', '1.0')
expect(parsed_response['data']['attributes']['status']).to eq('offer')
end
end
Expand All @@ -385,7 +385,7 @@
}

course_option = application_choice.course_option
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse')
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse', '1.0')
expect(parsed_response['data']['attributes']['status']).to eq('offer')
expect(parsed_response['data']['attributes']['offer']).to eq(
'conditions' => [],
Expand Down
4 changes: 2 additions & 2 deletions spec/requests/vendor_api/v1.0/post_reject_application_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
post_api_request "/api/v1.0/applications/#{application_choice.id}/reject", params: request_body

expect(response).to have_http_status(:ok)
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse')
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse', '1.0')
expect(parsed_response['data']['attributes']['status']).to eq 'rejected'
expect(parsed_response['data']['attributes']['rejection']).to match a_hash_including(
'reason' => 'Does not meet minimum GCSE requirements',
Expand All @@ -39,7 +39,7 @@
post_api_request "/api/v1.0/applications/#{application_choice.id}/reject", params: request_body

expect(response).to have_http_status(:ok)
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse')
expect(parsed_response).to be_valid_against_openapi_schema('SingleApplicationResponse', '1.0')
expect(parsed_response['data']['attributes']['status']).to eq 'rejected'
expect(parsed_response['data']['attributes']['rejection']).to match a_hash_including(
'reason' => 'Course is over-subscribed',
Expand Down
2 changes: 1 addition & 1 deletion spec/requests/vendor_api/v1.0/post_test_data_clear_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,6 @@

it 'returns responses conforming to the schema' do
post_api_request('/api/v1.0/test-data/clear')
expect(parsed_response).to be_valid_against_openapi_schema('OkResponse')
expect(parsed_response).to be_valid_against_openapi_schema('OkResponse', '1.0')
end
end
6 changes: 3 additions & 3 deletions spec/requests/vendor_api/v1.0/post_test_data_generate_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -145,22 +145,22 @@

post_api_request '/api/v1.0/test-data/generate?count=1'

expect(parsed_response).to be_valid_against_openapi_schema('OkResponse')
expect(parsed_response).to be_valid_against_openapi_schema('OkResponse', '1.0')
end

it 'returns error responses on invalid input' do
create(:course_option, course: create(:course, :open_on_apply, provider: currently_authenticated_provider))

post_api_request '/api/v1.0/test-data/generate?count=1&courses_per_application=2'

expect(parsed_response).to be_valid_against_openapi_schema('UnprocessableEntityResponse')
expect(parsed_response).to be_valid_against_openapi_schema('UnprocessableEntityResponse', '1.0')
end

it 'returns error when you ask for zero courses per application' do
create(:course_option, course: create(:course, :open_on_apply, provider: currently_authenticated_provider))

post_api_request '/api/v1.0/test-data/generate?count=1&courses_per_application=0'

expect(parsed_response).to be_valid_against_openapi_schema('UnprocessableEntityResponse')
expect(parsed_response).to be_valid_against_openapi_schema('UnprocessableEntityResponse', '1.0')
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ def post_interview!(params:, skip_validation: nil)
post_interview! params: update_interview_params

expect(response).to have_http_status(:not_found)
expect(parsed_response).to be_valid_against_openapi_schema('NotFoundResponse')
expect(parsed_response).to be_valid_against_openapi_schema('NotFoundResponse', '1.1')
expect(parsed_response['errors'].map { |error| error['message'] })
.to contain_exactly('Unable to find Application(s)')
end
Expand Down

0 comments on commit be0c839

Please sign in to comment.