gadelkareem / sails-dynamodb

Amazon DynamoDB adapter for Waterline / Sails.js
http://sailsjs.org
46 stars 22 forks source link

[Breaking Chgs] Indices, updates, model caching, etc. #3

Closed devinivy closed 9 years ago

devinivy commented 9 years ago

I made tons of updates here. It definitely resulted in breaking changes. But I think I've added many features and made the adapter function more naturally. If you'd rather not make such breaking changes now, that's understandable! Here's what I did:

  1. Allow for named global secondary indices using index: 'IndexName-hash' and index: 'IndexName-range'. Similarly, you specify primary keys as primaryKey: 'hash' and primaryKey: 'range'.
  2. Allow for named local secondary indices using index: 'secondary'. In that case, Vogels assumes that the index name is the name of the field, which the adapter respects.
  3. Cache Vogels models instead of recreating them all the time. They live in _vogelsReferences. To avoid confusion, I renamed _modelReferences to _collectionReferences.
  4. If a table name ends in an 's' then that 's' is removed before creating the Vogels model. That allows one to create a waterline collection with identity ending in 's' and use it normally. The waterline identity can then match the table name.
  5. I don't automatically create a primary key using keyId (since the name is created once explicitly for the table) or name indices using indexPrefix (because it seems like an unnecessary limitation).
  6. Removed the automatic creation of updatedAt and createdAt in the adapter. Standard waterline collection configurations should be able to deal with that.
  7. find automatically uses the most relevant index with the following precedence,

    Primary hash and range > primary hash and secondary range > global secondary hash and range
    > primary hash > global secondary hash > no index/primary
  8. update performs updates using conditional checks, so you can use conditions other than key conditions.
gadelkareem commented 9 years ago

Thank you for your effort. I get an error while testing this code:

server-1 (err):         indexName = columnName;
server-1 (err):                     ^
server-1 (err): ReferenceError: columnName is not defined
server-1 (err):     at Object.module.exports.adapter._parseIndex (/var/www/server/node_modules/sails-dynamodb/index.js:298:21)
devinivy commented 9 years ago

Sorry, that was a silly oversight! It's a simple fix– I'll update soon.

devinivy commented 9 years ago

Should be resolved– let me know if it gives you any more issues. I expect it will not work with your current configuration, as

someName: {
  index: true
}

currently implies the key name is someName (the name of the column). It can be adjusted by using

someName: {
  index: 'someIndexName-hash'
}

using any index someIndexName.

gadelkareem commented 9 years ago

Can you add it to readme file so it would be documented?

devinivy commented 9 years ago

Do these additions seem adequate?

gadelkareem commented 9 years ago

I added a fix for endpoint while using a local dynamoDB and made changes to the model but I keep getting "Specified index is not part of table" for a model that does not have an index. Can you check?

devinivy commented 9 years ago

This occurs during a find?

devinivy commented 9 years ago

This bug should be resolved now.

gadelkareem commented 9 years ago

Creating a new record without specifying an id produces error: create error: { [Error: the value of id is not allowed to be undefined]

devinivy commented 9 years ago

I see that Vogels has an autogenerating UUID type– are you expecting to use that in a certain scenario? How should your primary key attribute be configured in order to use Vogels' UUID type? Some users may want their primary key hash to be something other than a UUID.

gadelkareem commented 9 years ago

Yes, It is part of sails framework activated by autoPK setting http://sailsjs.org/#/documentation/concepts/ORM/model-settings.html

devinivy commented 9 years ago

Okay, here's how I added autoPk support:

  1. I made the type for the autoPk default to "string"
  2. I made any auto-incrementing string attribute a UUID field, as UUIDs are the nearest way of supporting an auto-incrementing id in DynamoDB.

Because autoPk creates an auto-incrementing string attribute in the collection definition, it will become a UUID. How does that seem to you?

gadelkareem commented 9 years ago

Yes that is how it is currently working on master.

devinivy commented 9 years ago

Did that last commit make it work for you?

gadelkareem commented 9 years ago

Yes it does not seem to solve the problem server (err): /var/www/server/node_modules/sails/node_modules/waterline/lib/waterline/error/WLError.js:36 server (err): this.rawStack = (this._e.stack.replace(/^Error(\r|\n)*(\r|\n)*/, '')); server (err): ^ server (err): TypeError: Cannot call method 'replace' of undefined server (err): at new WLError (/var/www/server/node_modules/sails/node_modules/waterline/lib/waterline/error/WLError.js:36:34)

devinivy commented 9 years ago

It is working for me. I'll take a closer look, though. We should try to write some tests.

I'm also confused, as that stack trace does not include any files in sails-dynamodb.