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

RFC: 175 AppSync Mapping Template Object Model #177

Closed
wants to merge 1 commit into from

Conversation

duarten
Copy link

@duarten duarten commented Jun 17, 2020


title: "RFC: 175 AppSync Mapping Template Object Model"
labels: management/rfc

Rendered version


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

@mergify
Copy link
Contributor

mergify bot commented Jun 17, 2020

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@MrArnoldPalmer MrArnoldPalmer self-requested a review June 17, 2020 23:28
Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer left a comment

Choose a reason for hiding this comment

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

@duarten definitely feels like we are on the right track here. Couple of questions.

  1. I know you've been experimenting with this, is it possible to publish that as a library for experimentation and iteration? Even just looking at any of your existing code could help clarify the model a bit.

  2. If I'm understanding correctly, the reason to use a higher order function like requestTemplate(mt => {}) is that we can give users access to the utilities on the mt object to compose templates. My understanding is those utilities are mostly static, whats the benefit of this over just providing a class with a bunch of static methods with all the utils?

Or is mt (provided as the callback arg) more intelligent, like the utilities present change based on the service the user has created and previous calls to these utilities? IE, it knows about the data sources that you have attached and can tell you if you're trying to make a query that isn't valid based on schema etc?

A holistic solution to these problems would look like:

```ts
requestTemplate(mt => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does requestTemplate exist as a static method on a class somewhere? JSII doesn't support exporting free floating functions because java/c#.

Copy link
Author

Choose a reason for hiding this comment

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

It exists as a static method of some class, e.g. MappingTemplate. I'll make it clear.

Today, although there exists a `MappingTemplate ` abstract class, the only implementation is `StringMappingTemplate`, which completely type-erases the template. Instead, we propose to have a rich hierarchy of `MappingTemplate`s, preserving semantics and meaning as much as possible. For example, we should have a:

```typescript
class TransactionMappingTemplate extends MappingTemplate {
Copy link
Contributor

Choose a reason for hiding this comment

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

In the above example does mt.dynamodb.transactition.putItem construct a class the inherits from TransactionMappingTemplate?

Copy link
Author

Choose a reason for hiding this comment

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

As a different class representing the individual PutItem. Something like

export interface TransactPutItemProps {
    readonly tableName: string
    readonly key: PrimaryKey
    readonly values?: AttributeValues
    readonly cond?: ConditionExpression
    readonly returnValuesOnConditionCheckFailure?: boolean
}

class TransactPutItem extends MappingTemplate {
    constructor(readonly props: TransactPutItemProps) { super() }
    renderTemplate(): string { ... }
}

@MrArnoldPalmer
Copy link
Contributor

Also, I think it would be great if we reference the usability of this solution in higher level constructs. We have talked about a strongly typed "code first" graphql experience on appsync using cdk and I think we should keep that in mind during design.

@duarten
Copy link
Author

duarten commented Jun 29, 2020

Sorry for the late reply, I somehow missed the notification :(

@duarten definitely feels like we are on the right track here. Couple of questions.

Awesome :)

  1. I know you've been experimenting with this, is it possible to publish that as a library for experimentation and iteration? Even just looking at any of your existing code could help clarify the model a bit.

I've quickly pushed https://github.com/duarten/appsync-mapping-templates

Let me know if it's complete enough to make the general direction clear, before I add more code.

  1. If I'm understanding correctly, the reason to use a higher order function like requestTemplate(mt => {}) is that we can give users access to the utilities on the mt object to compose templates. My understanding is those utilities are mostly static, whats the benefit of this over just providing a class with a bunch of static methods with all the utils?

The utilities are static, and we could certainly do that. What led to propose this approach is:

  1. It adds discoverability and guides the user. As in the example repo, we can expose different utilities in the request template (e.g., DynamoDB operations) and in the response template (e.g., access to the result property).

