Skip to content

Implemented nested dict re-cast - #174

Merged
darrylabbate merged 1 commit into
aws-cloudformation:masterfrom
compose-x:fix/patternProperties-nested-objects
Mar 22, 2022
Merged

Implemented nested dict re-cast#174
darrylabbate merged 1 commit into
aws-cloudformation:masterfrom
compose-x:fix/patternProperties-nested-objects

Conversation

@JohnPreston

@JohnPreston JohnPreston commented Feb 11, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Related to #172

When trying to recast dict items, if the key is not found, it will iterate over the "nested" dict items first.
At first, I had

    except KeyError:
        child_cls = _field_to_type(cls.__dataclass_fields__[k].type, k, classes)
        for _child, _child_definition in v.items():
            recast_object(child_cls, _child_definition, classes)
            _new_child = child_cls._deserialize(_child_definition)
            json_data[k][_child] = _child_definition

which works fine. However, then the recast_test would not pass as the type for json_data[k][_child] would be that of cls instead of more "primitive" ones.
Drawback to that is that in the handlers code, every object is a dict instead of being of type cls. which means no access to the object attributes directly.

Welcome for suggestions on how to keep the typing in handlers when using model instead of having to deal in pure dict

Edit 1:
For the record, for the purpose of handlers, casting the nested value to the correct class, then use it and return to CFN works totally fine. In my CFN resource type I left the code as follows

    except KeyError:
        child_cls = _field_to_type(cls.__dataclass_fields__[k].type, k, classes)
        for _child, _child_definition in v.items():
            recast_object(child_cls, _child_definition, classes)
            _new_child = child_cls._deserialize(_child_definition)
            json_data[k][_child] = _new_child

And everything works totally fine.

@JohnPreston

JohnPreston commented Feb 11, 2022

Copy link
Copy Markdown
Contributor Author

I just realized, I never came across any CFN resource that would have patternProperties 🤔
Is that something then that might be best left unmanaged if not even denied ?

@ammokhov

ammokhov commented Feb 14, 2022

Copy link
Copy Markdown
Contributor

Thank you for your contribution. CFN open source team will take a look at the pull request and provide an update. There are some resources that use patternProperties - AWS::Batch::SchedulingPolicy, AWS::Synthetics::Canary, AWS::WAFv2::RuleGroup

@JohnPreston

JohnPreston commented Feb 14, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thank you for your contribution. CFN open source team will take a look at the pull request and provide an update. There are some resources that use patternProperties - AWS::Batch::SchedulingPolicy, AWS::Synthetics::Canary, AWS::WAFv2::RuleGroup

So 3 I didnt use so far! Bummer
Good to know though. Edit: good to see the WAF one is not a string but uses objects defined too.

Do let me know if this passes your tests and requirements :)
Thanks

@JohnPreston

JohnPreston commented Feb 25, 2022

Copy link
Copy Markdown
Contributor Author

Hey @ammokhov
What's the status on this ? Are you guys working on something holding up the merge ?
Thank you,

ammokhov
ammokhov approved these changes Feb 28, 2022
View reviewed changes

@ammokhov ammokhov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this looks good to me but still subject to integ testing

@JohnPreston

JohnPreston commented Mar 3, 2022

Copy link
Copy Markdown
Contributor Author

Hello @ammokhov
Any update on this ? And on the error I related to in #171 / AWS Case 9598785821
Thank you,

darrylabbate
darrylabbate approved these changes Mar 21, 2022
View reviewed changes

@darrylabbate darrylabbate left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I pulled these changes and did some manual integration testing. LGTM.

@darrylabbate

darrylabbate commented Mar 21, 2022

Copy link
Copy Markdown
Contributor

@JohnPreston

We'll have to add some kind of CLA integration for future PRs, but for now, can you simply respond to this issue with this text to confirm you give us the right to include your contribution to our code base?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

License for reference.

@JohnPreston

JohnPreston commented Mar 22, 2022

Copy link
Copy Markdown
Contributor Author

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@darrylabbate
darrylabbate merged commit 1fee342 into aws-cloudformation:master Mar 22, 2022
@JohnPreston
JohnPreston deleted the fix/patternProperties-nested-objects branch March 25, 2022 10:56
@JohnPreston JohnPreston mentioned this pull request Mar 25, 2022
Closed
kddejong pushed a commit to kddejong/cloudformation-cli-python-plugin that referenced this pull request Oct 13, 2022
@JohnPreston @kddejong
Implemented nested dict re-cast (aws-cloudformation#174)
ad9b3c0
kddejong pushed a commit to kddejong/cloudformation-cli-python-plugin that referenced this pull request Oct 13, 2022
@JohnPreston @kddejong
Implemented nested dict re-cast (aws-cloudformation#174)
1c4b203
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ammokhov ammokhov ammokhov approved these changes
@jaymccon jaymccon Awaiting requested review from jaymccon
+3 more reviewers
@darrylabbate darrylabbate darrylabbate approved these changes
@sungkkim sungkkim sungkkim approved these changes
@omkhegde omkhegde omkhegde approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@JohnPreston @ammokhov @darrylabbate @sungkkim @omkhegde