Fixes

[Saumya-Mishra9129]

@chimosky @quozl @srevinsaju Please Review. I reproduced it with find-words-activity.

[srevinsaju]

Wow! I am interested to know how you found the error. It looks interesting. I will test it on a clean build; How can I reproduce the error on master? Thanks

[Saumya-Mishra9129]

Wow! I am interested to know how you found the error. It looks interesting. I will test it on a clean build; How can I reproduce the error on master? Thanks

Thanks @srevinsaju , I got this error while testing find-words-activity on a VM with Ubuntu 20.04, I am using packaged sugar i.e. sucrose. I installed find-words-activity via terminal activity using git clone <activity-repo> . Tried opening find-words-activity , got the error in shell.log. I probably not able to reproduce it again after solving as I am not using a clean build.

[quozl]

This is wrong in a few ways;

• the commit message mentions preview instead of filesize,
• the change adds an unnecessary conversion from anything to int to the datastore, removing the strict requirement to use the correct data types in metadata.

Please instead fix the caller so that a quoted bytes of b'' is not used in metadata for creation_time, timestamp or filesize properties.

[Saumya-Mishra9129]

the commit message mentions preview instead of filesize,

This is probably a typing mistake I have done.

the change adds an unnecessary conversion from anything to int to the datastore, removing the strict requirement to use the correct data types in metadata.

The overall analysis of source code says

def _set_property(self, uid, key, value, md_path=False):
"""Set a property in metadata store
Value datatypes are almost entirely dbus.String, with
exceptions for certain keys as follows;
* "timestamp", and "creation_time" of dbus.Int32,
* "preview" of dbus.ByteArray,
* "filesize" of int, and
* "checksum" of str.
"""

The above method clearly specifies that filesize is of type int. And timestamp and creation_time take values of dbus.Int32.

def retrieve(self, uid, properties=None):
"""Retrieve metadata for an object from the store.
Values are read as dbus.ByteArray, then converted to expected
types.
"""
metadata_path = layoutmanager.get_instance().get_metadata_path(uid)
if properties is not None:
properties = [x.encode('utf-8') if isinstance(x, str)
else x for x in properties]
metadata = metadatareader.retrieve(metadata_path, properties)

    # convert from dbus.ByteArray to expected types
    for key, value in metadata.items():
        if key in ['filesize', 'creation_time', 'timestamp']:
            metadata[key] = dbus.Int32(value)
        if key in ['creation_time', 'timestamp']:
            metadata[key] = dbus.Int32(int(value))
        elif key == 'filesize':
            metadata[key] = int(value)

According to comment here # convert from dbus.ByteArray to expected types , we need to convert from dbus.ByteArray to expected datatype. I still don't know what wrong in this. I thought what I have done is correct.

[chimosky]

@Saumya-Mishra9129 said

According to comment here # convert from dbus.ByteArray to expected types , we need to convert from dbus.ByteArray to expected datatype. I still don't know what wrong in this. I thought what I have done is correct.

Yes the comment says that and the current code works for every int value as dbus.Int32 is a subclass of int hence it's difficult to understand why your change has any effect because like @quozl stated in his comment above;
"Please instead fix the caller so that a quoted bytes of b'' is not used in metadata for creation_time, timestamp or filesize properties."

Make sure the caller of datastore isn't using a quoted bytes for any of the properties above.

[quozl]

According to comment here # convert from dbus.ByteArray to expected types , we need to convert from dbus.ByteArray to expected datatype. I still don't know what wrong in this. I thought what I have done is correct.

The comment is probably correct, but it isn't to be taken as a definition of an API. Your traceback shows the value object is a str containing the text b'', and you haven't shown it is a dbus.ByteArray, though it could be. But in this case, the key is for an integer property, so the value object should have been an int, by being a dbus.Int or dbus.Int32.

Put it another way; why didn't you fix this in the activity or sugar-activity-web to make sure the value sent is an integer? Perhaps it is another one of those regressions we saw in sugar-activity-web with the port to later WebKit.

Or perhaps you have corrupt data in your datastore. Check it out.

Someone privately (don't know why privately) asked me why two of the keys are 32-bit integers instead of unspecified integer bit width. Answer is that both numbers are 32-bit on the Linux operating system at the time the datastore was written. They come from the time_t C type, see man 3 time, and read up about the 2038 problem coming up when we run out of bits for this value. For the moment, the value is 0x5EE853DF or 0b0101 1110 1110 1000 0101 0011 1101 1111, so there is one bit left. 😁

[Saumya-Mishra9129]

Put it another way; why didn't you fix this in the activity or sugar-activity-web to make sure the value sent is an integer? Perhaps it is another one of those regressions we saw in sugar-activity-web with the port to later WebKit.

Or perhaps you have corrupt data in your datastore. Check it out.

I agree with you now. Perhaps data is corrupted as I am not getting this error while testing with any other activity. More testing required instead of jumping to conclusion.
Can we add code here to test the data , and converting to specified datatype rather than just adding type conversion?

Someone privately (don't know why privately) asked me why two of the keys are 32-bit integers instead of unspecified integer bit width. Answer is that both numbers are 32-bit on the Linux operating system at the time the datastore was written. They come from the time_t C type, see man 3 time, and read up about the 2038 problem coming up when we run out of bits for this value. For the moment, the value is 0x5EE853DF or 0b0101 1110 1110 1000 0101 0011 1101 1111, so there is one bit left. grin

Interesting and new info for me :)

[quozl]

Can we add code here to test the data , and converting to specified datatype rather than just adding type conversion?

No. The additional complexity is unwarranted. Sending wrong data type will yield unpredictable outcome. It is responsibility of RPC caller to send the right data type.

Concentrate on how it is that the value became a quoted bytes.

[Saumya-Mishra9129]

I tried to look at workflow , we are calling retrieve method of metadatareader.c in order to obtain metadata.

sugar-datastore/src/carquinyol/metadatastore.py

Line 79 in e9477dc

metadata = metadatareader.retrieve(metadata_path, properties)

What this call does is that it goes to that method in file, where it reads all the properties list and makes a dictionary. To read properties list it goes to read_from_properties_list function and we are calling add_property method also there, which contains code to convert filesize to dbus.ByteArray.

[quozl]

Looking at the traceback in commit message for 968d87f, which is the only report of the problem I'm aware of, an RPC client has made a find method call, and in retrieving the metadata from the data file the Datastore has found that a value that should have been in integer form was actually found to be in quoted bytes form. Unfortunately, from the traceback we don't know anything else about the object in the datastore. The ambiguities of the situation are;

• don't know what created the datastore object,
• don't know what metadata has been written,
• don't know how the metadata acquired quoted bytes form for a value that should have been in integer form.

If you still have the datastore and can still reproduce the traceback, you can use filesystem tools to iterate through the datastore data and metadata to learn more about the object.