TypeError : section names must be strings

[Saumya-Mishra9129]

Steps to produce - open a fructose activity such as write on VM , click on neighbourhood icon , join on other VM , then stop activities on both VMs.

[quozl]

I don't understand this change; why is the section no longer added to config?

[Saumya-Mishra9129]

The reason that cause the change is first the error mentioned in commit message and second I tried to test it with different fructose activities , I first tried with write then with chat and with same friend. I found duplicate section error with chat , that means if a friend is already added in a section , no means to add it again as ConfigParser works as a dictionary , it can't contain duplicate keys. Second this I don't know why it's not working see if you could help with that is whenever a user exists with activity along with his friend , its section should be deleted. Seems like there is no option to delete or it's not working.

On Tue 2 Jun, 2020, 7:04 AM James Cameron, notifications@github.com wrote:

I don't understand this change; why is the section no longer added to config?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sugarlabs/sugar/pull/921#issuecomment-637215256, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKZH63JVIKKQGTFQ7UQPOXDRURJKNANCNFSM4NQAXQRQ .

[Saumya-Mishra9129]

If section is already present , no need to set it again according to me.

On Tue 2 Jun, 2020, 10:28 AM Srevin Saju, notifications@github.com wrote:

@srevinsaju commented on this pull request.

In src/jarabe/model/friends.py https://github.com/sugarlabs/sugar/pull/921#discussion_r433620363:

@@ -156,9 +156,10 @@ def save(self): cp = ConfigParser()

     for friend in self:

• section = friend.get_key()
• cp.add_section(section)
• cp.set(section, 'nick', friend.get_nick())
• section = str(friend.get_key())
• if section not in cp.sections():
• cp.add_section(section)
• cp.set(section, 'nick', friend.get_nick())

Should L#162 be outside the if conditional code block?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sugarlabs/sugar/pull/921#pullrequestreview-422342426, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKZH63LXZG5HLCKMF4VDMKLRUSBJFANCNFSM4NQAXQRQ .

[srevinsaju]

@Saumya-Mishra9129 I guess the section is added in L161; The section should be updated with the new information right?

[quozl]

The reason that cause the change is first the error mentioned in commit message and second I tried to test it with different fructose activities , I first tried with write then with chat and with same friend. I found duplicate section error with chat , that means if a friend is already added in a section , no means to add it again as ConfigParser works as a dictionary , it can't contain duplicate keys. Second this I don't know why it's not working see if you could help with that is whenever a user exists with activity along with his friend , its section should be deleted. Seems like there is no option to delete or it's not working.

I had asked why the section is no longer added. My question was wrong. You should have pointed that out. Only now do I see that the GitHub inline comment stream hid the remainder of the patch. The section is not no longer added.

But I still don't understand, and the commit message hasn't changed; I don't see the traceback, so I can't validate the commit does what it says.

The nick should be set again, as it may have changed without the key changing.

Also, section names must be strings has nothing to do with duplicate sections, so I think you are perhaps conflating the two problems; please use separate commits for separate problems.

[Saumya-Mishra9129]

But I still don't understand, and the commit message hasn't changed; I don't see the traceback, so I can't validate the commit does what it says.

The traceback I saw while testing on Ubuntu 20.04.

[Saumya-Mishra9129]

@Saumya-Mishra9129 I guess the section is added in L161; The section should be updated with the new information right?

Yeah that is what needed.

[quozl]

Thanks for the update. This is a critical repository, so my standards of review are much higher.

Still there's not enough detail in the commit message. I want to see the summary line say what you fix, but not expressed as an error message unless that error message is the only evidence of a harmless problem. I want to see in the commit message the complete traceback message, from the first line which says Traceback to the error. Please read making commits and follow the advice in each point where possible, and ask if you need me to clarify.

I'd also like to know why the key property is not a string already. How is it that it has become a bytes? What set the property? Are there any other uses of the property in other source code in the toolkit and activities that presume it is a string? I've done a quick search and I've found some uses of the property that seem to suggest it should be a string.

Because of this, I'm not sure if what you propose is the right solution.

[srevinsaju]

The current solution might be a workaround to solve the problem The source of the bytes type of object should be traced to where it might have begun, mostly it is a python2 to 3 regression which was not solved. Possibly starting a clean debian OS, and building from source should confirm the error then, It is possible that an older regression got fixed on git, or an older configuration file lies in .sugar and therefore misleading. Please check on the latest source, on clean debian / ubuntu without sugar installed, so the results are confirmed.

This a possible test case,

[Saumya-Mishra9129]

I logged about get_key return value it returns <class bytes> , review in commit 9ddc276

[quozl]

Why is it not str? Where did the value come from?

[Saumya-Mishra9129]

Why is it not str? Where did the value come from?

I finally find out the place where key has set to byte https://github.com/sugarlabs/sugar/commit/aa18879e9717dfe2d30f249549e9a43d6dd6da4f#diff-644e16988bb8fd551b1f0c2f32a6b2a3R164.

@srevinsaju as James is not available .. can you see if I am correct or not.?

[srevinsaju]

