added reduce function to btrdb Stream and BTrDB to make objects pickl…#69
added reduce function to btrdb Stream and BTrDB to make objects pickl…#69
Conversation
aorticweb
commented
Jun 17, 2020
- Added reduce function to btrdb Stream and BTrDB to make objects pickle-able. (had to modify their init functions)
|
Hi @aorticweb, can we get some tests included for this PR before I review? We'll also need information added to the documentation so people understand how to use this. Feel free to ping me on slack! |
|
Same issue with this PR. We only merge feature branches into the develop branch. Then we do a release commit to merge into master where we update the software version, etc. Can you change this PR to point to develop? |
tests/btrdb/test_stream.py
Outdated
| @pytest.fixture(scope="session") | ||
| def pickle_folder(tmpdir_factory): | ||
| fn = tmpdir_factory.mktemp("data") | ||
| return fn |
There was a problem hiding this comment.
I'm not convinced this is actually used anywhere...
There was a problem hiding this comment.
yep I removed it.
| self.conn_params = conn_params | ||
|
|
||
| def __reduce__(self): | ||
| return(self.__class__, (self.conn_params,)) |
There was a problem hiding this comment.
I read this to mean that we are essentially serializing the user's apikey into the binary file right? If that's the case then we may want to rethink how to handle that. Otherwise, anyone with access to the pickled object now has the user's api key. Ideally we'd want to re-inflate the stream with the "current" user's credentials.
There was a problem hiding this comment.
I see how this is a security issue, I will edit the file to rebuild from the the initial connection informations
| creds = credentials(conn_str, apikey) | ||
| if "endpoints" in creds: | ||
| return _connect(**creds) | ||
| return _connect(**creds, conn_params={"conn_str": conn_str}) |
There was a problem hiding this comment.
If I'm reading this correctly... conn_params does not include the apikey because we wouldnt want to have the apikey saved... but there might be a failure mode if they do supply the apikey and then the object gets unpickled and the ENV vars do not exist?
Seems like a thorny problem. I think we probably need to do an in-person code review so I understand all the implications.
|
@aorticweb This is a pretty complex problem and I think we need to setup a meeting to review this code. |
|
Closing this PR as we've decided not to allow pickle for the time being. |