Adding contracts for Cognito Trigger Events #1051
Conversation
jon-armen
force-pushed
the
CognitoTriggers
branch
3 times, most recently
from
4637353
to
feea1ad
Compare
jon-armen
force-pushed
the
CognitoTriggers
branch
8 times, most recently
from
703c9c5
to
77329a5
Compare
|
@normj , this is ready for review. |
|
@jon-armen Could you please rebase your branch and change target branch to |
jon-armen
force-pushed
the
CognitoTriggers
branch
2 times, most recently
from
b201517
to
624193d
Compare
PreSignup PostConfirmation PreAuthentication PostAuthentication DefineAuthChallenge CreateAuthChallenge VerifyAuthChallenge PreTokenGeneration MigrateUser CustomMessage CustomEmailSender CustomSmsSender
jon-armen
force-pushed
the
CognitoTriggers
branch
from
624193d
to
ffd1098
Compare
@ashishdhingra , done. |
|
@ashishdhingra This has been rebased. Any updates on when it may be reviewed / released? |
ashishdhingra
force-pushed
the
dev
branch
2 times, most recently
from
fb3ee76
to
98302e8
Compare
| /// <summary> | ||
| /// The input object containing the current group configuration. It includes groupsToOverride, iamRolesToOverride, and preferredRole. | ||
| /// </summary> | ||
| [DataMember(Name = "groupConfiguration")] |
There was a problem hiding this comment.
These should be JsonPropertyName not DataMember
Default behaviour of the serializer is to use PascalCase, and Cognito uses camelCase, so these are essential
There was a problem hiding this comment.
JsonPropertyName is included for NetCoreApp3.1. I followed the patterns called out in examples such as
with included tests to verify serialization for all supported targets, and under all situations we're getting the correct camelCase serialization. Is this pattern for calling out DataMember and JsonPropertyName deprecated and something that should now be avoided in this library?
There was a problem hiding this comment.
I've used your code in .Net 6 and it fails. Adding JsonPropertyName fixes it.
There was a problem hiding this comment.
I've used the Visual Studio 2022 template which injects
[assembly: LambdaSerializer(typeof(Amazon.Lambda.Serialization.SystemTextJson.DefaultLambdaJsonSerializer))]
This serializer has a bug that puts all output into PascalCase, and uses JsonPropertyName to override this.
There was a problem hiding this comment.
I'll take another look at it. I suspect the majority of serialization classes within this repo would exhibit this issue, given this is an existing pattern.
There was a problem hiding this comment.
I think it's as simple as removing the conditional and always having the JsonPropertyName attribute present
|
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. |
|
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. |
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. |
|
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
<#1051 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAPMKC2VSF2PJ7JISQV3THLVKUVL5ANCNFSM5MAOO3ZA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
ashishdhingra
force-pushed
the
dev
branch
2 times, most recently
from
122e41b
to
d7a1c37
Compare
|
Api design with separate models per event type looks nice (on the picture :), but it requires creating separate Lambdas for each event. |
|
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. |
|
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? |
@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. |
|
@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. |
|
This PR has been released as part of version 2.1.0 of Amazon.Lambda.CognitoEvents. Thanks for the PR! |
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.