Fix #28

[fmarczin]

This fixes the issue where PrefixDecorator-stores couldn't deal with other, non-prefixed keys in the base store (#28).

The first commit changes the test suite, adding extra keys to the base store which shows the issue. The second commit contains the fix.

Open issues with this patch:

1. Now all tests on PrefixDecorator-Stores run with extra keys in the base store. Maybe it makes sense to run all tests with and without those keys. But I was hesitant to blow up the number of tests by a factor of two.
2. I removed the call to _check_valid_key from _unmap_key, since at this point, key is untransformed and I think it's the responsibility of the base store to check those. If at all, we should do our own check after unmapping. But I'm not completely sure if that is the right way.

A more general point: The way the KeyTransformingDecorator is written right now, it is entirely possible to allow different keys than the base store, e.g. because the transformation can transform keys to/from whatever the base store allows. A subclass of KeyTransformingDecorator only has to define _check_valid_key() for this to work. But the tests via basic_store.py use specific invalid keys from conftest.py to check if KeyErrors are being raised. I noticed this because with a non-empty prefix, an empty key is technically valid, but the testsuite expects it to fail, even on a PrefixDecorator.

[mbr]

Alright, I've implemented the changes. I wasn't 100% happy with the PR, due to minor things:

• One commit mixed implementation and tests. That's usually fine, but in this case, it prevented me from cherry-picking the implementation, as I was not satisfied with the tests. Unfortunately, that meant that credit for you contribution in the form of being the author of said commit got lost that way. Sorry about that.
• Random data in the tests is something I try to avoid, it's better to come with something or pregenerate troubling/random cases before hand. I've also added the tests to make them test the old single-prefix and new behavior.
• For now, I'd like to keep the assertion in.

Let me know what you think, while I go fixing the Python3 compatiblity I apparently just broke. =)

[mbr]

I should also add:

Now all tests on PrefixDecorator-Stores run with extra keys in the base store. Maybe it makes sense to run all tests with and without those keys. But I was hesitant to blow up the number of tests by a factor of two.

That's okay. Travis is a patient man. =)

I removed the call to _check_valid_key from _unmap_key, since at this point, key is untransformed and I think it's the responsibility of the base store to check those. If at all, we should do our own check after unmapping. But I'm not completely sure if that is the right way.

One way of solving this could be checking for valid key names in the prefix decorator and calling the underscore-prefixed methods, skipping a possibly redundant key check.

[fmarczin]

One commit mixed implementation and tests. That's usually fine, but in this case, it prevented me from cherry-picking the implementation, as I was not satisfied with the tests. Unfortunately, that meant that credit for you contribution in the form of being the author of said commit got lost that way. Sorry about that.

That is totally fine! The commits were actually intended to keep tests and fix separate, I just goofed that up.

Random data in the tests is something I try to avoid, it's better to come with something or pregenerate troubling/random cases before hand. I've also added the tests to make them test the old single-prefix and new behavior.

I agree that it's fine to keep checking valid and invalid keys in the basic_store base class. As soon as someone uses key transformations to allow really different types of keys, this will become an issue again. This might be a tough one, since once that happens, only the person implementing the transformation knows what good and bad keys are.

About checking keys:

One way of solving this could be checking for valid key names in the prefix decorator and calling the underscore-prefixed methods, skipping a possibly redundant key check.

Why not use the public methods of the underlying store and make it their responsibility to check keys? They are already doing it. The decorator then only validates at it's own public interface, i.e. keys in the transformed space. That keeps responsibilities separate and each key is only checked once.

Should I close this PR? The issue (#28) is still there as a reminder, and the PR has served it purpose, I think.

[mbr]

Yeah, I think we can close this for now. Thanks again.