Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Model.update does not create 'required' fields, create 'createAt' or create 'updateAt' timestamps. #85

Closed
aloof-ruf opened this issue Dec 8, 2016 · 11 comments

Comments

@aloof-ruf
Copy link
Contributor

Model.update() works well when the desired item already exists in the database, but does not do the following when the item does not exist:

  1. create any attributes set as 'required' (nor generate defaults)
  2. create 'createdAt' or 'updatedAt' attributes/timestamps

For example:

apple.js (define schema)

var dynamoose = require('dynamoose');

dynamoose.AWS.config.update({
  region: 'whatever'
});

dynamoose.local();

var appleSchema = new dynamoose.Schema({
  id: {
    hashKey: true,
    type: Number,
    required: true
  },
  appleType: {
    type: String,
    default: 'spartan',
    required: true,
    trim: true
  },
  status: {
    type: String,
    default: 'crisp',
    required: true,
    trim: true,
    validate: function (value) { return ['rotten', 'bruised', 'crisp'].indexOf(value) !== -1; }
  }
},
{
  timestamps: true
});

var apple = dynamoose.model('Apple', appleSchema);

module.exports = { 
   apple: apple
};

index.js (perform database operations)

var appleModel = require('./apple.js').apple;

console.log("Apples Test");

function createNewWithUpdate() {
  appleModel.update({id: 4}, {appleType: 'honeygold'}, function (error, result) {
    if (error) return console.error(error);
    console.dir(result);
  });
}

function updateApple1 () {
  appleModel.update({id: 1}, {appleType: 'gala'}, function (error, result) {
    if (error) return console.error(error);
    console.dir(result);

    createNewWithUpdate();
  });
}

appleModel.batchPut([
  {id: 1, appleType: 'granny smith'},
  {id: 2, appleType: 'spartan'},
  {id: 3, appleType: 'red delicious'}
], function (error, apples) {
  if (error) return console.error(error);
  console.dir(apples);

  setTimeout(updateApple1, 5000);
});

Then, the result when I run: aws dynamodb scan --table-name Apple --endpoint-url http://localhost:8000 is:

{
    "Count": 4, 
    "Items": [
        {
            "status": {
                "S": "crisp"
            }, 
            "appleType": {
                "S": "spartan"
            }, 
            "id": {
                "N": "2"
            }, 
            "createdAt": {
                "N": "1481235232940"
            }, 
            "updatedAt": {
                "N": "1481235232940"
            }
        }, 
        {
            "status": {
                "S": "crisp"
            }, 
            "appleType": {
                "S": "gala"
            }, 
            "id": {
                "N": "1"
            }, 
            "createdAt": {
                "N": "1481235232940"
            }, 
            "updatedAt": {
                "N": "1481235232940"
            }
        }, 
        {
            "status": {
                "S": "crisp"
            }, 
            "appleType": {
                "S": "red delicious"
            }, 
            "id": {
                "N": "3"
            }, 
            "createdAt": {
                "N": "1481235232940"
            }, 
            "updatedAt": {
                "N": "1481235232940"
            }
        }, 
        {
            "appleType": {
                "S": "honeygold"
            }, 
            "id": {
                "N": "4"
            }
        }
    ], 
    "ScannedCount": 4, 
    "ConsumedCapacity": null
}

Notice how the item with id: 4 contains attributes appleType and id but does not contain anything for the 'required' attribute status nor does it create the timestamps I specified with {timestamps: true}.

I feel the expected result should be either a rejection/fail when performing an update on an item that does not exist (instead of creating it), or required fields and timestamps should be filled in as they are with Model.create().

@brandongoode
Copy link
Contributor

Sorry for the slow response and thanks for all the detailed info. I'll look into fixing this as soon a can.

If someone want's to try to beat me to it, a PR would be welcome.

@aloof-ruf
Copy link
Contributor Author

I'd love to make a PR for it, but I'm stuck on one thing.

Off the top of my head, I can't think of any other way to enforce required than by passing defaults/previous values of required attributes with each update. However, if the required attribute is large enough this could make updates cost more (ie, use up more write units).

I'll use my 'apple schema' as an example. When I perform the 'honeygold' update, I only pass the id and type. I only expect my update to affect type (or create a new item if it doesn't exist). I may not have any idea beforehand if the item exists. To force the update to have a value for updatedAt, createdAt, and status (the last one being required) I feel like I must send default (or at least current) values for those attributes. This, however, makes the update larger and, with sufficiently large required attributes, this may raise costs unexpectedly for dynamoose users.

What are your thoughts on this @brandongoode? If you are okay with this approach or have one of your own, let me know and I can make a PR.

