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

Remove dependency on ModelRegistry for syncability of models #252

Merged
merged 1 commit into from
Dec 16, 2019

Conversation

lawmicha
Copy link
Contributor

Major changes

This introduces a flag for whether a GraphQLDocument helper method should contain the sync-able fields. Methods like GraphQLSyncQuery by default will be true, and methods like GraphQLGetQuery can be used with syncEnabled parameter.

Minor changes

  • Moved MinimalGraphQLDeleteMutation over to PluginsCore as GraphQLDeleteSyncMutation
  • Updated GraphQLListQuery to include limit and nextToken, in anticipation for ModelBased List query to allow limit/nextToken.

Testing done

  • Ensured the Sync provisioned backend in API integration tests is working as expected, and so went to datastore plugin and updated the calls where necessary, like
let document = GraphQLSubscription(of: modelType,
                                   type: subscriptionType,
                                   syncEnabled: true)
// adding syncEnabled true parameter
  • Ensure Model based APIs are working. by default the models are not syncable until specified to be syncable. since Model based API's should never be used with sync and developer should use DataStore if using sync provisioned backend

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@lawmicha lawmicha requested review from palpatim and wooj2 December 11, 2019 19:06
@lawmicha lawmicha self-assigned this Dec 11, 2019
@lawmicha lawmicha added api Issues related to the API category datastore Issues related to the DataStore category labels Dec 11, 2019
Copy link
Member

@palpatim palpatim left a comment

Choose a reason for hiding this comment

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

My read on this is that syncability is an internal concept, which seems appropriate. The way we're representing it seems odd to me--why do some types have separate "Sync" flavors, while others are extended by virtue of an incoming boolean parameter? I'd like us to think a little more about this to see if this is the right way to model it. But as an incremental change to enable the functionality for now, LGTM.

@@ -48,12 +48,17 @@ class GraphQLSyncBasedTests: XCTestCase {
let uuid = UUID().uuidString
let testMethodName = String("\(#function)".dropLast(2))
let title = testMethodName + "Title"
guard let post = createPost(id: uuid, title: title) else {
guard let createMutation = createPost(id: uuid, title: title) else {
Copy link
Member

Choose a reason for hiding this comment

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

Is createPost creating a Post model or a CreatePost mutation? Please align the names appropriately

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updating to createdPost

Copy link
Contributor

@wooj2 wooj2 left a comment

Choose a reason for hiding this comment

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

Also okay with pushing this in.. we can talk about the interface for subscribe at a later time


public init(of modelType: Model.Type,
type subscriptionType: GraphQLSubscriptionType) {
type subscriptionType: GraphQLSubscriptionType,
syncEnabled: Bool = false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it makes sense to expose a syncEnabled flag for this class, right? Seems like if we have a subscription with a model, syncEnabled has to be true, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a backend can be provisioned with/without sync so the graphql document can be constructed with sync related fields or without by using this syncEnabled parameter

@lawmicha
Copy link
Contributor Author

My read on this is that syncability is an internal concept, which seems appropriate. The way we're representing it seems odd to me--why do some types have separate "Sync" flavors, while others are extended by virtue of an incoming boolean parameter? I'd like us to think a little more about this to see if this is the right way to model it. But as an incremental change to enable the functionality for now, LGTM.

Yes I agree, it is odd that there are specific types to build in the sync fields and then there are types that just take in the syncEnabled parameter. I can see these types getting another overhaul, especially with the @auth directive.

@lawmicha lawmicha changed the base branch from lawmicha/api-integ-tests-1 to master December 16, 2019 17:08
@lawmicha lawmicha changed the base branch from master to lawmicha/api-integ-tests-1 December 16, 2019 17:08
@lawmicha lawmicha force-pushed the lawmicha/has-syncable-flag branch from e437ec0 to fcc497a Compare December 16, 2019 17:17
@lawmicha lawmicha changed the base branch from lawmicha/api-integ-tests-1 to master December 16, 2019 17:17
…ction set.

This introduces a flag for whether a GraphQLDocument helper method should contain the sync-able fields. Methods like GraphQLSyncQuery by default will be true, and methods like GraphQLGetQuery can be used with syncEnabled parameter.
@lawmicha lawmicha force-pushed the lawmicha/has-syncable-flag branch from fcc497a to f3b683c Compare December 16, 2019 17:19
@lawmicha lawmicha merged commit d31e4cf into master Dec 16, 2019
@palpatim palpatim deleted the lawmicha/has-syncable-flag branch December 24, 2019 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issues related to the API category datastore Issues related to the DataStore category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants