diff --git a/CHANGELOG.md b/CHANGELOG.md index 511f0c95..09971991 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ * [#978](https://github.com/ruby-grape/grape-swagger/pull/978): Fix Grape 3.2+ compatibility: desc kwargs, custom types, multi-type param recovery; bump Grape to `>= 2.1, < 5.0`. See [UPGRADING](UPGRADING.md) - [@numbata](https://github.com/numbata). * [#982](https://github.com/ruby-grape/grape-swagger/pull/982): Fix test suite compatibility with Grape 4.0 (grape=HEAD CI) - [@numbata](https://github.com/numbata). * [#981](https://github.com/ruby-grape/grape-swagger/pull/981): Use `endpoint.endpoints` instead of `endpoint.options[:app]` - [@ericproulx](https://github.com/ericproulx). +* [#985](https://github.com/ruby-grape/grape-swagger/pull/985): Fix the test suite against unreleased Grape 4.0: give collection custom types a `parse`, and stop building `Grape::Util::StackableValues` directly now that it is a read-only view. See [UPGRADING](UPGRADING.md) - [@ericproulx](https://github.com/ericproulx). * [#983](https://github.com/ruby-grape/grape-swagger/pull/983): Read route metadata via reader methods instead of `route.options[...]` - [@ericproulx](https://github.com/ericproulx). * [#984](https://github.com/ruby-grape/grape-swagger/pull/984): Drop the undocumented `formats:` / `content_types:` aliases for `produces:` - [@ericproulx](https://github.com/ericproulx). * Your contribution here. diff --git a/UPGRADING.md b/UPGRADING.md index 8c149d34..0fd5d24f 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -20,6 +20,8 @@ end ``` + On **Grape 4.0** the same requirement extends to collections: `type: Array[MyClass]` and `type: Set[MyClass]` now raise the same `ArgumentError` while the `params` block is evaluated, i.e. as the API class loads. Grape used to build the element coercer lazily, on the first request that supplied the parameter, so a documentation-only collection type was never coerced and never raised — until it was, as a confusing per-request `400 "is invalid"`. See [ruby-grape/grape#2817](https://github.com/ruby-grape/grape/pull/2817) and Grape's UPGRADING. The fix is the same `parse` — note that `parse(val) = val` performs no validation and is only appropriate when the type is documentation-only and the route never actually coerces client input; otherwise implement real coercion or supply `coerce_with:`, or drop `type:` and declare the type under `documentation:` if it only describes the parameter. + - **Multi-type params (`type: [A, B]`) on Grape 3.2+**: swagger output now reflects the first declared type (e.g. `type: [Integer, Float]` produces `"integer"`). Previously, Grape 3.2+ serialized the `VariantCollectionCoercer` wrapper via `#to_s`, leaking `"#"` into the documentation. No action required, but if you were programmatically post-processing that string, the fix will change the output. - **`desc(..., formats:)` and `desc(..., content_types:)` no longer set the swagger `produces` field.** These were undocumented aliases for `produces:`. Use `produces:` instead (e.g. `desc 'x', produces: ['application/xml']`), which is unchanged. diff --git a/spec/lib/request_param_parsers/route_spec.rb b/spec/lib/request_param_parsers/route_spec.rb index f237c0e5..7569842a 100644 --- a/spec/lib/request_param_parsers/route_spec.rb +++ b/spec/lib/request_param_parsers/route_spec.rb @@ -1,6 +1,25 @@ # frozen_string_literal: true describe GrapeSwagger::RequestParamParsers::Route do + # Grape::Util::StackableValues's constructor signature is a Grape-internal + # detail that differs across supported Grape versions (and grape=HEAD), so + # rather than calling `.new` directly, these specs harvest a real instance + # through the public API and stub its readers for the scenario under test. + # `instance_double` won't pass `route.rb`'s `is_a?(Grape::Util::StackableValues)` + # check, hence harvesting a real instance instead of building a plain double. + # `#[]` reads Grape's own ivars, not the stubbed readers below, so it is + # stubbed too -- otherwise it would silently return the harvest probe's real + # (empty) namespace instead of the fixture if a caller ever read through `[]`. + def stackable_values_double(new_values, inherited_values = {}) + harvested = Class.new(Grape::API) do + namespace(':harvest') { get('/probe') { 'probe' } } + end.routes.first.app.inheritable_setting.namespace_stackable + + allow(harvested).to receive_messages(new_values: new_values, inherited_values: inherited_values) + allow(harvested).to receive(:[]) { |key| new_values[key] || inherited_values[key] } + harvested + end + let(:route) { instance_double('route', app: nil) } let(:parser) { described_class.new(route, nil, nil, nil) } @@ -8,15 +27,16 @@ subject(:parse_request_params) { described_class.parse(route, nil, nil, nil) } context 'when inherited namespace stackable values contain path params across levels' do - let(:root_stackable) { Grape::Util::StackableValues.new } - let(:nested_stackable) { Grape::Util::StackableValues.new(root_stackable) } + let(:root_stackable) do + stackable_values_double(namespace: [instance_double('namespace', space: ':account_id', options: { required: true, type: 'Integer' })]) + end + let(:nested_stackable) do + stackable_values_double({ namespace: [instance_double('namespace', space: ':id', options: { required: true, type: 'String' })] }, root_stackable) + end let(:inheritable_setting) { instance_double('inheritable_setting', namespace_stackable: nested_stackable) } let(:app) { instance_double('app', inheritable_setting: inheritable_setting) } before do - root_stackable[:namespace] = instance_double('namespace', space: ':account_id', options: { required: true, type: 'Integer' }) - nested_stackable[:namespace] = instance_double('namespace', space: ':id', options: { required: true, type: 'String' }) - allow(route).to receive(:app).and_return(app) allow(route).to receive(:params).and_return( 'account_id' => {}, @@ -50,12 +70,13 @@ end context 'when route.params contains only symbol-keyed params' do - let(:stackable) { Grape::Util::StackableValues.new } + let(:stackable) do + stackable_values_double(namespace: [instance_double('namespace', space: ':id', options: { required: true, type: 'Integer' })]) + end let(:inheritable_setting) { instance_double('inheritable_setting', namespace_stackable: stackable) } let(:app) { instance_double('app', inheritable_setting:) } before do - stackable[:namespace] = instance_double('namespace', space: ':id', options: { required: true, type: 'Integer' }) allow(route).to receive(:app).and_return(app) allow(route).to receive(:params).and_return(id: {}) end @@ -68,12 +89,13 @@ end context 'when namespace space is a colon-prefixed string for a symbol-keyed param' do - let(:stackable) { Grape::Util::StackableValues.new } + let(:stackable) do + stackable_values_double(namespace: [instance_double('namespace', space: ':account_id', options: { required: true, type: 'Integer' })]) + end let(:inheritable_setting) { instance_double('inheritable_setting', namespace_stackable: stackable) } let(:app) { instance_double('app', inheritable_setting:) } before do - stackable[:namespace] = instance_double('namespace', space: ':account_id', options: { required: true, type: 'Integer' }) allow(route).to receive(:app).and_return(app) allow(route).to receive(:params).and_return(account_id: {}) end @@ -86,12 +108,13 @@ end context 'when namespace space has more than one leading colon' do - let(:stackable) { Grape::Util::StackableValues.new } + let(:stackable) do + stackable_values_double(namespace: [instance_double('namespace', space: '::id', options: { required: true, type: 'Integer' })]) + end let(:inheritable_setting) { instance_double('inheritable_setting', namespace_stackable: stackable) } let(:app) { instance_double('app', inheritable_setting:) } before do - stackable[:namespace] = instance_double('namespace', space: '::id', options: { required: true, type: 'Integer' }) allow(route).to receive(:app).and_return(app) allow(route).to receive(:params).and_return(':id' => {}) end @@ -104,15 +127,16 @@ end context 'when inherited namespace stackable values redefine the same path param' do - let(:root_stackable) { Grape::Util::StackableValues.new } - let(:nested_stackable) { Grape::Util::StackableValues.new(root_stackable) } + let(:root_stackable) do + stackable_values_double(namespace: [instance_double('namespace', space: ':id', options: { required: true, type: 'Integer' })]) + end + let(:nested_stackable) do + stackable_values_double({ namespace: [instance_double('namespace', space: ':id', options: { required: true, type: 'String' })] }, root_stackable) + end let(:inheritable_setting) { instance_double('inheritable_setting', namespace_stackable: nested_stackable) } let(:app) { instance_double('app', inheritable_setting:) } before do - root_stackable[:namespace] = instance_double('namespace', space: ':id', options: { required: true, type: 'Integer' }) - nested_stackable[:namespace] = instance_double('namespace', space: ':id', options: { required: true, type: 'String' }) - allow(route).to receive(:app).and_return(app) allow(route).to receive(:params).and_return( 'id' => {} @@ -127,23 +151,28 @@ end context 'when inherited namespace stackable values partially override the same path param' do - let(:root_stackable) { Grape::Util::StackableValues.new } - let(:nested_stackable) { Grape::Util::StackableValues.new(root_stackable) } + let(:root_stackable) do + stackable_values_double( + namespace: [instance_double( + 'namespace', + space: ':id', + options: { documentation: { type: 'integer', format: 'int64' } } + )] + ) + end + let(:nested_stackable) do + stackable_values_double( + { namespace: [instance_double( + 'namespace', + space: ':id', + options: { desc: 'inner description' } + )] }, root_stackable + ) + end let(:inheritable_setting) { instance_double('inheritable_setting', namespace_stackable: nested_stackable) } let(:app) { instance_double('app', inheritable_setting:) } before do - root_stackable[:namespace] = instance_double( - 'namespace', - space: ':id', - options: { documentation: { type: 'integer', format: 'int64' } } - ) - nested_stackable[:namespace] = instance_double( - 'namespace', - space: ':id', - options: { desc: 'inner description' } - ) - allow(route).to receive(:app).and_return(app) allow(route).to receive(:params).and_return( 'id' => {} @@ -161,23 +190,28 @@ end context 'when inherited namespace stackable values partially override nested documentation' do - let(:root_stackable) { Grape::Util::StackableValues.new } - let(:nested_stackable) { Grape::Util::StackableValues.new(root_stackable) } + let(:root_stackable) do + stackable_values_double( + namespace: [instance_double( + 'namespace', + space: ':id', + options: { documentation: { type: 'integer', format: 'int64' } } + )] + ) + end + let(:nested_stackable) do + stackable_values_double( + { namespace: [instance_double( + 'namespace', + space: ':id', + options: { documentation: { desc: 'inner description' } } + )] }, root_stackable + ) + end let(:inheritable_setting) { instance_double('inheritable_setting', namespace_stackable: nested_stackable) } let(:app) { instance_double('app', inheritable_setting:) } before do - root_stackable[:namespace] = instance_double( - 'namespace', - space: ':id', - options: { documentation: { type: 'integer', format: 'int64' } } - ) - nested_stackable[:namespace] = instance_double( - 'namespace', - space: ':id', - options: { documentation: { desc: 'inner description' } } - ) - allow(route).to receive(:app).and_return(app) allow(route).to receive(:params).and_return( 'id' => {} @@ -194,23 +228,28 @@ end context 'when inherited namespace stackable values partially override deeply nested hashes' do - let(:root_stackable) { Grape::Util::StackableValues.new } - let(:nested_stackable) { Grape::Util::StackableValues.new(root_stackable) } + let(:root_stackable) do + stackable_values_double( + namespace: [instance_double( + 'namespace', + space: ':id', + options: { documentation: { schema: { type: 'integer', format: 'int64' } } } + )] + ) + end + let(:nested_stackable) do + stackable_values_double( + { namespace: [instance_double( + 'namespace', + space: ':id', + options: { documentation: { schema: { desc: 'inner description' } } } + )] }, root_stackable + ) + end let(:inheritable_setting) { instance_double('inheritable_setting', namespace_stackable: nested_stackable) } let(:app) { instance_double('app', inheritable_setting:) } before do - root_stackable[:namespace] = instance_double( - 'namespace', - space: ':id', - options: { documentation: { schema: { type: 'integer', format: 'int64' } } } - ) - nested_stackable[:namespace] = instance_double( - 'namespace', - space: ':id', - options: { documentation: { schema: { desc: 'inner description' } } } - ) - allow(route).to receive(:app).and_return(app) allow(route).to receive(:params).and_return( 'id' => {} @@ -227,23 +266,28 @@ end context 'when inherited namespace stackable values override array options' do - let(:root_stackable) { Grape::Util::StackableValues.new } - let(:nested_stackable) { Grape::Util::StackableValues.new(root_stackable) } + let(:root_stackable) do + stackable_values_double( + namespace: [instance_double( + 'namespace', + space: ':id', + options: { documentation: { values: %w[outer-a outer-b] } } + )] + ) + end + let(:nested_stackable) do + stackable_values_double( + { namespace: [instance_double( + 'namespace', + space: ':id', + options: { documentation: { values: %w[inner-a inner-b] } } + )] }, root_stackable + ) + end let(:inheritable_setting) { instance_double('inheritable_setting', namespace_stackable: nested_stackable) } let(:app) { instance_double('app', inheritable_setting:) } before do - root_stackable[:namespace] = instance_double( - 'namespace', - space: ':id', - options: { documentation: { values: %w[outer-a outer-b] } } - ) - nested_stackable[:namespace] = instance_double( - 'namespace', - space: ':id', - options: { documentation: { values: %w[inner-a inner-b] } } - ) - allow(route).to receive(:app).and_return(app) allow(route).to receive(:params).and_return( 'id' => {} diff --git a/spec/lib/swagger_routing_spec.rb b/spec/lib/swagger_routing_spec.rb index 6f1d94d7..698c4245 100644 --- a/spec/lib/swagger_routing_spec.rb +++ b/spec/lib/swagger_routing_spec.rb @@ -1,6 +1,17 @@ # frozen_string_literal: true describe GrapeSwagger::SwaggerRouting do + # Grape::Util::StackableValues's constructor and writers are Grape-internal + # details that differ across supported Grape versions (and grape=HEAD), where + # the class is a read-only view with no `.new(0 args)` and no `[]=`. + # `combine_namespaces` only ever reads keys off it, so these specs stub that + # contract — `[](key) => Array` — rather than building a real instance. + def namespace_stackable_double(values) + instance_double('namespace_stackable').tap do |stackable| + allow(stackable).to receive(:[]) { |key| values[key] } + end + end + let(:routing) do Class.new do include GrapeSwagger::SwaggerRouting @@ -43,9 +54,10 @@ context 'when joined mount paths contain multiple double-slash runs' do it 'normalizes all doubled slashes in the namespace key' do - namespace_stackable = Grape::Util::StackableValues.new - namespace_stackable[:namespace] = namespace - namespace_stackable[:mount_path] = ['//foo/', '/bar'] + namespace_stackable = namespace_stackable_double( + namespace: [namespace], + mount_path: ['//foo/', '/bar'] + ) endpoint = instance_double( 'endpoint', endpoints: nil, @@ -63,14 +75,16 @@ context 'when an endpoint exposes nested endpoints' do it 'processes sub-endpoints returned by the endpoint accessor' do - parent_namespace_stackable = Grape::Util::StackableValues.new - parent_namespace_stackable[:namespace] = namespace - parent_namespace_stackable[:mount_path] = ['/api'] + parent_namespace_stackable = namespace_stackable_double( + namespace: [namespace], + mount_path: ['/api'] + ) child_namespace = instance_double('child_namespace', options: {}) - child_namespace_stackable = Grape::Util::StackableValues.new - child_namespace_stackable[:namespace] = child_namespace - child_namespace_stackable[:mount_path] = ['/api', '/v1'] + child_namespace_stackable = namespace_stackable_double( + namespace: [child_namespace], + mount_path: ['/api', '/v1'] + ) child_endpoint = instance_double( 'child_endpoint', diff --git a/spec/support/model_parsers/mock_parser.rb b/spec/support/model_parsers/mock_parser.rb index 5b120e55..6b76d72e 100644 --- a/spec/support/model_parsers/mock_parser.rb +++ b/spec/support/model_parsers/mock_parser.rb @@ -63,7 +63,11 @@ class QueryInputElement < OpenStruct; end class QueryInput < OpenStruct; end - class ApiError < OpenStruct; end + class ApiError < OpenStruct + # Custom types need a one-argument .parse for Grape to coerce them: + # `type: X` since Grape 3.2, `type: Array[X]` since Grape 4.0 (grape#2817). + def self.parse(val) = val + end class SecondApiError < OpenStruct; end @@ -73,7 +77,8 @@ class DocumentedHashAndArrayModel < OpenStruct; end module NestedModule class ApiResponse < OpenStruct - # Grape 3.2+ requires unknown types to implement .parse (arity 1) + # Custom types need a one-argument .parse for Grape to coerce them: + # `type: X` since Grape 3.2, `type: Array[X]` since Grape 4.0 (grape#2817). def self.parse(val) = val end end diff --git a/spec/support/model_parsers/representable_parser.rb b/spec/support/model_parsers/representable_parser.rb index 4014f5ca..09e11ad5 100644 --- a/spec/support/model_parsers/representable_parser.rb +++ b/spec/support/model_parsers/representable_parser.rb @@ -91,6 +91,10 @@ class ApiError < Representable::Decorator property :code, documentation: { type: Integer, desc: 'status code' } property :message, documentation: { type: String, desc: 'error message' } + + # Custom types need a one-argument .parse for Grape to coerce them: + # `type: X` since Grape 3.2, `type: Array[X]` since Grape 4.0 (grape#2817). + def self.parse(val) = val end module NestedModule @@ -100,7 +104,8 @@ class ApiResponse < Representable::Decorator property :status, documentation: { type: String } property :error, documentation: { type: ::Entities::ApiError } - # Grape 3.2+ requires unknown types to implement .parse (arity 1) + # Custom types need a one-argument .parse for Grape to coerce them: + # `type: X` since Grape 3.2, `type: Array[X]` since Grape 4.0 (grape#2817). def self.parse(val) = val end end