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(data-classes): AppSync Resolver Event #323

Merged
merged 34 commits into from Mar 12, 2021

Conversation

michaelbrewer
Copy link
Contributor

@michaelbrewer michaelbrewer commented Mar 10, 2021

Description of changes:

Add support for AppSync Resolver Events either Direct or Amplify

from aws_lambda_powertools.utilities.data_classes import AppSyncResolverEvent
from aws_lambda_powertools.utilities.typing import LambdaContext


def handler(event: dict, context: LambdaContext):
    event = AppSyncResolverEvent(event)
    assert event.type_name == "Merchant"
    assert event.field_name == "locations"
    assert event.info.parent_type_name == event.type_name
    assert event.info.field_name == event.field_name

Can also be used as a decorator

from aws_lambda_powertools import Logger
from aws_lambda_powertools.utilities.data_classes import AppSyncResolverEvent
from aws_lambda_powertools.utilities.data_classes.appsync.resolver_utils import (
    AppSyncResolver
)
from aws_lambda_powertools.utilities.data_classes.appsync.scalar_types_utils import (
    make_id,
    aws_date,
    aws_datetime,
    aws_time,
    aws_timestamp,
)

logger = Logger()
app = AppSyncResolver()


@app.resolver(field_name="listLocations", include_event=True)
@app.resolver(type_name="Merchant", field_name="locations", include_event=True)
def get_locations(event: AppSyncResolverEvent, name: str = None):
    logger.info(event)
    return f"returning locations for {name}"


@app.resolver(type_name="Merchant", field_name="anotherField")
def another_field_resolver(count: int):
    return {
        "id": make_id(),
        "date": aws_date(),
        "date_time": aws_datetime(),
        "time": aws_time(),
        "ts": aws_timestamp(),
        "message": f"another_field_resolver with parameter count={count}",
    }


@app.resolver(type_name="Query", field_name="noParams")
def no_params():
    return "no_params has no params"


def handler(event: dict, context):
    return app.resolve(event, context)

This is based on what Amplify produces by default:

Key Description
typeName The name of the parent object type of the field being resolver.
fieldName The name of the field being resolved.
arguments A map containing the arguments passed to the field being resolved.
identity A map containing identity information for the request. Contains a nested key ‘claims’ that will contains the JWT claims if they exist.
source When resolving a nested field in a query, the source contains parent value at runtime. For example when resolving Post.comments, the source will be the Post object.
request The AppSync request object. Contains header information.
prev When using pipeline resolvers, this contains the object returned by the previous function. You can return the previous value for auditing use cases.

Which is a simplification of is possible in the app sync $context :

Key Description
arguments A map containing the arguments passed to the field being resolved.
identity A map containing identity information for the request.
source When resolving a nested field in a query, the source contains parent value at runtime. For example when resolving Post.comments, the source will be the Post object.
request The AppSync request object. Contains header information.
prev A container for the results of this resolver. This field is only available to response mapping templates. (like source)
info An object that contains information about the GraphQL request.
stash The stash is a map that is made available inside each resolver and function mapping template.

Checklist

  • Implement the identity class (API_KEY, AWS_IAM vs AMAZON_COGNITO_USER_POOLS)
  • Decide whether to create a data class specifically for Amplify ?
  • Add support for correlation id
  • Meet tenets criteria
  • Update tests
  • Update docs
  • PR title follows conventional commit semantics

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@heitorlessa heitorlessa added area/utilities feature New feature or functionality labels Mar 10, 2021
@codecov-io
Copy link

codecov-io commented Mar 10, 2021

Codecov Report

Merging #323 (8ba4495) into develop (6a9a554) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           develop     #323    +/-   ##
=========================================
  Coverage    99.88%   99.88%            
=========================================
  Files           92       94     +2     
  Lines         3356     3519   +163     
  Branches       165      172     +7     
=========================================
+ Hits          3352     3515   +163     
  Misses           2        2            
  Partials         2        2            
Impacted Files Coverage Δ
aws_lambda_powertools/logging/correlation_paths.py 100.00% <100.00%> (ø)
...mbda_powertools/utilities/data_classes/__init__.py 100.00% <100.00%> (ø)
...ies/data_classes/appsync/appsync_resolver_event.py 100.00% <100.00%> (ø)
...s/utilities/data_classes/appsync/resolver_utils.py 100.00% <100.00%> (ø)
...lambda_powertools/utilities/data_classes/common.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a9a554...8ba4495. Read the comment docs.

@heitorlessa
Copy link
Contributor

hey @awsed - could you take a quick peek at this PR in case we're missing anything obvious from the Lambda Resolver event object?

Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

It looks mostly great - only one-liner that could be more readable to ease maintenance by others.

Docs are lacking on confirming the two variants of object, as you pointed out - Amplify GraphQL Transformer vs AppSync Direct Lambda Resolvers.

Hopefully @awsed or @dabit3 have a few minutes to confirm whether there is a good complete event available for each in the docs somewhere

@heitorlessa
Copy link
Contributor

