KeyValueStore icon indicating copy to clipboard operation
KeyValueStore copied to clipboard

Shared key flattening

Open EmanueleMinotto opened this issue 9 years ago • 4 comments

Closes https://github.com/doctrine/KeyValueStore/issues/63

I'm not fully sure about the method output because it isn't based on a RFC nor a standard, but this could be done in the v2.

EmanueleMinotto avatar Dec 15 '15 23:12 EmanueleMinotto

:+1:

billschaller avatar Dec 16 '15 00:12 billschaller

Looking at this a bit more, it might be better to roll find, insert, update, delete up into AbstractStorage as well. Once the key is flattened, you can have a simpler internal storage API that simply accepts a string key. For storages that implement composite keys without flattening, they can simply skip inheriting AbstractStorage and directly implement Storage.

What do you think?

class AbstractStorage implements Storage
{
...
    /**
     * {@inheritDoc}
     */
    public function insert($storageName, $key, array $data)
    {
        $key = $this->flattenKey($storageName, $key);
        $this->doInsert($storageName, $key, $data);
    }

    /**
     * @param string $storageName
     * @param string $key
     * @param array $data
     */
    abstract protected function doInsert($storageName, $key, $data);
...
}

billschaller avatar Dec 16 '15 01:12 billschaller

That's interesting! So basically do you suggest to follow internally the same doctrine/cache implementation?

But imo the number of abstractions should be as low as possible until a complete overview of the features/requirements is not done, for example:

  • the flattenKey method is implemented by 2/11 of the currently available storages
  • the RangeQueryStorage interface is implemented by 3/11 of the currently available storages

Honestly I don't know which are the implications (in a long term) of that approach in a system that could become more complex than the doctrine/cache.

So imo that approach should be considered only after the v1. /cc @beberlei @Ocramius

EmanueleMinotto avatar Dec 16 '15 08:12 EmanueleMinotto

@EmanueleMinotto

Yeah, upon further consideration this morning maybe that's not the best route.

Currently, SingleIdHandler is taking array keys and returning only the member matching the first identifier metadata. I think if you flatten keys at all, they should be flattened in the IdHandler instead of at the Storage level.

Looking at the CouchDB Storage, it seems as though this code in flattenKey never even runs, because the SingleIdHandler flattens the key already:

    private function flattenKey($storageName, $key)
        ...
        if ( ! is_array($key)) {
            throw new \InvalidArgumentException('The key should be a string or a flat array.');
        }

        foreach ($key as $property => $value) {
            $finalKey .= sprintf('%s:%s-', $property, $value);
        }

        return $finalKey;
    }

billschaller avatar Dec 16 '15 15:12 billschaller