@Saumya-Mishra9129 it looks like the source of bytes is correct as you have pointed out.

EDIT: I have checked a recursive grep on the sugar repository; There are only four instances of encode, and this is one of them. Can you please confirm the error is fixed if you remove the current commit https://github.com/sugarlabs/sugar/pull/921/commits/9ddc27669c3bc19f37c0a0a6e381eb918d4f1b2d and add another commit, removing the .encode() ?

[Saumya-Mishra9129]

@Saumya-Mishra9129 it looks like the source of bytes is correct as you have pointed out.

EDIT: I have checked a recursive grep on the sugar repository; There are only four instances of encode, and this is one of them. Can you please confirm the error is fixed if you remove the current commit 9ddc276 and add another commit, removing the .encode() ?

I checked it didn't work , got stuck in another error. But I am sure the key value is in bytes here. This is possibly the a regression, but I am still not able to find the right cause. :( Edit : @quozl mentioned this also in https://github.com/sugarlabs/sugar-toolkit-gtk3/issues/433

[srevinsaju]

The use of bytes might be also because of the use of sockets. Most sockets require only data as bytes. This might be another reason. I do not have extensive knowledge on this. Maybe @shaansubbaiah might know. He had recently finished a great PR https://github.com/sugarlabs/sugar/pull/911, amazing work there. May be he can help

[quozl]

Just to remind you, that https://github.com/sugarlabs/sugar-toolkit-gtk3/issues/433 is where the object is quoted bytes, not bytes. I suggest black-box testing D-Bus and checking if it is data clean.

[shaansubbaiah]

Sorry for the late reply, what is the error seen after removing the encode() as @srevinsaju suggested?

[Saumya-Mishra9129]

I finally find out the place where key has set to byte aa18879#diff-644e16988bb8fd551b1f0c2f32a6b2a3R164.

Thanks @shaansubbaiah , The error was straight forward in https://github.com/sugarlabs/sugar/blob/aa18879e9717dfe2d30f249549e9a43d6dd6da4f/src/jarabe/model/buddy.py#L165 this line. as we have a need to encode key need while passing to dbus.ByteArray as it takes only while initialization. I tried decoding it again after the line , that doesn't work also.

[quozl]

I've reviewed the discussion so far.

• it doesn't matter what an existing ConfigParser instance holds, because the save function creates a new one each time, so we can ignore that part of the conversation,
• the Buddy key is a dbus.ByteArray, which is a subclass of Python 3 bytes,
  • in theory it should never be any other type,
  • so there is no need to test for the type, (why did you add code to test for type?),
