fix: Handle dot notations in the path_prop_name variables (#1537)

[wingkwong]

Issue #, if available:
#1537

Description of changes:
Support dot notations in the path_prop_name variables under samcli/commands/_utils/template.py

Checklist:

• Write Design Document (Do I need to write a design document?)
• Write unit tests
• Write/update functional tests
• Write/update integration tests
• make pr passes
• Write documentation

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

[sanathkr]

There are some changes in this codepath that could change your code. Can you rebase on latest develop?

[wingkwong]

@sanathkr Merged with the latest develop.

[jfuss]

@wingkwong I am not sure this PR actually solves the problem of the issue. If all of our code was went through this Template class, this would work but build does not (Template is used only by the package command from what I could trace and understand). The package command in the artifact_exporter.py file uses a library called (jmespath)[https://github.com/awslabs/aws-sam-cli/blob/develop/samcli/lib/package/artifact_exporter.py#L133] to handle the querying for nested attributes in json. Package handles this properly but build updates the template differently (due to when things where built) here. This method is not aware of the '.' notation and therefore won't update the template correctly.

Maybe I am missing something, where this PR is actually solving the issue. I think what we will want to do here is create another dummy glue resource here and update this test to assert the dot notation update is actually happening. Would you be able to add this tests? This will help us with ensure this doesn't break in the future and help us understand this is doing what we are expecting.

[wingkwong]

@jfuss Thanks for the clarification. I'll take a look later.

[wingkwong]

@jfuss I think my fix updates the path inside move_template -> _update_relative_paths instead of update_template . If you run sam build, ScriptLocation is able to be updated to ../../glue.py because the template is updated before moving it to the destination path. Or you prefer to move this logic back to build app_builder -> update_template and add the tests as you stated?

[wingkwong]

@jfuss on second thought, there is no functions_to_build in this case which means artifacts should be {} . update_template returns the original dict. Besides, based on the description of the method update_template in app_builder.py, I think we should update the path in _update_relative_paths instead.

[jfuss]

@wingkwong Good catch. I think _update_relative_paths is the right place. I thought the update_template did both for some reason.

[wnkz]

@jfuss @wingkwong How close are we to getting this merged?
I really need this to work and it's been almost two months now. I can help with reviewing, testing or even code if needed. Please let me know.

[wingkwong]

@wnkz I don't have further changes to make. I've been waiting for someone to review it.

[jfuss]

@wingkwong Apologize on the delay here. The missing piece is another test. I think this got lost in one of my comments above.

I think what we will want to do here is create another dummy glue resource here and update this test to assert the dot notation update is actually happening. Would you be able to add this tests? This will help us with ensure this doesn't break in the future and help us understand this is doing what we are expecting.

I know you have added a test within the test_template.yaml file but I think we need one (or an integ tests) that covers this case for build specifically.

[wingkwong]

@jfuss Sure. I'll take a look tonight. Thanks a lot.

[wingkwong]

@jfuss Added. Please take a look to see if it is what you want. Thanks.

[jfuss]

@wingkwong Thanks for adding the extra tests. While reviewing, I noticed that you were not using jmespath (allows searching json based on dot notation). In the package command, we already use jmespath to do something very similar and why we have the dot notation defined in the resources.py. I updated your implementation to mimic what we do in package already.

I also added an integ tests to validate that we update the nested property correctly. This is what I thought the test_app_builder unit tests was more or less doing but on deeper inspection, I noticed I was wrong. I went ahead and updated it to reduce back and forth. Really, appreciate the effort you put into this and keeping it updated over time!

[sriram-mv]

With addition of jmespath, the PR is much easier to follow. Approved.