  2. It's more practical, since we have to accumulate the mapping templates in some data structure.

Or is mt (provided as the callback arg) more intelligent, like the utilities present change based on the service the user has created and previous calls to these utilities? IE, it knows about the data sources that you have attached and can tell you if you're trying to make a query that isn't valid based on schema etc?

Yes, I think that should be a long term goal. Unfortunately, generics aren't support in jsii (maybe until Go leaves the 70's and arrives in the 00's), or we could do cool things like passing in the schema of the ctx.args.

@duarten
Copy link
Author

duarten commented Jul 7, 2020

@MrArnoldPalmer What are the next steps here? :) Shall I incorporate the feedback so far into the RFC?

@MrArnoldPalmer
Copy link
Contributor

@duarten yeah go ahead with any changes from feedback so far. I'd also love to work towards getting your repo into a published npm module so I (and anyone else interested) can experiment a bit with my own apps and we can iterate a bit there before solidifying on the design too much. What do you think about that?

@duarten
Copy link
Author

duarten commented Jul 8, 2020

Sounds good!

@duarten duarten force-pushed the feature/appsync-rfc branch 2 times, most recently from 1620d1a to 33b762d Compare July 12, 2020 03:45
@duarten
Copy link
Author

duarten commented Jul 12, 2020

@MrArnoldPalmer Updated the document (heavily), updated the repo, and published packages :)

@duarten duarten force-pushed the feature/appsync-rfc branch 2 times, most recently from ee9e875 to 2b994f4 Compare July 12, 2020 03:50
Signed-off-by: Duarte Nunes <duarte.m.nunes@gmail.com>
@duarten duarten force-pushed the feature/appsync-rfc branch from 2b994f4 to 276c7f9 Compare July 12, 2020 03:51
@MrArnoldPalmer
Copy link
Contributor

Awesome!! I'm gonna play with the packages and I'll add any feedback in the next couple days.

@andrestone
Copy link

@BryanPan342
Copy link

A working (similar) implementation: https://github.com/punchcard/punchcard/tree/master/packages/punchcard/lib/appsync/lang

aw man this looks fantastic.. but it uses generics 😭

@duarten
Copy link
Author

duarten commented Aug 25, 2020

I've made some updates to duarten/appsync-mapping-templates. Most importantly, I integrated it with my AppSync project for some proper testing.

Some sample resolvers:

    requestMappingTemplate: r => {
        r.invoke({
            claimInviteInput: r.util.toJson(
                r.literal({
                    id: r.ctx.arg(Mutation.claimInvite.input.id),
                    claimedBy: r.ctx.identity.username,
                }),
            ),
            userGroup: r.util.toJson(r.util.defaultIfNull(r.ctx.identity.claims.get("cognito:groups"), r.literal([]))),
        })
    },
    responseMappingTemplate: r => r.util.toJson(r.ctx.results)

Where r.invoke() corresponds to MappingTemplate. lambdaRequest (I want to scope this better, like r.dataSource.lambda.invoke()).

    requestMappingTemplate: r => {
        const pfId = r.variable(`PF#${r.util.autoId()}`)
        ensureUserIsArtist(r)
        validateStrSize(r, Mutation.createPortfolio.input.alias, 30, "alias")
        validateStrSize(r, Mutation.createPortfolio.input.name, 30, "name")
        r.dynamoDb.transactWrite({
            puts: [
                {
                    tableName: s.thecubeName,
                    key: {
                        [TheCube.portfolioAlias.Alias]: r.literal(
                            `ALIAS#${r.ctx.arg(Mutation.createPortfolio.input.alias)}`,
                        ),
                        [TheCube.portfolioAlias.PfId]: pfId,
                    },
                    cond: ConditionExpression.attributeNotExists(TheCube.portfolioAlias.Alias),
                },
                {
                    tableName: s.thecubeName,
                    key: { [TheCube.portfolio.PfId]: pfId, [TheCube.portfolio.PfMarker]: r.literal("_") },
                    attributes: {
                        values: {
                            [TheCube.portfolio.Pf]: r.literal("PF"),
                            [TheCube.portfolio.PfName]: r.ctx.arg(Mutation.createPortfolio.input.name),
                        },
                    },
                },
                {
                    tableName: s.thecubeName,
                    key: {
                        [TheCube.portfolioUser.PfId]: pfId,
                        [TheCube.portfolioUser.PfUser]: r.literal(`USER#${r.ctx.identity.username}`),
                    },
                    attributes: {
                        values: {
                            [TheCube.portfolioUser.PfJoinedTs]: now(r),
                            [TheCube.portfolioUser.PfSummary]: r.literal({
                                [TheCube.documents.portfolioSummary.Name]: r.ctx.arg(
                                    Mutation.createPortfolio.input.name,
                                ),
                                [TheCube.documents.portfolioSummary.Alias]: `ALIAS#${r.ctx.arg(
                                    Mutation.createPortfolio.input.alias,
                                )}`,
                            }),
                        },
                    },
                },
            ],
        })
    },