• in Python 2 the dbus.ByteArray is effectively a Python 2 str,
  • using a dbus.ByteArray as a section name works fine (did you test? you didn't say),
• the commit message does not follow our guide for writing commits, please follow it.
• you must also handle the reverse translation in the load function now.

[Saumya-Mishra9129]

@quozl @srevinsaju Please review bfae395 and 8a065b3. 8a065b3 solves the error mentioned in mailing list by @shaansubbaiah

Removing the same user as a friend produces: Traceback (most recent call last): File "/usr/lib/python3/dist-packages/jarabe/desktop/groupbox.py", line 67, in _friend_removed_cb icon = self._friends[key] KeyError: "b'AAAAB3NzaC1kc3MAAACBAP1g0TB6F/q8LSrX2APaHBJm89b+IocnRJVovwu4pEMVlLmg5fScoXUZ1qi8l04hQOxetyWmc4wYiFMS/2MXmb7PvWT0Mlmx1y9A3Am3y0XP5870uYNgXJ4UgL9lGX6uz3ExsqcG9X5X5wbwIJ1ckQxXnxf2VxwUxBd2p4B/tPG1AAAAFQCN1CuSl6JjCOm4RS+nCuI3mIlalwAAAIEA6YEg849ugww7gpmT7aUHl3qetdtl+/fkL8BxyYnOeaR2Mcs3phrYQNvm2/ac0HA16TqJOoVatoPpD/Z84IBQxU2wNQilhU9VEwiP/+Wrukg3LbU/oyEPnOOJfasR0lgzLL+RR20zLtixer3irlsv2wvZU/9PFbZvuCEzbB3LiC0AAACBAOWzSUShSPWHQGKALTyWTvl481IIxPwizbKmraEeKm3xvqB8dP5Hy2QQlRXZrb1QgMDoARRYxzUoO8/PKzLSpFYoQvF9v31DtJGkNilNdfRuiJTWZJah3DBYMAiPHU478DH3zcHbRYI4Prkmu00v+smv7qSUhDQrClMF+ka6mJN4'"

Can you test and confirm that it doesn't happen with above changes.? @JuiP @chimosky Please Review.

[chimosky]

Reviewed 8a065b3, haven't tested yet but I don't think it'll fix the issue. Have you tested?

Reason why I don't think it'll fix the issue is because in the commit, the only functional change is the change from [str] to [object].

[Saumya-Mishra9129]

Reviewed 8a065b3, haven't tested yet but I don't think it'll fix the issue. Have you tested?

Reason why I don't think it'll fix the issue is because in the commit, the only functional change is the change from [str] to [object].

I have tested it. It fixes the problem for me. The key was a quoted bytes because we were taking it as str. It should be treated as object as it is treated in add-friend. Can you test it once?

[quozl]

Reviewed 8a065b3. This changes the API. I don't know of anything using the API apart from Sugar, but I'm sensitive to changes to API because of the later damage that can happen. Specific change is friend-removed signal is now given an object (something with a get_key method which is called) instead of a str (with the key already provided). I don't see why this change is made. The commit message doesn't explain. While it may well remove the KeyError, it doesn't seem likely to be correct as a solution. Why isn't str changed to bytes instead?

[Saumya-Mishra9129]

Reviewed 8a065b3. This changes the API. I don't know of anything using the API apart from Sugar, but I'm sensitive to changes to API because of the later damage that can happen. Specific change is friend-removed signal is now given an object (something with a get_key method which is called) instead of a str (with the key already provided). I don't see why this change is made. The commit message doesn't explain. While it may well remove the KeyError, it doesn't seem likely to be correct as a solution. Why isn't str changed to bytes instead?

I agree, changes to APIs can be critical, and should have done after proper testing. The major reason why I go with this change is that key is seen as quoted bytes , which means a str object. in that error message. It could be because we had used str . The add-friend also uses an object there , so why not we use same object in both instead of str , and however if we allow this change , the next person who will use API somewhere will proceed accordingly in future.

This error makes (F2) Group View a lot messy, because it keeps on adding friends objects there without any removal.

[quozl]

I don't feel my questions have been answered. I'll try again. 8a065b3 is three changes at once;

• the API change; from type str to type object for the signal argument,
• deferring the call to get_key to the time the signal is delivered,
• making two calls to get_key instead of one at the time the signal is delivered.

Which of these changes is responsible for fixing the problem, and why? None of these changes affect how data is converted, so the problem of quoted bytes should be unchanged. Is it really that the get_key method is returning different type depending on when it is called?

[Saumya-Mishra9129]

Which of these changes is responsible for fixing the problem, and why?

All of them are necessary, as changing str to object causes all these change. get_key returns abyte object not a quoted byte, quoted byte is a str. For demonstration I tested this code-

from gi.repository import GObject

class MyObject(GObject.GObject):
    __gsignals__ = {
        'my_delete': (GObject.SignalFlags.RUN_FIRST, None,
                      (str,)),
        'my_add': (GObject.SignalFlags.RUN_FIRST,None,
                      (object,)),
    }

    def __init__(self):
        GObject.GObject.__init__(self)
        self.ls = {}

    def do_my_add(self, arg):
        self.ls[arg] = 22
        print("method handler for `my_add' called with argument", type(arg))

    def do_my_delete(self, arg):
        del self.ls[arg]
        print(self.ls)
        print("method handler for `my_delete' called with argument", type(arg))

my_object = MyObject()
my_object.emit("my_add",b'abcccc')
my_object.emit("my_delete",b'abcccc')

The output is -

method handler for `my_add' called with argument <class 'bytes'>
Traceback (most recent call last):
  File "test.py", line 21, in do_my_delete
    del self.ls[arg]
KeyError: "b'abcccc'"

It clears that even if we add bytes object in list, when delete signal is called , it automatically converts bytes to str . Hence key error is raised because of quoted bytes.

[quozl]

Then why not pass a str instead of bytes in the call to emit in remove? The data type defined is str, so passing bytes is wrong. The change would be much smaller.

[Saumya-Mishra9129]

Then why not pass a str instead of bytes in the call to emit in remove? The data type defined is str, so passing bytes is wrong. The change would be much smaller.

Tried it, another way. It also fixes the problem , @quozl @chimosky kindly review.

[quozl]

Thanks. Reviewed. The root cause of both problems is our failure to Port to Python 3 with respect to the D-Bus interface being used to obtain the connection interface buddy info from Telepathy. Instead of these two patches, I recommend 95b1c32d8 ("Coerce BaseBuddyModel.props.key to str"). This patch reverts and reimplements a tiny part of https://github.com/sugarlabs/sugar/commit/aa18879e9717dfe2d30f249549e9a43d6dd6da4f ("Port to Python 3"). Please review.

[quozl]

Then why not pass a str instead of bytes in the call to emit in remove? The data type defined is str, so passing bytes is wrong. The change would be much smaller.

Nobody answered, so I've tested. PyGObject cannot have bytes as a signal argument.

[Saumya-Mishra9129]

The root cause of both problems is our failure to Port to Python 3 with respect to the D-Bus interface being used to obtain the connection interface buddy info from Telepathy. Instead of these two patches, I recommend 95b1c32 ("Coerce BaseBuddyModel.props.key to str"). This patch reverts and reimplements a tiny part of aa18879 ("Port to Python 3"). Please review.

Thanks @quozl , I have reviewed and tested. Looks perfect to me and with no errors in the log. The root cause was very simple and even the fix also was. I am now happy to see it this way . :smile:

[Saumya-Mishra9129]

@quozl I think we can merge this.