@brandongoode
Copy link
Contributor

That was the same thing I've been debating. The nice thing about the update is the ability just to send the attributes that are updated. Just sending the updates saves me on some of my projects. I don't want to lose that aspect of the update.

What I started thinking about doing was creating an option (schema or system) that would allow you to have an update fail if the key didn't exist. Then you could use a create or save to apply the defaults. This could either be done automatically by dynamoose or manually.

What are your thoughts?

@aloof-ruf
Copy link
Contributor Author

aloof-ruf commented Dec 20, 2016

Okay, I've given this some thought and I think our options are:

  1. Perform the updated by 'shimming-in' the required parts as I described (could cost users more, doing nasty things behind the scenes, but don't lose functionality)
  2. 'Demote' the required attribute to something like requiredOnCreate that explicitly tells the user what's going on (not the best though, as the 'required' feature would be lovely)
  3. Do as you described. Have an additional option that throws when the user tries to make an update to a key that doesn't exist. This option could be toggled so that the user can still get the ol' create-if-it-doesn't-exist treatment.

I feel that the third option is good, however there are some drawbacks. As the default behaviour of dynamoDB is to 'update-or-create', restricting this might feel limiting to some regular dynamoDB users. There is also the matter of actually knowing if we are creating or doing an update. For example, let's say we have the option 'fail-on-update-create' (not a great name, I know) which is set to true. The user is careful but misses one instance where they would 'create' with their update. We won't know that this update is actually a 'create' until we actually perform the update (or we do a query beforehand). Thus, we are bound to use additional read/write units in order to check to see if our update is actually a create.

I suggest doing both option 2 and 3. We carry a few more options that will describe desired behaviour on create and update. These options are then well documented to let users know what they're getting into.

@brandongoode
Copy link
Contributor

I agree. A combination of 2 & 3 with the default behavior being the current behavior (at least for now - to prevent breaking existing code).

For the 'fail-on-update-create', we may be able to use the ConditionExpression to keep it from creating. This would not require any additional read/write units. It would be the reverse logic of the overwrite = false used in .create.

Option name ideas:

  • fail-on-update-create
  • strictUpdating
  • doNotCreateOnUpdate

@joshuadutton
Copy link
Contributor

joshuadutton commented Dec 22, 2016

I've been working with @aloof-ruf on this, and I would like to propose a 4th option:

DynamoDB UpdateItem is more flexible than PutItem and it also will create an item if it doesn't exist. I would like to change all the create, save, put, and update logic to use UpdateItem and use DynamoDB's if_not_exists function on each attribute that has a default value that is not provided. That way we will correctly add default attribute values without overwriting existing ones. Since DynamoDB handles all this logic for us, it doesn't require any additional read/write units.

For required attributes that don't have a default value: Do as you suggest and use a conditional expression using attribute_not_exists so it will produce an error if all the required attributes aren't there. That way it will fail on create if any required attributes are missing and will only fail on update if a required attribute is missing and the item wasn't created previously.

The only downside is that from what I see in the documentation, DynamoDB's BatchWriteItem doesn't support the if_not_exists function for attributes. So even though it can update existing items, we should discourage that usage since it could overwrite existing values with default ones.

I will create a PR for this if you agree with this direction going forward.

@joshuadutton
Copy link
Contributor

@brandongoode
Copy link
Contributor

UpdateItem and PutItem behave differently. PutItem replaces the whole data structure, so if you remove attributes or add them, they are 100% in sync. The PutItem logic aligns nicely with the create, save, and put actions. This could be done with UpdateItem but would require you to have all the manual adds and removes in the update expression.

That being said, I've meant to move to using the newer conditional expressions for both PutItem and UpdateItem:

http://docs.aws.amazon.com/amazondynamodb/latest/developerguide/Expressions.SpecifyingConditions.html

Then you could use the attribute_exists option on the UpdateItem to for an exclusive update.

@joshuadutton
Copy link
Contributor

Thanks for the explanation. I didn't realize that PutItem does a replace if the object exists. Given that, I agree that create, save, and put actions should remain the same. Since you already check that all required fields are there before calling PutItem, the only advantage I see of using conditional expressions would be to have a conditional create that would only create an object if it doesn't already exist.

So for now we will just modify the update action to use the if_not_exists function for default values and the conditional expressions with attribute_not_exists to check required values for the case that update is called and the object didn't exist previously.

@brandongoode
Copy link
Contributor

Sounds like a good plan. Looking forward to your PR.

@brandongoode
Copy link
Contributor

Closed by #96

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants