Use StorageKey for prefixes [ECR-831]

[vitvakatu]

• Removed gen_prefix function
• Changed signature of with_prefix method

UPDATED

• Replaced with_prefix with new_in_family.

TODO:

• Add changelog entry

[vitvakatu]

Yay, finally it builds

[slowli]

Looks pretty good as of fc145e1.

I wonder if it could make sense to refactor index creation to something like

let x = BaseIndex::new(&snapshot).downcast::<MapIndex<_, K, V>>()?;

That is:

• Hide all non-BaseIndex constructors
• (Optional) Hide the BaseIndex constructor too, instead implementing something like snapshot.index("foo") and fork.index_mut(("bar", &public_key)) (both would return BaseIndex).
• Introduce downcasting to indexes that can fail (and will according to Implement runtime index type checks [ECR-705] #525).

Some description for the second step: introduce a trait

enum IndexLocation {
    Name(String),
    PrefixedName(String, Vec<u8>),
}

impl<'a, S: 'a + AsRef<str>> From<&'a S> for IndexLocation { /* ... */ }
// lifetimes omitted for brevity
impl<S: AsRef<str>, K: StorageKey> From<(&S, &K)> for IndexLocation { /* ... */ }

trait IndexBag<S: AsRef<Snapshot>> {
    fn snapshot(&self) -> S;

    fn index<L, K, V>(&self, location: L) -> Option<BaseIndex<S, K, V>>
    where
        L: Into<IndexLocation>,
        K: StorageKey,
        V: StorageValue
    { /* checks go here */ }

   // Maybe, some other methods
}

// This may be be temporary impl: the checks may be not confined to
// the snapshot, but to more wide context as well (e.g., the service
// requesting access to the storage)
impl<'a> IndexBag<&'a Snapshot> for &'a Snapshot {
    fn snapshot(&self) -> &'a Snapshot { self }
}

Analogous trait can be implemented for mutable indexes:

trait IndexBagMut: IndexBag {
    fn fork(&mut self) -> &mut Fork;
    fn index_mut(&mut self, ...) { /* ... */ }
}

[slowli]

A side note: maybe, it would sense to generate (with macros) StorageKey for [u8; $size] with reasonably small $sizes?

[vitvakatu]

I think no, as user will expect that static arrays are working and we'll get a lot of questions like "I've created array of 33 elements but can't use it, what's going on?". I'm looking forward for const expressions in generics.

[slowli]

I don't see it any different from the standard library, which too has "privileged" array types. But I won't insist, ofc.

[dmitry-timofeev]

That's great to see a PoC, but I don't think we can and should merge it into master as is, because there is a discussion about the idea we want to convey with prefixes and the requirements. Until these things are perfectly clear to everyone, we must not proceed with either design or implementation.

On top of that, by making the existing prefix a StorageKey, the existing thing becomes even more dangerous! Currently it's quite a low-level thing: a byte array. This PR makes it any object that has a StorageKey implementation, creating a false impression that the framework handles any prefixes produced by the serialization code. That's a lie!

Please join the discussion, there are open questions: https://wiki.bf.local/display/EXN/2018-02-27+Meeting+Notes%3A+Friendly+Prefixes

[slowli]

@dmitry-timofeev As I see it, this API (and creation of indexes in general, for that matter) is low-level plumbing that should be visible / useful for core devs, not service devs. That is, whether this is merged or not, the entire approach with service devs exposed to prefixes is still suboptimal and should be improved.

[vitvakatu]

Anyway, I'm waiting for @DarkEld3r decision.

[dmitry-timofeev]

is low-level plumbing that should be visible / useful for core devs

I agree it is low-level, but this PR disguises it as a high-level API, therefore, making it more dangerous. Some serialization code may produce a prefix that is a prefix of another existing prefix!

… not service devs.

That's not entirely true, as we've discovered during the meeting (you must have seen the notes) -- there are valid use-cases when you have to resort to using prefixes to solve a problem at service level.

[stanislav-tkach]

Personally, I don't see a problem in these changes (except for the naming: we should replace "prefix" by something more suitable). As for me, both current and updated APIs require reading the documentation for understanding what this additional parameter is for. Plus we probably preserve API in this state for some time, only prefixes collisions should be fixed, but that is an implementation detail.

[stanislav-tkach]

As for the naming, I'm thinking of something like that:

IndexType::new<S: AsRef<str>>(index_name: S);
IndexType::new_in_family<S: AsRef<str>, I: StorageKey>(family_name: S, index_id: I);

And one more thing: we should prevent the creation of different indexes with the same index_id, just as we do for index names.

[vitvakatu]

@DarkEld3r it's not obvious for me why we should check index_id for uniqueness. It's absolutely normal to create several indexes with different family names, but same index ids. It's impossible to use the same index id on the different storage type, because we check storage type while creating family.

[stanislav-tkach]

@vitvakatu You are right, but still there is a case which we should prevent: user shouldn't be able to create an index (even with the same type) with "prefix" if a non-prefixed index with the same name already exists (or otherwise).

[stanislav-tkach]

This test should be renamed. 🙃

It can be named index_in_family_as_ordinary_index and the previous test can be named ordinary_index_as_index_in_family.

Discussed: will be fixed as separate task