search

tableau / altimeter

Graph AWS resources in Neptune
MIT License
82 stars 25 forks source link

TagsField is case-sensitive #130

Open lllama opened 3 years ago

lllama commented 3 years ago

API Gateway domain names (and other API Gateway resources tbh) have their keys set to all lowercase letters. (an example can be seen in the response here: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/apigateway.html#APIGateway.Client.get_domain_names)

Because TagsField defaults to optional=True, the lowercase key will be missed and the tags will be empty.

jbmchuck commented 3 years ago

Thanks for the report! I'm hesitant to modify the existing TagField class but I'd definitely be open to adding a similar Field class which also produces a LinkCollection of TagLinks, in this case something like LowerCaseTagDictField which would expect a key tags containing a dict (rather than list of dict with Key, Value keys). Do you see any issues with that?

lllama commented 3 years ago

That seems fine. Explicit being better than implicit and all that. My only thought is whether AWS is moving to the lowercase example for future APIs. In which case a TagsV2Field might make more sense. But that would just be a guess as to what AWS are planning.

Happy with the proposed solution.

lllama commented 3 years ago

A further thought after sleeping in it: at the moment, if the lowercase tags key is present, then the existing TagsField will silently fail to include the tags. It feels like this could be tricky to debug. Maybe check for the lowercase and print a warning?

jbmchuck commented 3 years ago

Sounds good. Due to other obligations and vacation it's fairly unlikely I'll be able to work on this until August 9th at the earliest but feel free to submit a PR if you end up implementing it.