Make python name to JSON name attribute mapping via attribute_map optional.

[milancermak]

I'm submitting a...

[ ] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[ ] Performance issue
[x] Feature request
[ ] Documentation issue or request
[ ] Other... Please describe:

Expected Behavior

When serializing, use the python attribute name if not present in attribute_map.

Current Behavior

If a name of a python attribute is the same as its name in the serialized object, it still needs to be present in the attribute_map.

Possible Solution

// Not required, but suggest a fix/reason for the bug,
// or ideas how to implement the addition or change

I guess it's a one-line change in the Serializer? Sorry, didn't dig into it too much.

Steps to Reproduce (for bugs)

// Provide a self-contained, concise snippet of code
// For more complex issues provide a repo with the smallest sample that reproduces the bug
// Including business logic or unrelated code makes diagnosis more difficult

@attr.s
class Skill:
    launch_count = attr.ib()

    deserialized_type = {'launch_count': 'int'}
    attribute_map = {'launch_count': 'launch_count'} # needless
    # snip

Context

I'm using the DefaultSerializer to serialize my models for DynamoDB. It's really cool. I want to suggest of using the python's attribute name as the name of the value in the serialized object if it is not present in the attribute_map mapping. In the example posted above, the attribute_map could be removed altogether, a pattern in a lot of my models. This direct mapping is also quite common in the SDK models, for example here in SimpleCard

Your Environment

• ASK SDK for Python used: 1.5.0
• Operating System and version: OS X

Python version info

• Python version used for development: 3.7

[nikhilym]

Hi @milancermak . As you mentioned above, we use this attribute_map to map the attribute name to the JSON key name. This is particularly useful during serialization of the Response object, during skill invocation and return.

We had to do this because the JSON keys cannot be directly specified as attribute names since they do not follow PEP8 standards. If we remove this, then using some magic method, we have to convert the attributes back to the specific key names. This will lead to performance issues when the skill has to return a Response JSON which itself might have hierarchical JSONs.

Also, in cases when the skill developer attaches any additional attributes on any of the Response or internal objects, the skill invocation still runs fine but the Alexa service might fail with the invalid Response not being able to process. However, I understand that if you want to use the DefaultSerializer on your own code, there is some duplicated code to work with.

Do you think we can do something under DefaultSerializer's serialize method that can still work with Response serialization for the above cases but works for custom classes as well?

[milancermak]

Yes, I do. Here's a diff of the changes that could be done to the DefaultSerializer that would keep the current functionality but also support my case.

103a104,108
>             class_attribute_map = getattr(obj, 'attribute_map', {})
>             class_attribute_map.update({k: k for k
>                                         in obj.deserialized_types.keys()
>                                         if k not in class_attribute_map})
>
105c110
<                 obj.attribute_map[attr]: getattr(obj, attr)
---
>                 class_attribute_map[attr]: getattr(obj, attr)
282,283c287
<             if hasattr(obj_type, 'deserialized_types') and hasattr(
<                     obj_type, 'attribute_map'):
---
>             if hasattr(obj_type, 'deserialized_types'):
289c293,296
<                 class_attribute_map = obj_type.attribute_map
---
>                 class_attribute_map = getattr(obj_type, 'attribute_map', {})
>                 class_attribute_map.update({k: k for k
>                                             in obj_type.deserialized_types.keys()
>                                             if k not in class_attribute_map})

I can send a PR if it helps.

[nikhilym]

Thanks for the diff @milancermak . We will look into it and let you know. You can also contribute to the changes by submitting a PR.

[nikhilym]

Hey @milancermak , do you have a PR that we can look at?

[milancermak]

There it is 👍

[nikhilym]

Closing this since the PR is merged. Will be updating the PR #60 once the changes are released to PyPI. Thanks @milancermak for getting this to our attention and raising the PR.