Add simplekv.transform to apply value transformations like gzip and encrypt

mbr / simplekv

A simple key-value store for binary data.

http://simplekv.readthedocs.io

MIT License

152 stars 50 forks source link

Add simplekv.transform to apply value transformations like gzip and encrypt #60

Closed fmarczin closed 3 years ago

fmarczin commented 7 years ago

@mbr What is your take on adding this? I think this could be a very versatile and useful thing.

criemen commented 6 years ago

Ping @byjott

coveralls commented 6 years ago

Coverage decreased (-2.1%) to 87.239% when pulling 4e9032b257a1d0fa39998c9db8583bd70adbf857 on fmarczin:transforms into c8c875adc8e36c2c52d07f49317e000952cd5930 on mbr:master.

coveralls commented 6 years ago

Coverage decreased (-2.1%) to 87.248% when pulling 4e9032b257a1d0fa39998c9db8583bd70adbf857 on fmarczin:transforms into c8c875adc8e36c2c52d07f49317e000952cd5930 on mbr:master.

coveralls commented 6 years ago

Coverage decreased (-2.1%) to 87.248% when pulling 4e9032b257a1d0fa39998c9db8583bd70adbf857 on fmarczin:transforms into c8c875adc8e36c2c52d07f49317e000952cd5930 on mbr:master.

jochen-ott-by commented 6 years ago

Concerning your high-level comments:

on the crypto part: I can move it out of here and into a separate python package if you want, but leave the "framework" of value transformation and the gzip transformation here. Note, though that I do not consider that a case of "own crypto", as the parameters and algorithms follow best-practice standards. Once you follow them, there actually isn't much choice left: encrypt-then-MAC with AES in CTR mode with a reasonable Mac determines almost everything.
for the Pipe mechanism: I consider KeyValueStores to be rather heavy-weight objects, so chaining several ValueTransformingDecorator together feels rather unnatural if the same effect can be achieved via very light-weight chaining achieved in Pipe, which consists of just under 10 lines of code. More specifically, the intermediate objects created if stacking ValueTransformingDecorator, especially allocating several read buffers seems to be overcomplicated.

coveralls commented 6 years ago

Coverage decreased (-2.7%) to 86.755% when pulling fffe53ab41a30630b186f473648740061b06b246 on fmarczin:transforms into 4d99e2b166f260d0e7460749d0f95217d1758d3d on mbr:master.

jochen-ott-by commented 6 years ago

I think I addressed all comments now. The decreasing coverage is something I don;t understand, as it affects code I never touched.

criemen commented 6 years ago

The azure tests are only run on mbr-repo-branches, as not to expose the credentials (a travis security feature). Thus, by design these are not run and coveralls detects this. Maybe we should exclude that file from coveralls, but on the other hand, for branches in this repo it is quite handy. I will try to review the changes next week.

coveralls commented 6 years ago

Coverage decreased (-2.7%) to 86.755% when pulling c3e43340c2afcd30824ddf441100f9bc1f3cea4b on fmarczin:transforms into 4d99e2b166f260d0e7460749d0f95217d1758d3d on mbr:master.

fmarczin commented 3 years ago

Closed due to staleness