SierraSoftworks / Iridium

A high performance MongoDB ORM for Node.js
http://sierrasoftworks.github.io/Iridium/
Other
569 stars 25 forks source link

Trouble with Date and ObjectID Data Types #124

Open ghost opened 6 years ago

ghost commented 6 years ago

Working with iridium@8.0.0-alpha.13 on node@9.4.0 and npm@6.0.1.

Four things here: 1) The built-in Date validator won't accept a valid date string. Is that intentional? 2) Model.save doesn't execute transforms. Is that also intentional? 3) I can't make an ObjectID field other than _id required. 4) ObjectIDs are not converted to strings on retrieval

Consider the following test:

import { ObjectId } from 'bson';
import { Collection, Core, Instance, Model, ObjectID, Property, Transform } from 'iridium';
import { expect } from 'chai';
import * as config from 'config';
import moment = require('moment');

process.env.NODE_ENV = 'test';

interface TestDocument
{
    _id?:string;
    job:string|JobDocument;
    startDate:Date;
}

interface JobDocument
{
    _id?:string;
}

@Collection('tests')
class TestEntity extends Instance<TestDocument, TestEntity> implements TestDocument
{
    @ObjectID
    _id:string;

    // No way to require this field
    // @Property(ObjectId, true) with a custom transform to/from ObjectId doesn't require it either
    @ObjectID
    job:string;

    // It'd also be nice if I didn't have to transform a date string
    @Property(Date)
    @Transform(d => d, d => moment(d).toDate())
    startDate:Date;
}

describe('save/insert discrepancy', () =>
{
    const ctxt:Core = new Core(config.Database.connectionOptions.url),
        db:Model<TestDocument, TestEntity> = new Model<TestDocument, TestEntity>(ctxt, TestEntity);

    before(async () =>
    {
        await ctxt.connect();
    });

    after((done) =>
    {
        ctxt.db.dropDatabase().then(() => done());
    });

    describe('TestEntity', () =>
    {
        it('does not run transforms using Model.save', async () =>
        {
            try
            {
                const test = {
                    job: '123123123123',
                    startDate: '2018-05-05 22:39:50.362Z'
                };

                const entity = new TestEntity(db, <any>test);
                await entity.save();
            }
            catch(e)
            {
                const msg = [
                    'Expected "123123123123" to be a valid MongoDB.ObjectID object',
                    'Expected startDate to be a date, but got "2018-05-05 22:39:50.362Z" instead'
                ].join('\n');

                expect(e.message).to.eq(msg);
            }
        });

        it('properly runs transforms using Model.insert', async () =>
        {
            const test = {
                job: '123123123123',
                startDate: '2018-05-05 22:39:50.362Z'
            };

            const res = (await db.insert(<any>test)).toJSON();
            expect(res.job instanceof ObjectId).to.eq(true);
            expect(res.startDate instanceof Date).to.eq(true);
        });

                it('does not convert ObjectID to string on retrieve', async () =>
        {
            const test = {
                job: '123123123123',
                startDate: '2018-05-05 22:39:50.362Z'
            };

            await db.insert(<any>test);
            const doc = (await db.findOne({ job: '123123123123' })).toJSON();
            // Fails
            expect(typeof doc.job).to.eq('string');
        });

        it('does not require the job field, but I\'d sure like it to', async () =>
        {
            const test = {
                startDate: '2018-05-05 22:39:50.362Z'
            };

            const res = (await db.insert(<any>test)).toJSON();
            expect(res.startDate instanceof Date).to.eq(true);
        })
    });
});
troywweber7 commented 6 years ago

Regarding the @ObjectID decorator, there is actually a bug in their code.

Here is where the bug is: https://github.com/SierraSoftworks/Iridium/blob/master/lib/Decorators.ts#L144

They simply need to change the order of the calls to the Property and Transform decorators and they will be good to go.

Further, they might consider changing the decorator to something like below to allow the user to make the field required or not.

export function ObjectId(required = true)
{
    return function(target, name)
    {
        Transform(DefaultTransforms.ObjectID.fromDB, DefaultTransforms.ObjectID.toDB)(target, name);
        Property(ObjectId, required)(target, name);
    };
}
troywweber7 commented 6 years ago

It seems there is actually a larger issue with their @Property decorator not properly requiring an ObjectId but always allowing that to be optional...