Adding contracts for Cognito Trigger Events

[jon-armen]

Issue #, if available: #1050

Description of changes: Adding contracts for Cognito Trigger Events

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

[jon-armen]

@normj , this is ready for review.

[jon-armen]

@normj , @ganeshnj Any updates on when this may be reviewed?

[TobbenTM]

This seems like a great addition, any chance this could get some attention @ganeshnj @normj ?

[ashishdhingra]

@jon-armen Could you please rebase your branch and change target branch to aws:dev?

[jon-armen]

@jon-armen Could you please rebase your branch and change target branch to aws:dev?

@ashishdhingra , done.

[jon-armen]

@ashishdhingra This has been rebased. Any updates on when it may be reviewed / released?

[ganeshnj]

Thanks for the RP, apologies for picking this late.

How this change is integration tested? I understand from the change, POCOs are created using the docs available but it has proven be wrong lately because of staled version of docs.

If you have not tested the integration, could you?

Thanks again.

[jon-armen]

Thanks for the RP, apologies for picking this late.

How this change is integration tested? I understand from the change, POCOs are created using the docs available but it has proven be wrong lately because of staled version of docs.

If you have not tested the integration, could you?

Thanks again.

@ganeshnj I haven't integration tested all of these classes, but I have tested the custom email sender and custom message events. Do you have a standard framework you would prefer me to use for integration testing these? I didn't see anything in the repo for integration testing. These classes are based on the docs, so it is possible it could be wrong if the docs are incorrect. Hopefully if it's a case of the docs being stale that would just mean I'm missing something that could be added at a later date.

[BryanCrotaz]

I've tested the pre token generation events and they work apart from being PascalCase when they should be camelCase. Interestingly they are case insensitive when deserialising but AWS is case sensitive when reading the serialised output.

I've added a review saying how to fix.

[ganeshnj]

@ganeshnj I haven't integration tested all of these classes, but I have tested the custom email sender and custom message events. Do you have a standard framework you would prefer me to use for integration testing these? I didn't see anything in the repo for integration testing. These classes are based on the docs, so it is possible it could be wrong if the docs are incorrect. Hopefully if it's a case of the docs being stale that would just mean I'm missing something that could be added at a later date.

I mean by integration tests here is, setting up a trigger in Lambda with Cognito and being able to invoke the Lambda. Also make sure all the properties send by the trigger are correctly deserialized.

[BryanCrotaz]

I’ve done that with the pre token trigger and the input is deserialised correctly. The output is not.

Bryan Crotaz Silver Curve

On 18 May 2022, at 18:56, Ganesh Jangir @.***> wrote:



I mean by integration tests here is, setting up a trigger in Lambda with Cognito and being able to invoke the Lambda. Also make sure all the properties send by the trigger are correctly deserialized.

— Reply to this email directly, view it on GitHub https://github.com/aws/aws-lambda-dotnet/pull/1051#issuecomment-1130319121, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPMKC2VSF2PJ7JISQV3THLVKUVL5ANCNFSM5MAOO3ZA . You are receiving this because you commented.Message ID: @.***>

[Maxwe11]

Api design with separate models per event type looks nice (on the picture :), but it requires creating separate Lambdas for each event. In a real-world scenario, I'd prefer to have a single lambda that can handle multiple event types.

[Simonl9l]

is there any chance this pull requests will be merged any soon... it would be great to have the ability to implement dotnet versions of these events.

[michaelakin]

Please review and merge this or 733 soon. The contract in the Nuget package Amazon.Lambda.CognitoEvents is not valid any longer and causes an exception. Can you please remove this package from Nuget?

[ashishdhingra]

I’ve done that with the pre token trigger and the input is deserialised correctly. The output is not.

@jon-armen As @ganeshnj pointed out, please test all the events end-to-end to ensure these work. We have seen issues in the past where changes were not fully integration tested resulting on post-release issues.

[jon-armen]

@ashishdhingra , I'll see about putting together a suite of integration tests. Hopefully I can get to it this evening, otherwise, in the next few days.

[normj]

This PR has been released as part of version 2.1.0 of Amazon.Lambda.CognitoEvents. Thanks for the PR!