RedHatInsights / insights-api-common-rails

Header, Encryption, RBAC, Serialization, Pagination and other common behavior for Insights microservices built with Rails
Apache License 2.0
3 stars 25 forks source link

Added args to the base_query call #177

Closed mkanoor closed 4 years ago

mkanoor commented 4 years ago

To get the arguments coming into GraphQL the args needs to be passed down to the base_query for any apps that have a custom resolver.

This is a breaking change.

We discovered this during testing for Approval Service, we needed the id of the object that the user is requesting. The graphql gem passes in an args parameter as the second parameter. As shown here and several other places in the gem.

https://github.com/rmosolgo/graphql-ruby/blob/73ac09cb95a0b03b3f00c09921b65534f66d0ae6/spec/fixtures/upgrader/starrable.original.rb#L31

There are currently 2 known repos using this gem for which this will be a breaking change

https://github.com/RedHatInsights/approval-api/blob/830f34c87db22b513b70a301f1907e4d80b29270/app/controllers/api/v1x0/graphql_controller.rb#L19

https://github.com/RedHatInsights/catalog-api/blob/77a579df64add264d02ba840ff3c1fe7019db478/app/controllers/api/v1x0/graphql_controller.rb#L11

These repos might not have an issue

https://github.com/RedHatInsights/sources-api/blob/f8356e5f67e86f732b986a9697c707b388499fd7/app/controllers/api/v1/graphql_controller.rb#L7

https://github.com/RedHatInsights/topological_inventory-api/blob/6a265cbe2d4d3dbf7aa97b2511a3da9e0c96a29d/app/controllers/api/v1/graphql_controller.rb#L6

Sample Query that we were using and we need to get the id => 48 which is in the args but wasn't being passed into the resolver.

{
  requests(id: "48") {
    id
    requests {
      id
      number_of_children
      decision
      description
      number_of_finished_children
      parent_id
      actions {
        id
        operation
        comments
        created_at
        processed_by
      }
    }
    number_of_children
    decision
    description
    number_of_finished_children
    parent_id
  }
}
abellotti commented 4 years ago

LGTM!! 👍 Thanks @mkanoor for updating this. yea, topo and sources don't use the base_query overlay.

@bdunne can we include this in the v3.10 build ? Thanks !!

bdunne commented 4 years ago

@bdunne can we include this in the v3.10 build?

Since it's a breaking change, it requires a major version bump

mkanoor commented 4 years ago

@bdunne Yes it can be included 3.10

bdunne commented 4 years ago

@bdunne Yes it can be included 3.10

I'm confused, this looks like a breaking change to me and you said:

There are currently 2 known repos using this gem for which this will be a breaking change https://github.com/RedHatInsights/approval-api/blob/830f34c87db22b513b70a301f1907e4d80b29270/app/controllers/api/v1x0/graphql_controller.rb#L19 https://github.com/RedHatInsights/catalog-api/blob/77a579df64add264d02ba840ff3c1fe7019db478/app/controllers/api/v1x0/graphql_controller.rb#L11

mkanoor commented 4 years ago

@bdunne It's a breaking change. I am not sure what the version number should be. Should it be 4.0?

bdunne commented 4 years ago

Yes, it needs to bump to 4.0