Skip to content

Conversation

@elbayaaa
Copy link
Contributor

…tionalry

Issue #, if available:

Description of changes:

Description of how you validated changes:

Checklist:

  • [√] Write/update tests
  • [√] make pr passes
  • Update documentation
  • Verify transformed template deploys and application functions as expected

Examples?

Please reach out in the comments, if you want to add an example. Examples will be
added to sam init through https://github.com/awslabs/aws-sam-cli-app-templates/

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

event_properties = event.get("Properties", {})

if event_properties and not isinstance(event_properties, dict):
raise InvalidEventException(logicalId, "Event Properties is invalid.")
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add a hint of what type it needs to be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the part I'm struggling with, what a clear message could be. I don't want to use the python-specific term "dict/dictionary". Do you have a good suggestion?

@codecov-io
Copy link

codecov-io commented Feb 24, 2021

Codecov Report

Merging #1938 (ca59c89) into develop (aaa1d15) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1938      +/-   ##
===========================================
+ Coverage    93.87%   93.92%   +0.05%     
===========================================
  Files           89       89              
  Lines         5942     5946       +4     
  Branches      1210     1212       +2     
===========================================
+ Hits          5578     5585       +7     
+ Misses         168      166       -2     
+ Partials       196      195       -1     
Impacted Files Coverage Δ
...translator/plugins/api/implicit_http_api_plugin.py 97.50% <100.00%> (+0.06%) ⬆️
...translator/plugins/api/implicit_rest_api_plugin.py 100.00% <100.00%> (ø)
samtranslator/translator/logical_id_generator.py 100.00% <0.00%> (+9.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aaa1d15...ca59c89. Read the comment docs.

@elbayaaa elbayaaa requested a review from sriram-mv February 24, 2021 05:09
Copy link
Contributor

@hawflau hawflau left a comment

Choose a reason for hiding this comment

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

nit: Can Properties for other event sources be a string at all? I think it should be a dict object for all event sources. Should we extend this check for all event sources?

@elbayaaa elbayaaa merged commit a07fbec into aws:develop Feb 24, 2021
@elbayaaa
Copy link
Contributor Author

nit: Can Properties for other event sources be a string at all? I think it should be a dict object for all event sources. Should we extend this check for all event sources?

We have a on_before_transform_template hook that scans lambda functions and step function for API events to merge them to the one serverless-api resource the template has/should have. during this hook we didn't handle the case if the Properties is not a dict. This PR is to solve this issue.

During the Lambda function resource transform i.e. after the "on_before_transform_template" hook, we process all the event-sources of a given function(API, Schedule, EventBridgeRule ...etc) and during this we handle such an issue(Properties not a dict) by a simple try..except. see here

mgrandis pushed a commit to mgrandis/serverless-application-model that referenced this pull request Mar 2, 2021
aws#1938)

* Raise an InvalidEventException if api-event's properties is not a dictionalry

* Make a cleaner error message

* typo
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.

5 participants