Implementation for Amazon DynamoDB

[nicktacular]

Added an implementation for Amazon DynamoDB. Using the $options key to the constructor, you can set which $storageName uses which key name for access. For example, if your key-val store called Awesome uses a key called MyKey then you can set it like so:

$credentials = new \Aws\Credentials\Credentials('yo', 'sup');

$aws = new \Aws\Sdk([
    'timeout' => 1,
    'credentials' => $credentials,
    'region' => 'us-west-2',
    'version' => 'latest',
    'endpoint' => 'http://127.0.0.1:11888',
]);

$storage = new AmazonDynamoDbStorage(
    $aws->createDynamoDb(),
    null,
    ['storage_keys' => ['Awesome' => 'MyKey']]
);

Otherwise, the option default_key_name which defaults to Id will be used as the key name.

[nicktacular]

Oh, also fixes #41.

[beberlei]

@nicktacular is the storage thing sometihng we could better configure in the mappings, i.e. annotations xml and such?

Otherwise this looks really great, what do you think @EmanueleMinotto ?

[EmanueleMinotto]

Probably would be better to declare and define the properties private $defaultKeyName = 'Id' and private $storageKeys = [], for some reasons:

1. $options is used only by AmazonDynamoDbStorage so its content don't need to be flexible
2. if the final developer defines the default_key_name key as a non-scalar type, the class will generates different unexpected errors
3. a more strict type checking could be used asking for an array $storageKeys = []

What do you think about this @nicktacular?

[nicktacular]

Agreed, will implement strict checking as well as separate these into different arrays.

[EmanueleMinotto]

@nicktacular I'm not sure if I'm understanding the part:

Using the $options key to the constructor, you can set which $storageName uses which key name for access.

In this moment, looking the implementation, it seems a lot like the DoctrineCacheStorage::flattenKey, can you provide a more concrete example please?
It could be useful for documentation too.

[nicktacular]

@EmanueleMinotto Sorry, I wasn't very clear there. What I meant to say is that the default name of the key is configurable. Also, if you want to override that key for any particular table (i.e. $storageName) then you can do that as well. Once I fix the format of the constructor and the way this is stored, I think it'll be clearer.

[nicktacular]

I believe I've addressed all the concerns except for this comment from @beberlei:

is the storage thing sometihng we could better configure in the mappings, i.e. annotations xml and such?

I agree that perhaps this would be a good place handle this. Can you give me an example of what you'd like the annotation to look like?

Also, I'm wondering if I should write an integration test for an end-to-end test, configurable via PHPUnit with <var name="DOCTRINE_KEYVALUE_AMAZON_DYNAMODB_ENDPOINT" value="http://localhost:8000" /> (among other properties) where, if you wish, I can write this test to work against some endpoint. One example could be to use DynamoDB local for this and skip those tests when this isn't configured. Thoughts on this?

[beberlei]

@nicktacular Integration test sounds great that only runs when all required environment variables are set, use env variables for this please, because other tools can do that more easily.

[EmanueleMinotto]

@nicktacular what's the status of the integration/e2e test addition?

[Ocramius]

must be require-dev

[nicktacular]

Sorry for the delay. Working on some issues with work; will be back to completing these tests shortly.

[Ocramius]

Please throw specialized exceptions, not SPL exceptions (write a new exception class - you may look at http://rosstuck.com/formatting-exception-messages/ )

[EmanueleMinotto]

@nicktacular don't worry, that's not a problem, thank you for your effort (ping me on IRC if you need a help) :)

[EmanueleMinotto]

@nicktacular do you have news about this? If you can't, than I could merge this into a separate branch and complete it by myself :)

[nicktacular]

@EmanueleMinotto I've been really sick recently. I also couldn't get integration tests to work, so I'm going to implement using DynamoDb local store instead. I will let you know as soon as I'm done.

[EmanueleMinotto]

oh ok, no problem
thanks @nicktacular, let me know :)

[nicktacular]

Rewrote functional tests to utilize DynamoDb local (removed waits and such), but now it seems to be randomly failing, possibly to do with something on OS X as per this AWS thread. I'm going to bring up an Ubuntu machine and try there.

[nicktacular]

Ok so it's not Dynamo. It's BasicCrudTestCase class which seems to be taking an inordinate amount on time on something. It's approximately 17 seconds per test and it seems to be hung up on the call:

$this->manager = $this->createManager($this->storage, $mappingDriver);

For what reason I can't understand, but I'm going to dig into this now to see why this is happening. Updates to follow...

[nicktacular]

Well, that was quick. When commenting out the xml mapping driver, the test speeds up immensely. So, I'm going to hazard a guess that the XSD reference in the Post fixture for XML mapping is what's causing this hangup:

<?xml version="1.0" encoding="UTF-8"?>
<doctrine-mapping xmlns="http://doctrine-project.org/schemas/orm/doctrine-mapping"
      xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
      xsi:schemaLocation="http://doctrine-project.org/schemas/orm/doctrine-mapping
                          http://raw.github.com/doctrine/doctrine2/master/doctrine-mapping.xsd">
    <entity name="Doctrine\Tests\KeyValueStore\Functional\Post" storage-name="post">
        <id>id</id>
    </entity>
</doctrine-mapping>

[nicktacular]

Ok, so I've discovered a conceptual issue with tables in DynamoDb. How do we want to deal with mismatches on types for ids? Suppose, for example, the type is a string id in DynamoDb and the id is set to an integer. Should the type of the primary key be deduced by means of a describeTable operation on the table first, or provide it as a configuration option?

When this occurs, the raw exception from DynamoDb looks like this:

Client error: `POST http://localhost:8000` resulted in a `400 Bad Request` response:
{"__type":"com.amazon.coral.validate#ValidationException","message":"Type mismatch for attribute to update"}

Doing a describeTable operation seems wasteful, so I'd prefer to rely on configuration (i.e. annotation) instead. Any thoughts on this?

[nicktacular]

Having thought about this more, I think that the right approach is to use an appropriate exception message to indicate that there is an attribute mismatch and leave it up to the consumer for resolving this problem. Additionally, a new configuration will need to be set to indicate the type of key that is on the table (or keys).

Furthermore, an additional test will need to be added to test composite keys which is the error @TiagoBrito mentions. However, it's only feasible to deal with this error when the table and entity actually have composite keys. If there is a mismatch, it will still be up to the developer to address this issue and ensure that the table key schema and entity match.

Finally, I'm not convinced that these configuration options make sense to be annotations. These configurations are too specific to the adapter, so I think they should be adapter configurations and not moved to annotations.

In summary:

• Will add configuration options so that the developer can configure the key or keys and the type for each key (string vs numeric) and the associated tests.
• Will add functional tests for composite keys.
• Will add functional tests for mismatches which will necessarily throw an exception with an adequate message explaining why this is an error and what is needed to address.

I will wait a few days before I continue this implementation in case there are any serious concerns with this approach.

[EmanueleMinotto]

well done @nicktacular, I'm also against the usage of a new annotation for a single driver, so 👍 for:

deduced by means of a describeTable operation on the table first

or other configurations you think are required for the DynamoDB storage

[larowlan]

DynamoDb chokes on empty values.
E.g

$data = [
  'foo' => '',
  'bar' => 'baz',
];

Would choke. The existing implementation includes a prepareData method - however it only consider one-dimensional arrays, multi-dimensional cleaning is needed too.

e.g

$data = [
  'foo' => [
     'baz' => '',
     'bar' => 'boo',
  ],
  'bar' => 'baz',
];

needs to be sent as

$data = [
  'foo' => [
     'bar' => 'boo',
  ],
  'bar' => 'baz',
];

[larowlan]

Promised work is in nicktacular#2, sent against @nicktacular branch so once combined will be available here

[larowlan]

Let me know if you'd prefer to close this and open a new one instead of sending fixes against @nicktacular branch