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

[Stitching] Ensure schema matches existing MPv2 definitions #504

Merged
merged 2 commits into from
Mar 5, 2020

Conversation

damassi
Copy link
Member

@damassi damassi commented Mar 5, 2020

Related: artsy/metaphysics#2221

This PR updates our GraphQL schema to match the currently unstitched MPv2 schema. We had to make these changes on the convection side because to enable stitching we had to take old mobile app versions into account, and if we modified the schema on the MP side it would be a breaking change.

There's some camelCase versus snake_case conversion that we had to do to line things up, but nothing too severe or blocking (there were only three mutations to update).

The other thing we had to change was the GraphQL type names. There were slight differences between what MP uses and what convection had defined.

Follow-up steps will be to open an additional PR to MP that removes Convection code unrelated to stitching, including three manually defined mutations that communicate with Convection via the REST API.

input_field :additional_info, types.String
input_field :artist_id, types.String
input_field :authenticity_certificate, types.Boolean
input_field :additionalInfo, types.String
Copy link
Member Author

@damassi damassi Mar 5, 2020

Choose a reason for hiding this comment

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

graphql-ruby has a nice (or rather, confusing) feature where it will automatically take snake_case fields and camelize them (convenient for MPv2). However, this doesn't apply to inputs.

@@ -49,9 +52,12 @@ def self.resolve(_obj, args, context)
raise(GraphQL::ExecutionError, 'Submission Not Found')
end

# FIXME: Why does the API reject this property?
params.delete('dimensions_metric')
Copy link
Member Author

@damassi damassi Mar 5, 2020

Choose a reason for hiding this comment

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

Not sure whats up here, but Rails will reject a submission if this property is included, only a naked numeric value seems to be used in the UI.

Copy link
Contributor

@pepopowitz pepopowitz left a comment

Choose a reason for hiding this comment

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

Are there tests around these mutations that prove nothing is changing to the consumer of the API? If not, it would be great to have them, and see them pass both with and without these changes.

@mzikherman
Copy link
Contributor

mzikherman commented Mar 5, 2020

@pepopowitz if I'm understanding right, the schema here in Convection is changing in a breaking way (to any consumers of the GraphQL schema in Convection).

It's just that, it isn't stitched into Metaphysics (so the iOS app uses Metaphysics schema -> Convection data loaders using the REST endpoints). The web app directly uses the Convection REST endpoints.

So, making breaking changes to the Convection GraphQL schema is necessary in order to be able to stitch in without breaking changes to the Metaphysics consumers of that schema (just the iOS app).

Implicit in this is the assumption/discovery(?!) that there are no consumers of the current Convection GraphQL API, and so thus this is ok to break.

Might be possible to confirm no consumers of the GraphQL API endpoint in Papertrail or something?

@damassi
Copy link
Member Author

damassi commented Mar 5, 2020

Implicit in this is the assumption/discovery(?!) that there are no consumers of the current Convection GraphQL API, and so thus this is ok to break.

That's the assumption, and if indeed there are consumers those we can change without worrying too much -- but we cant change MP as thats where our mobile client connects to.

@damassi damassi force-pushed the update-mutations branch 2 times, most recently from fa3fa01 to dd1e699 Compare March 5, 2020 22:08
@damassi damassi force-pushed the update-mutations branch from dd1e699 to 03ca14b Compare March 5, 2020 22:17
Copy link
Member

@jonallured jonallured left a comment

Choose a reason for hiding this comment

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

LGTM 🏆

@damassi damassi merged commit b2461fc into master Mar 5, 2020
@damassi damassi deleted the update-mutations branch March 5, 2020 23:25
@artsy-peril artsy-peril bot mentioned this pull request Mar 5, 2020
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.

4 participants