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

feat(appsync): implement resolvable fields for code-first schema #9660

Merged
merged 71 commits into from
Aug 24, 2020

Conversation

BryanPan342
Copy link
Contributor

@BryanPan342 BryanPan342 commented Aug 13, 2020

Implemented interfaces and resolvable fields for code-first schema.

Field extends GraphqlType and will allow you to define arguments.

Field Example
type Node {
  test(argument: string): String
}

The CDK code required would be:

const field = new appsync.Field(appsync.GraphqlType.string(), {
  args: {
    argument: appsync.GraphqlType.string(),
  },
});
const type = new appsynce.ObjectType('Node', {
  definition: { test: field },
});

ResolvableField extends Field and will allow you to define arguments and its resolvers.
Object Types can have fields that resolve and perform operations on
your backend.

Resolvable Field Example

For example, if we want to create the following type:

type Query {
  get(argument: string): String
}

The CDK code required would be:

const field = new appsync.Field(appsync.GraphqlType.string(), {
  args: {
    argument: appsync.GraphqlType.string(),
  },
  dataSource: api.addNoneDataSource('none'),
  requestMappingTemplate: dummyRequest,
  responseMappingTemplate: dummyResponse,
});
const type = new appsynce.ObjectType('Query', {
  definition: { get: field },
});

</details>

----

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

@BryanPan342 BryanPan342 removed the pr/work-in-progress This PR is a draft and needs further work. label Aug 20, 2020
@BryanPan342
Copy link
Contributor Author

@andrestone @asterikx would love your thoughts on the api!

The README can be found here.

The ResolvableField and Field change would essentially allow for users to implement queries and mutations as a base line!

I made an reference app for the Star Wars API here that you can check out to see a big example of dynamic schema generation.

We are planning to implement a Lazy feature that would make it so that users wouldn't have to use the appendToSchema function at all (planning to have that merged in by next week).

@andrestone
Copy link
Contributor

andrestone commented Aug 21, 2020

Are we missing enum?

Update / other thing(s):

  • SchemaDefinition -> SchemaDefinitionMode ?

@BryanPan342
Copy link
Contributor Author

Are we missing enum?

Yes! Will add this in a PR next week along with input and more comprehensive directives. This PR is just for Resolvable Fields.

Update / other thing(s):

  • SchemaDefinition -> SchemaDefinitionMode ?

I had an idea for another PR but to make the schemaDefinition: Schema. Where Schema has static functions like .fromCode or fromFile or from fromBucket. But this would be for another PR. Thoughts?

@andrestone
Copy link
Contributor

But this would be for another PR. Thoughts?

Sounds great, feels more like CDK.

@BryanPan342
Copy link
Contributor Author

@andrestone does the api feel natural? Like declaring types, fields and linking resolvers inline

@andrestone
Copy link
Contributor

@andrestone does the api feel natural? Like declaring types, fields and linking resolvers inline

I think it's a good lower level abstraction. I guess the important thing here is to create a solid base for L2/L3 constructs that will deliver less generic but more fluid experiences depending on developer preferences.

I couldn't play around much today, though. I'll certainly do it over the weekend.

@BryanPan342
Copy link
Contributor Author

I think it's a good lower level abstraction. I guess the important thing here is to create a solid base for L2/L3 constructs that will deliver less generic but more fluid experiences depending on developer preferences.

I couldn't play around much today, though. I'll certainly do it over the weekend.

Sweet! Yeah next week I plan to add functions like addInterface, addQuery, addMutation, etc. on top of GraphQL Api construct and make all the schema definition synthesizing to happen Lazily.

Hopefully, this will make the experience a lot more fluid!

@andrestone
Copy link
Contributor

andrestone commented Aug 21, 2020

Nice.

There's one thing that I think could be done better or maybe better documented. Since this is mostly a "helper" API, something that will support the development of a "backend" logic instead of infrastructure deployment per se, it would be good to have better defined flows for those two contexts.

For example, during the code-first schema definition "flow", when using most of the helper classes / functions, it makes sense that it all happens out of the "construct programming model" context. So, no need to reference the scope, since we would be outside of the tree. And it's pretty much like that already, with the exception of when you need to create a ResolvableField, because you have to specify a DataSource which is a resource (hence part of the construct tree).

So the suggestion would be to expose an API in Fields(like addResolver, that would set DataSource and templates for the field) so those two flows could be independent.

@BryanPan342
Copy link
Contributor Author

@andrestone thats a great suggestion!

Yeah I think this parallels a lot of what the intention of the code-first approach. Allowing declaration to happen externally allows for dynamic declaration.

Originally, the idea was that users would declare their resolvable fields in construction. So you would create your data sources and what not, and then pass it in (like what I do here in the SWAPI demo).

My only concern is that I don't want people to think they can declare resolvers on interfaces (which only allows for Field and GraphqlType). I would be down to make an api in ResolvableField that allow for addResolver.

@andrestone
Copy link
Contributor

@BryanPan342

How about a method from an abstract class that manages the attachment of the resolvers?

Or (even better?) a DataSource method that would ensure the type of the passed field to be a ResolverField?

@BryanPan342
Copy link
Contributor Author

How about a method from an abstract class that manages the attachment of the resolvers?

At the end of the day, if someone were to try to add a data source to an interface, it still wouldn't create a resolver because the generating function only exists in ObjectType. So in that sense its not a big issue.

Or (even better?) a DataSource method that would ensure the type of the passed field to be a ResolverField?

Im not sure what this means.. I think what we could end up doing, is just add a function to ObjectType that would allow people to attach a data source.

ResolverOptions {
  readonly pipelineConfig?: string[];
  readonly requestMappingTemplate?: MappingTemplate;
  readonly responseMappingTemplate?: MappingTemplate;
}

export class ObjectType extends InterfaceType {

  constructor(name: string, props: ObjectTypeProps) {
  ...
  }

  public attachResolverToField(fieldName: string, dataSource: BaseDataSource, options: ResolverOptions) {
  // This would search your ObjectType for the field name and adjust the definition
  }
}

@andrestone
Copy link
Contributor

Nevermind the other ideas, this snippet looks perfect.

@andrestone
Copy link
Contributor

Hello!

Could we expose pipelineConfig in ResolvableFieldOptions in order to have minimal support for pipeline resolvers using this API?

@BryanPan342
Copy link
Contributor Author

Hello!

Could we expose pipelineConfig in ResolvableFieldOptions in order to have minimal support for pipeline resolvers using this API?

Good point! I was going to add this once AppSync function creation was created but yeah the support here would be nice. I'll add this into the PR with some tests tmrw!

@BryanPan342
Copy link
Contributor Author

@andrestone added implementation for pipelineConfig!

@shivlaks I separated the tests (only for code-first tests) so each one file has one describe :) felt like it would get hairy pretty soon

Copy link
Contributor

@shivlaks shivlaks left a comment

Choose a reason for hiding this comment

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

nice work!

@mergify
Copy link
Contributor

mergify bot commented Aug 24, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: dbdd649
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Aug 24, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 9e3b798 into aws:master Aug 24, 2020
@BryanPan342 BryanPan342 deleted the resolvable-fields branch September 8, 2020 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-appsync Related to AWS AppSync contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants