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

Feature request: support BatchInvoke when using AppSyncResolver #1303

Closed
2 tasks done
cponfick opened this issue Jul 15, 2022 · 17 comments · Fixed by #1998
Closed
2 tasks done

Feature request: support BatchInvoke when using AppSyncResolver #1303

cponfick opened this issue Jul 15, 2022 · 17 comments · Fixed by #1998
Assignees
Labels

Comments

@cponfick
Copy link

Use case

Currently, an event is expected to be of type Dict[str,Any]. When using 'BatchInvoke' in a direct lambda resolver, the event becomes type List[Dict[str,Any]]. Hence, it is not possible to route the event to the expected resolver.

A use case is given by the official dev guide. Without the ability to use 'BatchInvoke' users will run into n + 1 problems, when using appsync-GraphQl.

Solution/User Experience

I am not 100% sure how a good solution would look like. Maybe it would be possible to use the AppSyncResolverEvent to handle one individual event, or a list of events.
Given the described use case, the events should usually only diverge in the source.

Alternative solutions

No response

Acknowledgment

@cponfick cponfick added feature-request feature request triage Pending triage from maintainers labels Jul 15, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Jul 15, 2022

Thanks for opening your first issue here! We'll come back to you as soon as we can.

@heitorlessa
Copy link
Contributor

hey @cponfick-bhs thank you for raising this, it makes sense. We'd need to do some digging and experiment with a few UX - would you be able to share a sample event with batch resolver?

That will help speed this up as we'd need to create some tests, find most optimal type definitions, check whether we need some additional fields/methods, and then refactor in a non-breaking change fashion.

Thanks a lot!

@heitorlessa heitorlessa added area/event_handlers and removed triage Pending triage from maintainers labels Jul 18, 2022
@cponfick
Copy link
Author

cponfick commented Jul 18, 2022

hey @cponfick-bhs thank you for raising this, it makes sense. We'd need to do some digging and experiment with a few UX - would you be able to share a sample event with batch resolver?

Sure, I can construct an example.
Assume the following GraphQl-Schema:

type Query{
   getProducts(): [Product]
}

type Product {
   name: String
   comments: [Comment!]
}

type Comment {
   text: String!
   userName: String!
}

The resolvers are attached as follows:

  • ParentTypeName: Query and FieldName: getProducts
  • ParentTypeName: Product and FieldName: comments

When using the usual Invoke, Appsync creates one event for the Query:

{
       "arguments": {},
       "identity": {
           "claims": {},
           "defaultAuthStrategy": "ALLOW",
           "groups": null,
           "issuer": "",
           "sourceIp": [""],
           "sub": "",
           "username": ""
       },
       "source": null,
       "request": {
           "headers": {},
           "domainName": null
       },
       "prev": null,
       "info": {
           "selectionSetList":["EVERYTHING"],
           "fieldName": "getProducts",
           "parentTypeName": "Query",
           "variables": {}
       },
       "stash": {}
   }

and for every product, an additional event and lambda invocation for the comments:

{
        "arguments": {},
        "identity": {
            "claims": {},
            "defaultAuthStrategy": "ALLOW",
            "groups": null,
            "issuer": "",
            "sourceIp": [""],
            "sub": "",
            "username": ""
        },
        "source": {THE FIELDS OF THE PRODUCT},
        "request": {
            "headers": {},
            "domainName": null
        },
        "prev": null,
        "info": {
            "selectionSetList":["EVERYTHING"],
            "fieldName": "comments",
            "parentTypeName": "Product",
            "variables": {}
        },
        "stash": {}
    }

Hence, for every Product an additional lambda invoke is executed.

When using "BatchInvoke", the first event remains the same, but the calls that return the comments for each product are "merged" into one lambda call with a list of events.

[{
        "arguments": {},
        "identity": {
            "claims": {},
            "defaultAuthStrategy": "ALLOW",
            "groups": null,
            "issuer": "",
            "sourceIp": [""],
            "sub": "",
            "username": ""
        },
        "source": {THE FIELDS OF THE PRODUCT 0},
        "request": {
            "headers": {},
            "domainName": null
        },
        "prev": null,
        "info": {
            "selectionSetList":["EVERYTHING"],
            "fieldName": "comments",
            "parentTypeName": "Product",
            "variables": {}
        },
        "stash": {}
    },
{
        "arguments": {},
        "identity": {
            "claims": {},
            "defaultAuthStrategy": "ALLOW",
            "groups": null,
            "issuer": "",
            "sourceIp": [""],
            "sub": "",
            "username": ""
        },
        "source": {THE FIELDS OF THE PRODUCT 1},
        "request": {
            "headers": {},
            "domainName": null
        },
        "prev": null,
        "info": {
            "selectionSetList":["EVERYTHING"],
            "fieldName": "comments",
            "parentTypeName": "Product",
            "variables": {}
        },
        "stash": {}
    },
{
        "arguments": {},
        "identity": {
            "claims": {},
            "defaultAuthStrategy": "ALLOW",
            "groups": null,
            "issuer": "",
            "sourceIp": [""],
            "sub": "",
            "username": ""
        },
        "source": {THE FIELDS OF THE PRODUCT 2},
        "request": {
            "headers": {},
            "domainName": null
        },
        "prev": null,
        "info": {
            "selectionSetList":["EVERYTHING"],
            "fieldName": "comments",
            "parentTypeName": "Product",
            "variables": {}
        },
        "stash": {}
    }]

Appsync now expects a list of responses associated with the corresponding sources. The fields of the events depend on the ResponseMappingTemplate used for the resolver.

@heitorlessa I hope this helps a bit.

Work Around

Currently, I work around this issue using the following AppSyncResolverEvent:

class MyAppSyncResolverEvent(appsync.AppSyncResolverEvent):
    def __init__(self, data: Union[dict, list]):
        if isinstance(data, dict):
            super().__init__(data)
        else:
            super().__init__(data[0])
            self._sources = [event.get('source', {}) for event in data]

    @property
    def arguments(self) -> Dict[str, Any]:
        if isinstance(self._data, dict):
            return self["arguments"]
        else:
            return {}

    @property
    def sources(self) -> List[Dict[str, Any]]:
        return self._sources

I must admit that this is not clean, but for now it works.

@heitorlessa heitorlessa added the help wanted Could use a second pair of eyes/hands label Aug 1, 2022
@heitorlessa heitorlessa added the triage Pending triage from maintainers label Aug 22, 2022
@heitorlessa
Copy link
Contributor

Updating the status so we can revisit early next year.

@heitorlessa heitorlessa added the revisit Maintainer to provide update or revisit prioritization in the next cycle label Dec 15, 2022
@thenamanpatwari
Copy link

+1
We had to build to a similar class as created by @cponfick-bhs

@leandrodamascena
Copy link
Contributor

leandrodamascena commented Dec 22, 2022

Hi @cponfick-bhs and @thenamanpatwari!

I was on parental leave and I'm returning to work now. I created an example similar to the one reported here to understand the behavior and look for the best solution.

By the middle of next week I'll be back on this issue with some updates on what I found.

Thanks

@leandrodamascena
Copy link
Contributor

Hi all! Updating here, I still haven't been able to fully analyze this and a possible solution. I'm focused on bringing you an update ASAP.

@heitorlessa
Copy link
Contributor

Leandro and @mploski will be updating it this week

@heitorlessa heitorlessa removed the triage Pending triage from maintainers label Feb 22, 2023
@mploski
Copy link
Contributor

mploski commented Feb 25, 2023

I was able to replicate the issue. Indeed, if we enable batch processing on AppSync side, resolver lambda gets list of events which is something we don't expect. Execution fails with [ERROR] AttributeError: 'list' object has no attribute 'get' in aws_lambda_powertools/utilities/data_classes/appsync_resolver_event.py. Attached SAM project zip with lambda and template that allow us to replicate it reliably. appsync-batch-debug.tar.gz
I will work together with @leandrodamascena on the solution.

@heitorlessa heitorlessa removed the help wanted Could use a second pair of eyes/hands label Mar 10, 2023
@heitorlessa heitorlessa removed the revisit Maintainer to provide update or revisit prioritization in the next cycle label Mar 29, 2023
@heitorlessa heitorlessa linked a pull request Apr 3, 2023 that will close this issue
7 tasks
@sthulb sthulb moved this from Triage to Pending review in Powertools for AWS Lambda (Python) Jun 19, 2023
@leandrodamascena leandrodamascena moved this from Pending review to Working on it in Powertools for AWS Lambda (Python) Jun 20, 2023
@heitorlessa
Copy link
Contributor

UPDATE: @mploski is working on an update by EOW.

@mploski
Copy link
Contributor

mploski commented Jul 10, 2023

UPDATE: @mploski is working on an update by EOW.

Hey sorry for lack of response here before. I was fully booked with other things for last month. We agreed that @leandrodamascena will support me in the further development so we can close it soon

@leandrodamascena
Copy link
Contributor

Hey everyone! We are working on this PR #1998 to add this support as quickly as possible. The most up-to-date situation is:

We will have an internal meeting with the AppSync Service team to discuss error handling when consuming an AppSync batch process. We want to hear from them about use cases and what challenges customers are facing.

After this meeting, I will update this PR. The expectation of having this feature in the next release remains, but I will update this PR if we have any additional news from this meeting.

Thanks

@heitorlessa
Copy link
Contributor

Update. We're going through the final documentation touches to get this released in the next version.

Since last week, we've added partial failure support so parts of the batch item could fail, letting AppSync or your own response mapping templates propagate partial errors accordingly.

from typing import Dict, Optional

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

app = AppSyncResolver()


posts_related = {
    "1": {"title": "post1"},
    "2": {"title": "post2"},
    "3": {"title": "post3"},
}


class PostRelatedNotFound(Exception):
    ...


@app.batch_resolver(type_name="Query", field_name="relatedPosts", raise_on_error=True)  
def related_posts(event: AppSyncResolverEvent, post_id: str) -> Optional[Dict]:
    post_found = posts_related.get(post_id, None)

    if not post_found:
        raise PostRelatedNotFound(f"Unable to find a related post with ID {post_id}.")

    return post_found


def lambda_handler(event, context: LambdaContext) -> dict:
    return app.resolve(event, context)

@heitorlessa
Copy link
Contributor

We're making a patch release today with numerous bugfixes, and the next feature release will have this feature released.

Deepest apologies on the delay here

@heitorlessa heitorlessa moved this from Next iteration to Working on it in Powertools for AWS Lambda (Python) Feb 26, 2024
@heitorlessa
Copy link
Contributor

We're marking this for release this Thursday

Copy link
Contributor

⚠️COMMENT VISIBILITY WARNING⚠️

This issue is now closed. Please be mindful that future comments are hard for our team to see.

If you need more assistance, please either tag a team member or open a new issue that references this one.

If you wish to keep having a conversation with other community members under this issue feel free to do so.

@leandrodamascena
Copy link
Contributor

Hi @cponfick! It took a long time, longer than we expected, but we will be releasing this feature tomorrow! 🚀

Thanks for opening this issue and we hope that it can be helpful in your applications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Coming soon
Development

Successfully merging a pull request may close this issue.

5 participants