clarkie / dynogels

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

Storage of dates #22

Open cdhowie opened 8 years ago

cdhowie commented 8 years ago

Joi allows specification of date types, which are represented as Date objects. Unfortunately, it appears the DynamoDB document client strips out types it doesn't understand, which includes dates. For schema entries that are dates, it would be nice if dynogels would convert them to and from strings automatically.

Using joi.string().isoDate() is a workaround, though it means that dates must be manually converted into Date objects if they need to be inspected.

clarkie commented 8 years ago

I'm not sure I fully understand the issue but I did notice the other day an inconsistency between the date format returned from a create and a query. I'm happy to look into this but would you be able to provide a breaking test case or an example how you found this issue? Thanks

cdhowie commented 8 years ago

In this particular case, I was attempting to store a date in a nested property. For example, take a look at this code using Joi:

'use strict';

const Joi = require('joi');

let schema = Joi.object().keys({
    a: Joi.object().keys({
        b: Joi.date()
    })
});

schema.validate({ a: { b: '2016-01-01' } }, (err, data) => console.log(err || data));

This successfully converts the a.b attribute to a JavaScript Date object. However, if this schema is used for a table and the same object here is stored in it, the a.b attribute is removed. Presumably either dynogels or the DynamoDB Document Client don't understand how to deal with Date objects?

guotai commented 7 years ago

I also found the inconsistency between the date format returned from create and get. Basically if in the schema, I use Joi.date(), then it will be a Date object in the response of create, but it will be a string in the response of get.

Using Joi.string().isoDate() is a workaround. But I think create should provide a more consistent behavior. One possible way is to change create to serialize the response from dynamodb document client and return it (table.js:195), just same as what update is doing now(table.js:368).

jkav77 commented 6 years ago

Is this related to #107?

jkav77 commented 6 years ago

Ok so a joi.date() will validate successfully for a javascript Date object, a date string or a number of milliseconds. Joi Dates

DynamoDB does not have a date type, but you can store dates as strings (ISO 8601 date strings) or as numbers (epoch time). AWS Naming Rules and Data Types

I'm not sure that we want to convert all date strings to javascript Date objects when we retrieve them from the database. It seems like the developer's prerogative to determine the data types. However, Dynogels should be consistent between the different methods. If create serializes Date objects to strings then it should report that it stored a string.

cdhowie commented 6 years ago

I'm not sure that we want to convert all date strings to javascript Date objects when we retrieve them from the database. It seems like the developer's prerogative to determine the data types. However, Dynogels should be consistent between the different methods. If create serializes Date objects to strings then it should report that it stored a string.

+1 in general, though I would not be opposed to some option to transform dates. I would want it to be flexible enough that a different factory can be substituted. For example, some developers may want to use momentjs instead of the built-in Date.

The challenge with transforming dates is detecting what values need to be transformed, especially when dealing with nested structures and Joi.alternative() schemas, where some alternatives have dates and some don't. It seems like the only way to really be bulletproof about this is to validate the retrieved document with the schema, and let Joi transform what it will. There have been some cases in my own code where this would be useful, and it's particularly useful for backwards-compatible schema changes, where the new Joi schema can effectively migrate data on-the-fly. (For example, adding a new attribute with a default value -- old records currently just won't get that value until you save them again and this creates its own level of pain, where your application code must be able to deal with the absence of the attribute. It would be nicer for Dynogels to take care of this detail.)

So by implementing that feature, we'd effectively get this feature for free. I really like the idea that the schema coerces stored objects to match it. It would provide a much higher degree of consistency and reliability.

However, this approach can be problematic in the case where the stored data doesn't validate against the schema. It's not clear what expected behavior would be in that case.