There are still a bunch of things to flesh out, mostly around ergonomics. I haven't made up my mind about requiring values to be VTL Expressions/References, or allowing them to be unknowns, so we can write raw (non-VTL) values instead of wrapping them in r.literal().

A pipeline resolver:

export const GetPortfolioByAliasResolver: PipelineResolver = fs => ({
    typeName: Query.__typename,
    fieldName: Query.getPortfolioByAlias.__self,
    ...Pipeline.before<"pfAlias">((r, input) => {
        r.util.quiet(r.ctx.stash.put(input.pfAlias, `ALIAS#${r.ctx.arg(Query.getPortfolioByAlias.portfolioAlias)}`))
        r.literal({})
    })
        .chain(fs.TranslateAlias)
        .chain(fs.GetPortfolioById)
        .after(r => r.util.toJson(r.ctx.results)),
})

(This one is using some custom scaffolding for pipeline resolvers, where each function statically specifies its dependencies and contributions, so the Pipeline class can enforce that all dependencies are met).

Overall, I like that I have discoverability and that I barely have to write any strings.

I want to fully mirror VTL/Java primitives (strings, arrays, maps), plus the well-known maps provided by AppSync's context, in the library. Now we have to write str.invoke("lastIndexOf", "#"), but we should write str.lastIndexOf("#").

This requires str to not be type-erased. That's achievable within the universe of known types, but breaks when dealing with arbitrarily shaped data in the GraphQL request and data source response. For those cases, I'm thinking of adding coercing versions of some methods. For example, MapReference#get() returns a Reference, but getMap() should return another MapReference (same for the not-yet-written ArrayReference#index() and indexMap()).

The only wrinkle are relational and arithmetic expressions, for which there isn't a 1-1 correspondence with Typescript operators (like there is for calling methods). The solution I found was to write operators as methods: index() for the [] operator, add() for +, etc. Now we would write str.invoke("lastIndexOf", "#").add(1). It's not pretty, and having all those methods attached to pretty much all types pollutes the interface. On the other hand, doing something like invoke("lastIndexOf", "#").math.add(1) feels very verbose.

Thoughts?

@duarten
Copy link
Author

duarten commented Sep 24, 2020

Ping? :)

@duarten
Copy link
Author

duarten commented Feb 24, 2021

Closing as the upcoming Javascript resolvers provide a good alternative to VTL, deprecating this RFC.

@duarten duarten closed this Feb 24, 2021
@BryanPan342
Copy link

BryanPan342 commented Feb 24, 2021

EDIT

Never mind I found it! For those who are curious Appsync RFC. This is so exciting! I hope this will make it significantly easier to GA the AppSync module 😊


Closing as the upcoming Javascript resolvers provide a good alternative to VTL, deprecating this RFC.

Woah no way! @duarten where did you find out about this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants