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

Maintenance: AppSyncResolverEvent uses typeName instead of parentTypeName #5560

Closed
1 of 2 tasks
aminalaee opened this issue Nov 14, 2024 · 5 comments · Fixed by #5801
Closed
1 of 2 tasks

Maintenance: AppSyncResolverEvent uses typeName instead of parentTypeName #5560

aminalaee opened this issue Nov 14, 2024 · 5 comments · Fixed by #5801
Assignees
Labels
event_handlers internal Maintenance changes

Comments

@aminalaee
Copy link
Contributor

Why is this needed?

I'm new to GraphQL in general, but I think in the original PR #323 the AppSyncResolverEvent was based on AWS Amplify, so instead of reading parentTypeName, we are reading typeName.

It confused me that the code is consistent (in some ways) with AWS AppSync Context info:

type Info = {
  fieldName: string;
  parentTypeName: string;
  variables: any;
  selectionSetList: string[];
  selectionSetGraphQL: string;
};

But we are assigning variables in a way they are not clear:

info = {"fieldName": self.get("fieldName"), "parentTypeName": self.get("typeName")}

And we define different property names for reading the parentTypeName:

@property
def parent_type_name(self) -> str:
"""The name of the parent type for the field that is currently being resolved."""
return self["parentTypeName"]

@property
def type_name(self) -> str:
"""The name of the parent type for the field that is currently being resolved."""
return self.info.parent_type_name

This has the side-effect that when setting up an AppSync resolved in AWS like the following, it does not work out of the box and you have to pass typeName to make it work:

import { util } from '@aws-appsync/utils';

/**
 * Sends a request to a Lambda function. Passes all information about the request from the `info` object.
 * @param {import('@aws-appsync/utils').Context} ctx the context
 * @returns {import('@aws-appsync/utils').LambdaRequest} the request
 */
export function request(ctx) {
    return {
        operation: 'Invoke',
        payload: {
            fieldName: ctx.info.fieldName,
            parentTypeName: ctx.info.parentTypeName,
            typeName: ctx.info.parentTypeName,  // Extra field which is exactly the same as the one above
            arguments: {}, // Extra field
            variables: ctx.info.variables,
            selectionSetList: ctx.info.selectionSetList,
            selectionSetGraphQL: ctx.info.selectionSetGraphQL,
        },
    };
}

/**
 * Process a Lambda function response
 * @param {import('@aws-appsync/utils').Context} ctx the context
 * @returns {*} the Lambda function response
 */
export function response(ctx) {
    const { result, error } = ctx;
    if (error) {
        util.error(error.message, error.type, result);
    }
    return result;
}

Which area does this relate to?

Event Source Data Classes

Solution

I'm not sure if it was the intention and it should stay the same, but I think we can improve this in different ways:

  • If the old approach is not desired, we can just deprecate it and mention parent_type_name should be used
  • If the old approach is still valid, we can try to read info from parentTypeName after typeName is checked. This way it is consistent with the AWS Resolver context here and with the Amplify structure

I can prepare a PR if the suggestion makes sense.

Acknowledgment

@leandrodamascena
Copy link
Contributor

Hi @aminalaee! Thanks for reporting this! I'll check on Monday, but I think we have a bug here.

The reason I'll check out this is because we use some of these fields in our AppSync Event Handler utility and I don't know if we have this bug there as well, so I want to test everything together.

@leandrodamascena leandrodamascena self-assigned this Nov 16, 2024
@leandrodamascena leandrodamascena added event_handlers and removed triage Pending triage from maintainers labels Nov 16, 2024
@leandrodamascena leandrodamascena moved this from Triage to Pending review in Powertools for AWS Lambda (Python) Nov 16, 2024
@aminalaee
Copy link
Contributor Author

Thanks, And about your point of this usage, I think it's kind of the same that in other places type_name is used where we mean parent_type_name. To me seems like this was only to follow AWS Amplify and seems like that is not consistent with AWS Resolver Context.

def resolver(self, type_name: str = "*", field_name: str | None = None) -> Callable:

https://docs.amplify.aws/gen1/react/build-a-backend/graphqlapi/custom-business-logic/#structure-of-the-function-event

@leandrodamascena
Copy link
Contributor

Hi @aminalaee! Sorry for the late reply, I'm working on a fix until the end of this week and will let you know when we release it.

Thanks a lot for pointing that out.

@leandrodamascena
Copy link
Contributor

Hey @aminalaee! I sent a PR to fix this, and it will be released in the first (or second) week of January.
The old approach is still valid. We can check parentTypeName before typeName, which will help keep the field names more consistent.

I apologize for the delay in fixing this. We have been very busy this month and last month.

@leandrodamascena leandrodamascena moved this from Pending review to Working on it in Powertools for AWS Lambda (Python) Dec 28, 2024
@github-project-automation github-project-automation bot moved this from Working on it to Coming soon in Powertools for AWS Lambda (Python) Dec 30, 2024
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.

@github-actions github-actions bot added the pending-release Fix or implementation already in dev waiting to be released label Dec 30, 2024
@leandrodamascena leandrodamascena moved this from Coming soon to Shipped in Powertools for AWS Lambda (Python) Jan 20, 2025
@leandrodamascena leandrodamascena removed the pending-release Fix or implementation already in dev waiting to be released label Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
event_handlers internal Maintenance changes
Projects
Status: Shipped
2 participants