Conversation
|
@PankajRawat333 Thanks for the PR. Please refer type information from https://www.npmjs.com/package/@types/aws-lambda?activeTab=code. Source: https://github.com/aws-samples/cdk-typescript-appsync-lambda/blob/main/src/Resolver/src/resolver.ts#L24. |
|
@ashishdhingra I have updated type according to above link. |
@PankajRawat333 Thanks for addressing comments for the PR. Discussed this PR with the team. @normj would have a high level look. Meanwhile, could you also please follow up with the Lambda team to get the schema details and data types for each field (since these are not documented in detail)? We would need to make sure to use the correct types for each field to avoid making any breaking changes in future for missed scenario. |
|
@ashishdhingra Received the AppSync schema from the internal group in the aws-powertool repository. All type information was accurate, but I renamed a few classes based on the schema. |
@PankajRawat333 Provided some review comments. Also update test cases (and JSON file names) accordingly while addressing the review comments. |
| /// <summary> | ||
| /// Gets or sets the variables passed to the GraphQL operation. | ||
| /// </summary> | ||
| public Dictionary<string, object> Variables { get; set; } |
There was a problem hiding this comment.
Should the data type for key value be object or string? Refer https://docs.powertools.aws.dev/lambda/python/1.15.0/api/utilities/data_classes/appsync_resolver_event.html#aws_lambda_powertools.utilities.data_classes.appsync_resolver_event.AppSyncResolverEventInfo.variables.
There was a problem hiding this comment.
Per https://www.npmjs.com/package/@types/aws-lambda?activeTab=code, the data type for value is any. So we are good for it to be object.
| /// <summary> | ||
| /// Represents the event payload received from AWS AppSync. | ||
| /// </summary> | ||
| public class AppSyncEvent<TArguments> |
There was a problem hiding this comment.
This appears to be a Resolver event. Should we change the name to AppSyncResolverEvent<TArguments>? Refer:
- https://www.npmjs.com/package/@types/aws-lambda?activeTab=code
- https://github.com/aws-powertools/powertools-lambda-typescript/blob/09f0aaaf92233d326acd9e5fbd21a5c241cdbfe7/packages/parser/src/schemas/appsync.ts#L108
We would also need to change documentation comment for this class, update Readme and it's example.
Is AppSyncAuthorizerEvent outside the scope for this PR for now?
There was a problem hiding this comment.
@PankajRawat333 Please advise if AppSyncAuthorizerEvent outside the scope for this PR for now.
There was a problem hiding this comment.
Updated. Added all type of appsync authorization
There was a problem hiding this comment.
I was asking to validate if AppSyncAuthorizerEvent (and AppSyncAuthorizerResult) it out of scope for now, refer such event in https://www.npmjs.com/package/@types/aws-lambda?activeTab=code.
There was a problem hiding this comment.
@ashishdhingra Initially, I was considering not including this, but after reviewing the documentation, I don't see any problem. Added AppSyncAuthorizerEvent & AppSyncAuthorizerResult now.
| /// <summary> | ||
| /// Represents Amazon Cognito User Pools authorization identity for AppSync | ||
| /// </summary> | ||
| public class AppSyncCognitoIdentity |
There was a problem hiding this comment.
Why we didn't implement AppSyncOidcIdentity and AppSyncLambdaIdentity? Please refer below links:
There was a problem hiding this comment.
Added AppSyncOidcIdentity and AppSyncLambdaIdentity
|
|
||
| # Sample Function | ||
|
|
||
| The following is a sample class and Lambda function that receives AppSync event record data as an `appSyncResolverEvent` and logs some of the incoming event data. (Note that by default anything written to Console will be logged as CloudWatch Logs events.) |
There was a problem hiding this comment.
Please change AppSync to AppSync resolver
|
@PankajRawat333 PR builds line locally and
Also,
|
| "Projects": [ | ||
| { | ||
| "Name": "Amazon.Lambda.AppSyncEvents", | ||
| "Type": "Patch", |
There was a problem hiding this comment.
The increment type should have been Major instead of Patch since this is a new package. You should have used --increment-type Major parameter while using autover (It's not well documented though, 😞)
There was a problem hiding this comment.
Also, I was asking to validate if validate if AppSyncAuthorizerEvent (and AppSyncAuthorizerResult) are out of scope for now, refer such event and result type at https://www.npmjs.com/package/@types/aws-lambda?activeTab=code.
There was a problem hiding this comment.
The increment type should have been
Majorinstead ofPatchsince this is a new package. You should have used--increment-type Majorparameter while usingautover(It's not well documented though, 😞)
@philasmar Please review comment #1939 (comment). Should we be specifying version in .csproj file for 1st version? Also should the --increment-type be Major for this use case.
| <Description>Amazon Lambda .NET Core support - AWS AppSync package.</Description> | ||
| <TargetFrameworks>netstandard2.0;netcoreapp3.1;net8.0</TargetFrameworks> | ||
| <AssemblyTitle>Amazon.Lambda.AppSyncEvents</AssemblyTitle> | ||
| <Version>1.0.0</Version> |
There was a problem hiding this comment.
I'm unsure if we should specify version when using 1st autover change file. CCing @philasmar for inputs.
There was a problem hiding this comment.
You should specify a version below 1.0.0, like 0.0.1 and then use the increment type Major which would bump it to 1.0.0.
There was a problem hiding this comment.
@philasmar Help me to understand your comment. My current .csproj file version is 0.0.1. Running autover with Major increment type won't automatically change the version in the .csproj file and just add new changefile.
There was a problem hiding this comment.
@PankajRawat333 Which autover command are you using?
The actual versioning of the project will happen when we start a release. You do not need to worry about incrementing it to 1.0.0 yourself. The 2 things that need to be done from your end prior to us merging this PR is set your .csproj Version to 0.0.1, which it is, and have a change file included in the PR with an increment type set to Major, which you don't. You just need to update that from Patch to Major. If you wanted to recreate the file, you could run:
autover change --increment-type Major --project-name Amazon.Lambda.AppSyncEvents -m "Your changelog message"
Make sure to delete the old change file if you decide to create a new one.
From our end, once the PR is merged in, we will create a release which will automatically run:
autover version
autover changelog
This will look at the available change files and perform the needed version increment for the projects.
There was a problem hiding this comment.
Also please add the new project in this section https://github.com/aws/aws-lambda-dotnet/blob/master/CONTRIBUTING.md#adding-a-change-file-to-your-contribution-branch
There was a problem hiding this comment.
Thanks @philasmar, Now created new changefile with increment-type Major and updated readme.
| public class AppSyncAuthorizerResult | ||
| { | ||
| // Indicates if the request is authorized | ||
| #if NETSTANDARD_2_0 |
There was a problem hiding this comment.
@ashishdhingra Added JsonProperty attribute, since AWS AppSync is case sensitive. I hope this is correct way to support both type of serializer.
There was a problem hiding this comment.
Almost there.
Please follow approach at
Amazon.Lambda.KinesisEvents also targets netstandard2.0;netcoreapp3.1;net8.0.
Also, readme doesn't demonstrate AppSyncAuthorizerEvent and AppSyncAuthorizerResult. Perhaps, we should add it as well (refer https://docs.aws.amazon.com/appsync/latest/devguide/security-authz.html). And also update test cases.
Once this is done, I will pull PR branch locally and validate.
There was a problem hiding this comment.
@ashishdhingra - Pushed the following changes:
- Updated README file to show how to use custom lambda authorizer
- Used
#if NETCOREAPP3_1_OR_GREATERdirective inAppSyncAuthorizerResultclass - Added unit tests for
AppSyncAuthorizerEvent&AppSyncAuthorizerResultclasses - Updated XML documentation comments
- Simplified csproj file as per other event projects
Added new tests, updated readme and incorporated review comments](9b8f7ae)
| public class AppSyncAuthorizerResult | ||
| { | ||
| // Indicates if the request is authorized | ||
| #if NETSTANDARD_2_0 |
There was a problem hiding this comment.
Almost there.
Please follow approach at
Amazon.Lambda.KinesisEvents also targets netstandard2.0;netcoreapp3.1;net8.0.
Also, readme doesn't demonstrate AppSyncAuthorizerEvent and AppSyncAuthorizerResult. Perhaps, we should add it as well (refer https://docs.aws.amazon.com/appsync/latest/devguide/security-authz.html). And also update test cases.
Once this is done, I will pull PR branch locally and validate.
| ## Example of Custom Lambda Authorizer | ||
| This example demonstrates how to implement a custom Lambda authorizer for AppSync using the AppSync Events package. The authorizer function receives an `AppSyncAuthorizerEvent` containing the authorization token and request context. It returns an `AppSyncAuthorizerResult` that determines whether the request is authorized and includes additional context. | ||
|
|
||
| The function also provides some data in the `resolverContext` object. This information is available in the AppSync resolver’s context `identity` object. |
There was a problem hiding this comment.
This information is available in the AppSync resolver’s context
identityobject.
What is the identity object we are talking about? ResolverContext in the AppSyncAuthorizerResult is a Dictionary. Or is it some other source which might be missing here?
|
Tests pass locally with latest changes from PR. |
|
@PankajRawat333 / @ankushjain358 Please review comment #1939 (comment). Else PR looks good. Test pass locally. CC @philasmar / @normj for 2nd review. |
ashishdhingra
left a comment
There was a problem hiding this comment.
Looks complete to me. Need to have 2nd review as well.
| "Name": "Amazon.Lambda.TestTool", | ||
| "Path": "Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Amazon.Lambda.TestTool.csproj" | ||
| }, | ||
| { |
| <Import Project="..\..\..\buildtools\common.props" /> | ||
|
|
||
| <PropertyGroup> | ||
| <Description>Amazon Lambda .NET Core support - AWS AppSync package.</Description> |
There was a problem hiding this comment.
nit: we don't need to mention "Core"
|
|
||
| <PropertyGroup> | ||
| <Description>Amazon Lambda .NET Core support - AWS AppSync package.</Description> | ||
| <TargetFrameworks>netstandard2.0;netcoreapp3.1;net8.0</TargetFrameworks> |
There was a problem hiding this comment.
Since this is a new package, we should only target net8.0
There was a problem hiding this comment.
NET 8 test was written in the 'EventsTests.NET6' project, so I included both net6.0 and net8.0
There was a problem hiding this comment.
We shouldn't add a "net6.0" target. For the tests, you could create a 'EventsTests.NET8' project and add the tests there.
There was a problem hiding this comment.
Added new project - EventsTests.NET8
| <PackageTags>AWS;Amazon;Lambda</PackageTags> | ||
| </PropertyGroup> | ||
|
|
||
| <PropertyGroup Condition="'$(TargetFramework)' == 'net8.0'"> |
There was a problem hiding this comment.
No need for the condition if we only target net8.0
There was a problem hiding this comment.
This condition should only be removed if the targets are net8 and higher. Right now, you are targeting both net6 and net8. You should drop net6.
There was a problem hiding this comment.
Now target framework is only net8.0
| /// Indicates if the request is authorized | ||
| /// </summary> | ||
| [DataMember(Name = "isAuthorized")] | ||
| #if NETCOREAPP3_1_OR_GREATER |
There was a problem hiding this comment.
not needed once we target net8
| @@ -0,0 +1,16 @@ | |||
| using System.Collections.Generic; | |||
|
|
|||
| namespace Amazon.Lambda.AppSyncEvents | |||
There was a problem hiding this comment.
nit: use top level statement
| namespace Amazon.Lambda.AppSyncEvents | ||
| { | ||
| /// <summary> | ||
| /// Represents AWS LAMBDA authorization identity for AppSync |
There was a problem hiding this comment.
nit: why is "LAMBDA" all caps?
| @@ -0,0 +1,25 @@ | |||
| using System.Collections.Generic; | |||
|
|
|||
| namespace Amazon.Lambda.AppSyncEvents | |||
There was a problem hiding this comment.
nit: use top level statement
| namespace Amazon.Lambda.AppSyncEvents | ||
| { | ||
| /// <summary> | ||
| /// Represents OPENID CONNECT authorization identity for AppSync |
There was a problem hiding this comment.
nit: why is "OPENID CONNECT" all caps?
| @@ -0,0 +1,104 @@ | |||
| using System.Collections.Generic; | |||
|
|
|||
| namespace Amazon.Lambda.AppSyncEvents | |||
There was a problem hiding this comment.
nit: use top level statement
| <WarningsAsErrors>IL2026,IL2067,IL2075</WarningsAsErrors> | ||
| <IsTrimmable>true</IsTrimmable> | ||
| <PropertyGroup> | ||
| <WarningsAsErrors>IL2026,IL2067,IL2075,IL3050</WarningsAsErrors> |
There was a problem hiding this comment.
Removed, added by mistake
|
|
||
| <PropertyGroup> | ||
| <TargetFrameworks>net8.0</TargetFrameworks> | ||
| <PackageId>EventsTests31</PackageId> |
There was a problem hiding this comment.
Update this to EventsTests8
|
The package https://www.nuget.org/packages/Amazon.Lambda.AppSyncEvents has been released. Thank you for your contribution! |
Issue #, if available:
#1938
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.