search

torproject / stem

Python controller library for Tor
https://stem.torproject.org/
GNU Lesser General Public License v3.0
279 stars 76 forks source link

Encode object before hashing #60

Closed juga0 closed 4 years ago

juga0 commented 4 years ago

When parsing cached files the following exception is raised: TypeError: Unicode-objects must be encoded before hashing

Encoding before hashing solve this issue.

This patch fails for python < 3.6 (https://travis-ci.org/github/juga0/stem/builds/676610639), so it needs to be modified to support those versions.

atagar commented 4 years ago

Hi juga. Stem's master branch requires python 3.6 or above so there's no need to support lower versions.

Could you please provide a unit test that reproduces this issue so I can see it (and prevent future regressions)?

Thanks!

arthtyagi commented 4 years ago

Hey @atagar but wouldn't backwards compatibility be a good thing?

juga0 commented 4 years ago

Hi, sorry i haven't written the test yet. If it is fast for any of you, feel free to fork this branch, add the test and then merge that. Otherwise i'll try to find some time.

atagar commented 4 years ago

Hey @atagar but wouldn't backwards compatibility be a good thing?

Nope, indefinite backward compatibility hurts. We use semantic versioning, for more information see...

https://stem.torproject.org/change_log.html#versioning

arthtyagi commented 4 years ago

Hey @atagar but wouldn't backwards compatibility be a good thing?

Nope, indefinite backward compatibility hurts. We use semantic versioning, for more information see...

https://stem.torproject.org/change_log.html#versioning

Oh alright, I read more about semantic versioning and it makes more sense. Thanks for letting me know :)

vEpiphyte commented 4 years ago

@atagar Sorry to message you directly on this PR, but I've been exploring this due a desire to use the caching; but even with these changes, I'm unable to get the file caching to work at all with real-world tests of a patched version of the 1.8.0 STEM library, trying both the proposed PR here and similar changes from the master branches collector.py. There also appears to be no test coverage for this functionality either - is this test omission intentional?

atagar commented 4 years ago

Hi vEpiphyte. As mentioned earlier on this ticket juga's branch is awaiting a unit test that reproduces the problem they mentioned so no, the lack of coverage is not intentional.

If somebody has code to reproduce a problem I'd be delighted to take a patch, but otherwise I'll probably close this PR at some point.

vEpiphyte commented 4 years ago

Would you prefer a reproduction case/test against the 1.8.0 tag or master ?

atagar commented 4 years ago

Against the master branch, please.

atagar commented 4 years ago

Closing. Feel free to reopen if we have a repro for the issue being addressed.