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

Python: Unable to loop if key.value() is Null #778

Closed
lanlooker opened this issue Aug 1, 2021 · 6 comments · Fixed by #791
Closed

Python: Unable to loop if key.value() is Null #778

lanlooker opened this issue Aug 1, 2021 · 6 comments · Fixed by #791

Comments

@lanlooker
Copy link
Contributor

lanlooker commented Aug 1, 2021

I am working on a use case to remove the "All Users" group from a folder, and I am using all_content_metadata_accesses.

In Looker, if a folder doesn't have any groups as its editor or viewer, then group_id is null (json), so I expect to loop and write some conditions with group_id, similar to this:

testing_loop = [{'a':1, 'group_id': None }, {'a':1, 'group_id':1}]  
for i in testing_loop:
  if i['group_id'] is not None:
    print('group_id is not Null')
  else: 
    print('group_id is Null')

However, I am unable to loop or do anything with the key-value pair 'group_id' if the value is None because we have an exception handling a key with a None value:

accesses = sdk.all_content_metadata_accesses(content_metadata_id=767)
test_dict = accesses[0] # Purposely choose accesses[0] because I know group_id here is Null 
print(type(test_dict))
print(test_dict['id']) # OK
print(test_dict['group_id']) # KeyError: 'group_id'
__getitem__(self, key)
     63             ret = ret.value
     64         if ret is None:
---> 65             raise KeyError(key)
     66         return ret
     67 

KeyError: 'group_id'

I'll post a reproduction case in our internal tool.

@lanlooker
Copy link
Contributor Author

lanlooker commented Aug 2, 2021

Update: I am hitting the same issue while working on another use case with authentication.

lan = sdk.user(user_id=434)
print(lan['credentials_ldap']) # Key Error: credentials_ldap

user_id=434 has values for the two keys credentials_email, and credentials_google; other credentials keys are null (i.e.: credentials_saml, credentials_ldap). This is expected because in Looker, one regular user can only have one additional auth method besides the email/password.

I think I can try to get around by using the requests and json module ... but ideally I would like to use the Python SDK. Maybe we can "loosen" up by not raising the KeyError if the value is None?

@lanlooker
Copy link
Contributor Author

@joeldodge79 @josephaxisa any chance you could take a look at this please? 🙏🏻

I can workaround by making a query using system activity to get rid of all null values and then work with the json body from sdk.run_look() but I am trying to make a sample demo doing a pure "pythonic" approach (without using a pre-built query from system activity).

@josephaxisa
Copy link
Contributor

@lanlooker sorry for the delay on this, I missed the notification. The behavior you're experiencing is not unique to the SDK unfortunately; accessing a dictionary item that does not exist throws a KeyError in Python. However, I can think of a couple of ways to get around this. Consider the below dictionary:

x = { 'foo': 'foo_val' }
  1. Use the in operator to check if the key exists before accessing it
if 'bar' in x:
  print(x['bar'])
  1. Use dict.get(key[, default]) (docs)
print(x.get('foo')) # foo_val
print(x.get('bar', None)) # None
print(x.get('bar', 'N/A')) # N/A
print(x.get('bar')) # None

Hope this helps!

@joeldodge79
Copy link
Contributor

TLDR; I believe the if ret is None: raise KeyError(key) logic is wrong and I'll see about removing it.

also sorry I missed the notification

this is more complicated. This "dictionary" access is a facade in front of the actual model/instance interface the python sdk presents.

When we wrote the initial [de]serialization for the model/instance interface we hit a snag where we if someone wrote this and sent it:

user = User(first_name="Jill")

we would send null for all unspecified json properties:

{"first_name": "Jill", "last_name": null, ...}

In order to work around this we decided to remove all None properties from a model before serializing it so the resulting json would only have what you specified. In the rare case that you meant to send a json null, then you'd do:

user = User(first_name="Jill", last_name=EXPLICIT_NULL) # defined in models.py

But that's all about creating input to be sent to the API. Here we're really talking about a response object/dict (though keeping in mind that often the response obj/dict is re-used as input). I think we should NOT be raising a KeyError if the field is None.

This raises another question that is applicable to both the model/instance and dictionary interfaces: for a sparse response (e.g. fields parameter was used on the api call) how can we distinguish between a field missing from the json payload versus one that is present and the value is null?

We currently don't distinguish when we populate a response model/instance and, because the dictionary implementation is just a proxy to the model/instance, then we should expose what it would do. It's gonna look kind of odd though:

user = User()
user.first_name  # None
user['first_name']  # None

@josephaxisa
Copy link
Contributor

Seems like I completely misinterpreted where the error was being thrown from. Thanks for clarifying @joeldodge79

@lanlooker
Copy link
Contributor Author

lanlooker commented Aug 10, 2021

Thank you both 🙏🏻

@joeldodge79 thank you! This is the exact issue I am having! I usually get a response from GET/ as a dict, hoping to transform this dict and use as a body of a different call (usually POST/), so not being able to access/transform a key-value pair if the value is null is a blocker. I hope that we remove the KeyError here.

Also, thank you for the great write up on the context of the KeyError 🙏🏻 I've learned a great deal

joeldodge79 added a commit that referenced this issue Aug 10, 2021
dictionary key access should reflect model property access
as closely as possible. In this case, do NOT raise a KeyError
for a model property that exists. https://git.io/JRrKm

fixes #778
joeldodge79 added a commit that referenced this issue Aug 11, 2021
dictionary key access should reflect model property access
as closely as possible. In this case, do NOT raise a KeyError
for a model property that exists. https://git.io/JRrKm

fixes #778
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants