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

Fixed an issue where delimited string and primitive values in payload were not parsed properly by Lambda Test Tool. #1851

Conversation

ashishdhingra
Copy link
Contributor

@ashishdhingra ashishdhingra commented Oct 28, 2024

Issue #, if available: #1135

Description of changes:
Fixed an issue where delimited JSON string payload was not parsed properly by Lambda Test Tool. This PR ports the string handling from .NET Lambda CLI tooling. .NET CLI Extensions tooling per logic here delimits the raw string payload with " if it doesn't start with {. JSON strings are enclosed in double quotes:

if(!invokeRequest.Payload.StartsWith("{"))
{
    invokeRequest.Payload = "\"" + invokeRequest.Payload + "\"";
}

Per testing with dotnet lambda invoke-function:

  • For dotnet lambda-test-tool-6.0 --no-ui --payload ""hello"", payload should be parsed as hello.
  • For dotnet lambda-test-tool-6.0 --no-ui --payload '"hello"', payload should be parsed as 'hello'.

EDIT: Per discussion with @normj, the implementation in .NET Extensions for CLI would not handle primitive types and needs to be fixed (refer #1197). I have implemented changes as suggested in this PR. Would work on .NET Extensions CLI tooling as part of issue #1197.


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

@ashishdhingra ashishdhingra requested a review from normj October 28, 2024 18:46
@ashishdhingra ashishdhingra force-pushed the user/ashdhin/LambdaTestTool-StringParameter-Issue1135 branch 2 times, most recently from 2f53875 to 10af223 Compare October 29, 2024 16:39
@peterrsongg peterrsongg requested a review from philasmar October 29, 2024 17:39
@@ -318,6 +318,12 @@ private static string DeterminePayload(LocalLambdaOptions localLambdaOptions, Co
}
}

// Enclose simple string payload in double quotes so that it's parsed properly by Lambda JSON serializer.
if (!payload.StartsWith("{") && !payload.StartsWith("\""))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried the logic we added in Amazon.Lambda.Tools isn't great logic to duplicate. The problem with the Amazon.Lambda.Tools logic is what if you want to send a bool or an int. This logic automatically converts true to "true" or 100 to "100".

I wonder instead if we should attempt to parse the JSON using with System.Text.Json. If that parses with no problems then send as is. If it doesn't parse add surrounding quotes if it doesn't already start with quotes then use that value. We could retrofit Amazon.Lambda.Tools with the same logic. I wouldn't view that as a breaking change there and we would fix some scenarios that aren't working correct in Amazon.Lambda.Tools.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented fix. Please review.

@ashishdhingra ashishdhingra force-pushed the user/ashdhin/LambdaTestTool-StringParameter-Issue1135 branch from 10af223 to 2e8b04f Compare October 30, 2024 18:21
@ashishdhingra ashishdhingra requested a review from normj October 30, 2024 18:22
@ashishdhingra ashishdhingra force-pushed the user/ashdhin/LambdaTestTool-StringParameter-Issue1135 branch from 2e8b04f to 69d7f0f Compare October 30, 2024 18:23
@ashishdhingra ashishdhingra force-pushed the user/ashdhin/LambdaTestTool-StringParameter-Issue1135 branch from 69d7f0f to 04c3722 Compare October 30, 2024 19:03
@ashishdhingra ashishdhingra changed the title Fixed an issue where delimited JSON string payload was not parsed properly by Lambda Test Tool. Fixed an issue where delimited string and primitive values in payload were not parsed properly by Lambda Test Tool. Oct 30, 2024
@ashishdhingra ashishdhingra force-pushed the user/ashdhin/LambdaTestTool-StringParameter-Issue1135 branch from 04c3722 to bfaba21 Compare October 30, 2024 20:41
… were not parsed properly by Lambda Test Tool.
@ashishdhingra ashishdhingra force-pushed the user/ashdhin/LambdaTestTool-StringParameter-Issue1135 branch from bfaba21 to bef150b Compare November 1, 2024 18:05
@aws-sdk-dotnet-automation aws-sdk-dotnet-automation merged commit d51554c into dev Nov 1, 2024
4 checks passed
@aws-sdk-dotnet-automation aws-sdk-dotnet-automation deleted the user/ashdhin/LambdaTestTool-StringParameter-Issue1135 branch November 1, 2024 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants