doctrine / KeyValueStore

Abstraction for Key-Value to Plain Old PHP Object mapping
http://www.doctrine-project.org
MIT License
200 stars 59 forks source link

Shared key flattening #65

Open EmanueleMinotto opened 8 years ago

EmanueleMinotto commented 8 years ago

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.

billschaller commented 8 years ago

:+1:

billschaller commented 8 years ago

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);
...
}
EmanueleMinotto commented 8 years ago

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:

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

billschaller commented 8 years ago

@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;
    }