zopefoundation / ZODB

Python object-oriented database
https://zodb-docs.readthedocs.io/
Other
681 stars 92 forks source link

Fix TypeError for fsoids #351

Closed ale-rt closed 3 years ago

ale-rt commented 3 years ago

Fix TypeError: can't concat str to bytes when running fsoids.py script with Python 3.

Closes #350

ale-rt commented 3 years ago

I made the patch in such a way that the shorten function can handle bytes as well as text. The code we have now on master actually says we should deal with strings: https://github.com/zopefoundation/ZODB/blob/1fb097b41cae4ca863c8a3664414c9ec0e204393/src/ZODB/FileStorage/fsoids.py#L24-L32 One other possibility is to enforce the shorten function to refuse any output that is not a native string or to convert it to a native string, but this appears to me that increases the change of breaking some other stuff...

Suggestions are welcome.

Once it is clear what to do I can even add some more tests.

jmuchemb commented 3 years ago

It seems that we could simply write:

     return s[:nleading] + b" ... " + s[-ntrailing:]

because if I read the code correctly, s is always of type bytes. To be checked with a pdb.

ale-rt commented 3 years ago

@jmuchemb I also never found that the function argument is text.

Anyway the comment says # Shorten a string for display. While that is correct in Python 2, on Python 3 it is not true.

With this patch, the output of this script is something like:

oid 0x46b8 ZODB.blob.Blob 2 revisions
    tid 0x03e1ec746d9d5a77 offset=32500086 2021-08-06 13:40:25.690936
        tid user=b' ale'
        tid description=b'/Plone/workspaces/...'

Note the b'ale' and the b'/Plone/workspaces/...'.

I think that this is an error that should be fixed as well, there is no reason print the str representation of those bytes instances.

In short this patch does not solve the misuse of bytes and text, it just wants to make the script work again.

icemac commented 3 years ago

Is this PR ready to be merged or is there still something to be done to make it ready?

ale-rt commented 3 years ago

@icemac I would say this is good to be merged, I had to merge master because of a conflict

icemac commented 3 years ago

Thank you for this PR and reviewing it. 😃