Note to self - Validate how AppSync Direct Lambda resolvers event look like compared to what Amplify @function does. AppSync is unlikely to break contract while Amplify @function as at first glance it isn't formalized.

@KoldBrewEd
Copy link

This is a better event with the info object: https://github.com/aws/aws-sam-cli/blob/develop/samcli/lib/generated_sample_events/events/appsync/AppSyncDirectResolver.json

@michaelbrewer
Copy link
Contributor Author

This is a better event with the info object: https://github.com/aws/aws-sam-cli/blob/develop/samcli/lib/generated_sample_events/events/appsync/AppSyncDirectResolver.json

Thanks @awsed i will add that to the test suite, interesting how it differs from what Amplify generates.

@KoldBrewEd
Copy link

@michaelbrewer the info object is missing, check line 60 here https://github.com/aws/aws-sam-cli/blob/ab3dcfae17f971cfbfea3f459ed872ba14cead83/samcli/lib/generated_sample_events/events/appsync/AppSyncDirectResolver.json#L60

@michaelbrewer
Copy link
Contributor Author

@michaelbrewer the info object is missing, check line 60 here https://github.com/aws/aws-sam-cli/blob/ab3dcfae17f971cfbfea3f459ed872ba14cead83/samcli/lib/generated_sample_events/events/appsync/AppSyncDirectResolver.json#L60

yes, that does not come through for Amplify AppSync events.

@michaelbrewer
Copy link
Contributor Author

@heitorlessa can you run through this again

@awsed - i have added all of the optional fields that might be in direct appsync calls.

Copy link
Contributor Author

@michaelbrewer michaelbrewer left a comment

Choose a reason for hiding this comment

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

@heitorlessa hopefully this is looking for merging

@heitorlessa heitorlessa self-assigned this Mar 12, 2021
@heitorlessa
Copy link
Contributor

Looking :) Is the PR description ready? I'll use that for the docs later

@michaelbrewer
Copy link
Contributor Author

Looking :) Is the PR description ready? I'll use that for the docs later

The description progressed over time as i found more things. I can put in a checklist of changes. For the data-classes docs in general we could start to expand on the examples, but that could warrant a who cookbook section.

Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

Some suggestions on wording, a suggestion to follow what you did with Cognito data classes so including utils is cleaner, and a question on whether it's best to leave AppSyncResolver decorator out until we have a more complete written RFC for discussion?

Whatever we decide on AppSyncResolver will open precedent to discuss API Gateway Lightweight decorators too, for things like CORS, HTTP Methods, etc.

docs/utilities/data_classes.md Outdated Show resolved Hide resolved
docs/utilities/data_classes.md Outdated Show resolved Hide resolved
Comment on lines +31 to +33
app = AppSyncResolver()

@app.resolver(field_name="createSomething", include_context=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this would be considered a Dataclass. I see two options:

  1. Discuss some design options in a RFC for lightweight API handling
  2. Expand Event Source Data Classes description to include utilities in it - Current description:The event source data classes utility provides classes describing the schema of common Lambda events triggers.

The first is my preference as we might find other use cases, UX, as we put this into written form.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@heitorlessa kind of started that here:
#324

But yes, in general having lightweight decorators help. Maybe like the SQS batch processing?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's missing the RFC body like summary, drawbacks, alternatives, etc ;) one step a time

Michael Brewer and others added 5 commits March 12, 2021 07:26
Co-authored-by: Heitor Lessa <heitor.lessa@hotmail.com>
Co-authored-by: Heitor Lessa <heitor.lessa@hotmail.com>
…solver_event.py

Co-authored-by: Heitor Lessa <heitor.lessa@hotmail.com>
@michaelbrewer
Copy link
Contributor Author

Some suggestions on wording, a suggestion to follow what you did with Cognito data classes so including utils is cleaner, and a question on whether it's best to leave AppSyncResolver decorator out until we have a more complete written RFC for discussion?

Whatever we decide on AppSyncResolver will open precedent to discuss API Gateway Lightweight decorators too, for things like CORS, HTTP Methods, etc.

I was thinking of leaving it as an undocumented or beta feature (but i will feature restructure the locations)

Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

After some thought, let's leave it as-is AppSyncResolver() and undocumented.

We can document that later once that RFC is written. That way, we add AppSync support within the data class, and have a good initial draft implementation of an AppSync Resolver ready to go once we find a better home for it.

To account for the utils() bit in AppSync data class, please make those changes in the Event Source Data Classes top description to include utilities too, so it doesn't get ruled out of scope for anything beyond a schema like this now

@heitorlessa heitorlessa added this to the 1.12.0 milestone Mar 12, 2021
Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

Great, thank you!! I'll merge it as soon as the top text in the Data classes change to increase its scope to support utilities for event sources

Comment on lines +31 to +33
app = AppSyncResolver()

@app.resolver(field_name="createSomething", include_context=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's missing the RFC body like summary, drawbacks, alternatives, etc ;) one step a time

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

Successfully merging this pull request may close these issues.

None yet

4 participants