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

Trouble with Date and ObjectID Data Types #124

Open
ghost opened this issue May 23, 2018 · 2 comments
Open

Trouble with Date and ObjectID Data Types #124

ghost opened this issue May 23, 2018 · 2 comments

Comments

@ghost
Copy link

ghost commented May 23, 2018

Working with [email protected] on [email protected] and [email protected].

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
Copy link

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
Copy link

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

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

No branches or pull requests

1 participant