Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update keys() to be able to iterate over a dict by updating the loop … #1492

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,60 @@ def get(
return decoded

def keys(self, carrier: dict) -> typing.List[str]:
return [_key.decode("utf8") for (_key, _value) in carrier]

"""
In most examples of propogators, they attempt to get a header key with .get() but when that fails they seem to
want to search all keys within carrier. There is not a prescribed structure for carrier in the specs
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/api-propagators.md
So we need to evaluate the carrier and peer into any child lists or dicts within carrier and return all of the keys
"""


#define the default empty list
returnable = []

#internal function that appends keys
def append_keys(key):
Copy link
Member

Choose a reason for hiding this comment

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

I think if you will move the append_keys function outside the keys function will be more clear and readable

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure I agree whole heartedly on that. There are many use cases for inner functions, in this case, as a helper function. going this route allowed it to have access to everything within the keys() scope. which was beneficial. And all this does is gives us reusablility within the keys() method. they do not serve a purpose outside of the scope of keys()

if isinstance(key, bytes):
returnable.append(key.decode("utf8"))
elif isinstance(key, str):
returnable.append(key)

#carrier is a dict, so iterate over .items()
for _key, _val in carrier.items():

#if we have another dict, lets make a recursive call
if isinstance(_val, Dict):
Copy link
Member

Choose a reason for hiding this comment

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

Consider move the code that append dict to new function
You use twice the same code exactly

Copy link
Author

Choose a reason for hiding this comment

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

done

#append our current key
append_keys(_key)

#append all keys within the dict
for x in self.keys(_val):
append_keys(x)

# if we have a list, lets iter over that. List can contain tuples(headers) dicts and string so lets approach them all as well
elif isinstance(_val, List):
for list_key in _val:

#Check for the Tuple
if isinstance(list_key, Tuple):
append_keys(list_key[0])
Copy link
Member

Choose a reason for hiding this comment

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

What does the tuple represent? Do we need the rest of the tuple?

Copy link
Author

Choose a reason for hiding this comment

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

you know i thought long and hard on this. So the only example of a Tuple ive seen come through is within the header, and the tuple is used to keep devs from modifying header values when they are received. So really every tuple was only 2 keys long and acted as a key value store. So i grab the first one as the key and let the 2nd value drop. for example. the headers come in looking like. Carrier{'headers':[('host','127.0.0.01'),('port','6020')]}. I decided not to spend time on checking if the tuple was longer than 2 because there wouldnt be a defined way for this method to determine what a key was and what a value was over. To be perfectly honest i feel like this is all a little heavy for what this method needs to do, but i dont see any prescribed structure. However I could be convinced that this needs to account and add all tuple values to list just to make sure we get all possible header values.

Copy link
Member

Choose a reason for hiding this comment

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

Ok sounds good to me, maybe just check if the tuple is not empty so the code will not throw an exception


#check for the dict
elif isinstance(list_key, Dict):
append_keys(_key)

#append all keys within the dict
for x in self.keys(_val):
append_keys(x)
else:
append_keys(list_key)

#finally, if our key was just a string, append that
else:
append_keys(_key)

return returnable


asgi_getter = ASGIGetter()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,28 @@ def test_keys(self):
getter = ASGIGetter()
keys = getter.keys({})
self.assertEqual(keys, [])

def test_populated_keys(self):
getter = ASGIGetter()
header = {
"type": "http.response.start",
"status": 200,
"headers": [
(b"Content-Type", b"text/plain"),
(b"custom-test-header-1", b"test-header-value-1"),
(b"custom-test-header-2", b"test-header-value-2"),
(
b"my-custom-regex-header-1",
b"my-custom-regex-value-1,my-custom-regex-value-2",
),
(
b"My-Custom-Regex-Header-2",
b"my-custom-regex-value-3,my-custom-regex-value-4",
),
(b"my-secret-header", b"my-secret-value"),
],
}

expected_val = ['type','status','Content-Type', 'custom-test-header-1', 'custom-test-header-2', 'my-custom-regex-header-1', 'My-Custom-Regex-Header-2', 'my-secret-header']
keys = getter.keys(header)
self.assertEqual(keys, expected_val)