Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix entity data type identification #1863

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fotos
Copy link
Contributor

@fotos fotos commented Feb 12, 2019

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.

The gem `grape-swagger` uses the `type` documentation parameter in order
to identify the entity 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.
@fotos
Copy link
Contributor Author

fotos commented Feb 13, 2019

⚠️ 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… 🐟

@dblock
Copy link
Member

dblock commented Feb 13, 2019

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

@fotos
Copy link
Contributor Author

fotos commented Feb 16, 2019

@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?

@dblock
Copy link
Member

dblock commented Feb 17, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants