grape icon indicating copy to clipboard operation
grape copied to clipboard

Fix entity data type identification

Open fotos opened this issue 6 years ago • 4 comments

I hit a pretty obscure bug 🐛.

The grape-swagger gem uses the type documentation parameter in order to identify the deduce name. When the type (class) is stringified grape-swagger cannot check for method existance and falls back to taking the last module as the name.

Here is a (simplified) example of the problem:

class API::V1::Entities::Event::Date < Grape::Entity
  expose :id
end

module API
  module V1
    class Events < Grape::API
      class Dates < Grape::API
        resources :events do
          route_param :event_id do
            params do
              requires :event_id, type: String, uuid: true, documentation: { format: 'uuid' }
            end

            resources :dates do
              params do
                requires :date, type: API::V1::Entities::Event::Date
              end
              post do
                # do something meaningful

                present date, with: API::V1::Entities::Event::Date
              end
            end
          end
        end
      end
    end
  end
end

If the entity name ends in Date then it's regarded as primitive and the swagger documentation is generated as a formData:

post:
  parameters:
    - in: path
      name: event_id
      type: string
      format: uuid
      required: true
    - in: formData
      name: date
      type: string
      format: null
      required: true

(where the date type is then also marked as a string instead of object).

While generating the param documentation type attribute, the type is converted to a string. The string type is only used to check whether the param is an Array, which can be accomplished by a class inheritance check as well.

With the changes in this PR and by adding API::V1::Events::Date.entity_name

class API::V1::Entities::Event::Date < Grape::Entity
  expose :id

  def self.entity_name
    'EventDate'
  end
end

the generated swagger doc is correct:

post:
  parameters:
    - in: path
      name: event_id
      type: string
      format: uuid
      required: true
    - name: EventsEventIdDates
      in: body
      required: true
      schema:
        $ref: '#/definitions/postEventsEventIdDates'

Everything works as expected, the specs pass and I didn't see any regressions.

I'm not 100% sure if I got this right, please let me know what you think.

fotos avatar Feb 12 '19 17:02 fotos

⚠️ this PR may not be correct after all, please do not merge yet 😢

Although no specs are broken by the change, I think I might have triggered a bug similar to ruby-grape/grape-swagger#666 👹. By changing the array "identification" the generated swagger docs for an endpoint that accepts an array of elements (with the hack mentioned in ruby-grape/grape-swagger#666) broke.

I'll investigate a bit more and try to write a few more specs that cover all cases (this and ruby-grape/grape-swagger#666).

Something is fishy with the arrays… 🐟

fotos avatar Feb 13 '19 09:02 fotos

Ok, I do like the general direction of this though. Strong type checking is better than Array.

dblock avatar Feb 13 '19 18:02 dblock

@dblock I traced the problem with arrays in grape-swagger and proposed a fix in ruby-grape/grape-swagger#742 (previous code included a nasty trick).

I'm not 100% sure about the implications. What do you think is the best way forward?

fotos avatar Feb 16 '19 15:02 fotos

Let's see the maintainer(s) of that repo merge it! Keeping this open.

dblock avatar Feb 17 '19 12:02 dblock