convert all members of a group to string #102
Conversation
|
Is this to fix issue #101 you just filed? |
|
Yes. The members list of a group can have many users, with the original implementation only the first member is mapped. With this fix I check the length of a list and convert each member to string and replace it accordingly. Maybe it is better to specify the key If you have a better idea, pleas modify it. |
|
may be: would be better than: |
|
like this: |
|
I think this is easier and will make it as well: |
|
I don't know what is the pythonic way to check it correctly. |
|
It would make sense to compare the returned members list with the given one in |
nss_cache/sources/ldapsource.py
Outdated
| if isinstance(record[1][key][0], bytes) and key != 'objectSid': | ||
| value = record[1][key][0].decode('utf-8') | ||
| record[1][key] = [value] | ||
| if len(record[1][key]) > 1: |
There was a problem hiding this comment.
Avoid the if statement here, by using a temporary list we can append to, then finally assign record[1][key] = tmp_list.
There was a problem hiding this comment.
this is possible as well.
nss_cache/sources/ldapsource.py
Outdated
| value = record[1][key][0].decode('utf-8') | ||
| record[1][key] = [value] | ||
| if len(record[1][key]) > 1: | ||
| for i, item in enumerate(record[1][key]): |
There was a problem hiding this comment.
Then i think we can avoid the enumerate() line by juster iterating over record[1][key],
There was a problem hiding this comment.
this will fail the runtests.py as far as I remember.
There was a problem hiding this comment.
if record[1][key] is a string this will not work, right?
There was a problem hiding this comment.
we need to consider, that there one element lists, where the iteration will be over the string characters instead of the list elements. So applying the following:
for item in record[1][key]:
Won't work on [b'Test User']:
will iterate over the string characters
There was a problem hiding this comment.
may be using range is a better way?
nss_cache/sources/ldapsource.py
Outdated
| for i, item in enumerate(record[1][key]): | ||
| if isinstance(item, bytes) and key != 'objectSid': | ||
| value = item.decode('utf-8') | ||
| record[1][key][i] = value |
There was a problem hiding this comment.
this line becomes tmp_list.append(value)
There was a problem hiding this comment.
of course on way or another.
nss_cache/sources/ldapsource.py
Outdated
| else: | ||
| if isinstance(record[1][key][0], bytes) and key != 'objectSid': | ||
| value = record[1][key][0].decode('utf-8') | ||
| record[1][key] = [value] |
There was a problem hiding this comment.
and all of this doesn't need to exist, and instead finally set record[1][key] = tmp_list.
There was a problem hiding this comment.
IMHO I think the only way to make it work for the tests as well, of course it will work with a tmp_list:
for record in data:
# If the dn is requested, return it along with the payload,
# otherwise ignore it.
for key in record[1]:
for i, item in enumerate(record[1][key]):
if isinstance(item, bytes) and key != 'objectSid':
value = item.decode('utf-8')
record[1][key][i] = value
if self._dn_requested:
merged_records = {'dn': record[0]}
merged_records.update(record[1])
yield merged_records
else:
yield record[1]
There was a problem hiding this comment.
This is a possible solution as well:
for key in record[1]:
for i in range(len(record[1][key])):
if isinstance(record[1][key][i], bytes) and key != 'objectSid':
value = record[1][key][i].decode('utf-8')
record[1][key][i] = value
|
Why is key = 'member' worth special casing? |
Nothing special, it is just an example for the members list, which can be |
|
If your suggestions work, I'll apply it, until now I couldn't make it work. |
This is the critical part:
member list is > 1, so it converts the first member only, we need to check the length of the list, as for members it is not always 1, as for the other attributes. My solution would be:
I'll create a pull request, please modify as needed.