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

Added args to the base_query call #177

Merged
merged 2 commits into from
Apr 15, 2020

Conversation

mkanoor
Copy link
Contributor

@mkanoor mkanoor commented Apr 7, 2020

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
  }
}

@mkanoor mkanoor requested a review from abellotti April 7, 2020 21:53
@abellotti abellotti requested a review from bdunne April 7, 2020 22:03
@abellotti
Copy link
Contributor

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 !!

mkanoor added a commit to mkanoor/catalog-api that referenced this pull request Apr 8, 2020
Related to PR RedHatInsights/insights-api-common-rails#177
The Gemfile has been modified to point to my branch
@bdunne
Copy link
Contributor

bdunne commented Apr 8, 2020

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

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

@mkanoor
Copy link
Contributor Author

mkanoor commented Apr 8, 2020

@bdunne Yes it can be included 3.10

@bdunne
Copy link
Contributor

bdunne commented Apr 8, 2020

@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
Copy link
Contributor Author

mkanoor commented Apr 8, 2020

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

@bdunne
Copy link
Contributor

bdunne commented Apr 8, 2020

Yes, it needs to bump to 4.0

Copy link
Contributor

@syncrou syncrou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@syncrou syncrou merged commit 8850491 into RedHatInsights:master Apr 15, 2020
eclarizio pushed a commit to eclarizio/catalog-api that referenced this pull request Apr 28, 2020
Related to PR RedHatInsights/insights-api-common-rails#177
The Gemfile has been modified to point to my branch
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.

None yet

4 participants