clarkie / dynogels

DynamoDB data mapper for node.js. Originally forked from https://github.com/ryanfitz/vogels
Other
490 stars 110 forks source link

.update() does not enforce schema #39

Closed csi-lk closed 7 years ago

csi-lk commented 8 years ago

Don't know if intended or a bug but:

(copying format from update example)

const Account = dynogels.define('example-update', {
  hashKey: 'email',
  timestamps: true,
  schema: {
    email: Joi.string(),
    name: Joi.string()
  }
});
Account.update({ email: 'test@example.com', whatever: true }, (err, acc) => {
    console.log('ran);
});

Produces on dynamoDB SCAN:

0: {
  "email" : {
    "S": "test@example.com"
  }
  "whatever" : {
    "BOOL": true
  }
}

Should I be able to pass through non defined attributes in the update?

Was expecting them to error

clarkie commented 8 years ago

You're correct. I've tested this on both create and update with create returning an error:

got error { ValidationError: "somethingElse" is not allowed on foobars ...

Do you have a suggestion on how this could be fixed? How far did you get with your investigation? Thanks

csi-lk commented 7 years ago

Sorry for the late reply, haven't yet done much investigation, have also noticed running .update() will create a table item if it doesn't exist, wondering if this is also intended?

Will be able to look into this when I have time in the next few weeks

csi-lk commented 7 years ago

I've added a bounty for this on Bounty source if anyone else wants to pick it up before I have time :)

Bountysource

jkav77 commented 7 years ago

I am going to try to add some integration tests and fix this issue. I came up against the same problem on my project and I would like to see this fixed. I like this library, but this bug(s) is a problem for me so I will see if I can help out.

jkav77 commented 7 years ago

I added a conditional check to the update method that ensures the item already exists and errors if it does not. This causes most of the other update tests to fail. They test by updating an item that does not exist. This does not seem like the intended or desirable effect to me. Is that what we want to solve this issue?

I haven't started on the schema validation checks yet, but I see where that should be in the code now.