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

Inconsistent type of 'date-time' Model properties on instances #663

Closed
newhouse opened this issue Dec 14, 2017 · 11 comments
Closed

Inconsistent type of 'date-time' Model properties on instances #663

newhouse opened this issue Dec 14, 2017 · 11 comments

Comments

@newhouse
Copy link
Contributor

So I was writing/running some tests today and I was seeing some strange, inconsistent behavior. My app is a single-page app, so it makes a sort of "initialize" API call to start things off. This call does the following:

  • checks the cache for the necessary data for the requesting User
    • If it's found, user gets is value by being parsed something like const user = User.fromJson(dataFromCache);
    • If it's not found, user gets its value from a query that is made with a bunch of eager clauses for the User and other relations that I need.

Let's say that User has a HasManyRelation relation to Roles, and each Role has a created_at property which is defined on the schema as a string with the date-time format. I had some logic that was looking through the User.roles array for the Role with the most recent created_at time. The particulars of that logic, when I was writing/testing it originally, had found that typeof role.created_at was an object.

When I today found this issue, in my investigation it turns out that typeof role.created_at was actually string. Upon further investigation, the difference (at least in this particular case) between when typeof role.created_at was an object vs when it was a string depended on whether or not the User.roles were found in cache and loaded via User.fromJson(...) or whether they were loaded via User.query().eager('roles')....

That was my "eureka" moment.

It turns out that Objection (or something it uses) is sometimes making created_at a Date object and sometimes making it a String. This is a problem! I wrote a gist here to reproduce this issue, and also highlight a bunch of other scenarios where it comes out as one type or another:
https://gist.github.com/newhouse/c3b052a3548396274210827c6be66875

As you'll see, depending on how you are obtaining your instance, the time field comes out as a different type. In short:

  • Instances returned through a single (vs graph) insert return it as a string
  • Instances returned through a graph insert underneath a parent return it as a string (though time fields in the parent are objects!!!)
  • Instances created from SomeModel.fromJson({...}) return it as a string.
  • All other ways I tried return it as an object.

There are some interesting inconsistencies in the gist.

Needless to say, I'm not sure which way it "should" be, but at least if it were consistent, one could code with the assumption that it will always be a string (or object or whatever). This has caused me a lot of headaches.

Is this an Objection issue?

@koskimas
Copy link
Collaborator

Objection does no type conversions whatsoever. Neither does knex. It's the db driver that does that. With postgres you can use pg-types library to change this behaviour. You can also implement the $fromDatabaseJson to make the conversion.

@koskimas
Copy link
Collaborator

koskimas commented Dec 14, 2017

class BaseModel extends Model {
  $parseDatabaseJson(json) {
    json = super.$parseDatabaseJson(json);
    
    const propSchemas = this.constructor.jsonSchema.properties;
    
    Object.keys(propSchemas).forEach(prop => {
      const schema = propSchemas[prop];
      const value = json[prop];
      
      if (schema.format === 'date-time' && value instanceof Date) {
        json[prop] = value.toISOString();
      }
    });

    return json;
  }
}

class User extends BaseModel {
  ...
}

class Role extends BaseModel {
  ...
}

@koskimas
Copy link
Collaborator

koskimas commented Dec 14, 2017

Or with pg-types:

const types = require('pg').types
const moment = require('moment')

const TIMESTAMPTZ_OID = 1184
const TIMESTAMP_OID = 1114

const parseFn = (val) => {
   return val === null ? null : moment(val).toISOString()
}

types.setTypeParser(TIMESTAMPTZ_OID, parseFn)
types.setTypeParser(TIMESTAMP_OID, parseFn)

@koskimas
Copy link
Collaborator

Actually, this is all you need in $parseDatabaseJson

class BaseModel extends Model {
  $parseDatabaseJson(json) {
    json = super.$parseDatabaseJson(json);
    
    Object.keys(json).forEach(prop => {
      const value = json[prop];
      
      if (value instanceof Date) {
        json[prop] = value.toISOString();
      }
    });

    return json;
  }
}

class User extends BaseModel {
  ...
}

class Role extends BaseModel {
  ...
}

@newhouse
Copy link
Contributor Author

Thanks, @koskimas and thanks for the suggestions!

Since I may go bring this up over at pg, is there any insight you can provide based on my examples and your knowledge of what's going on under the hood query-wise that might point to specific things that bring out the different behaviors?

For example:
Why might a graph insert produce a parent with a Date, while its children have string types? If you cannot, or don't want to spend the time, that's totally understandable!

Thanks again much for all of your hard work on Objection. It's a pleasure to work with!

@newhouse
Copy link
Contributor Author

Oh, nevermind, I think I understand it a bit more and it's still something that's definitely in pg land, and isn't a bug there, either. It's simply a side-effect of pgs "expected" behavior combined with particular things that Objection/knex do/don't do in each scenario (namely using the value returned from the DB vs the value provided by the User/code), right?

@koskimas
Copy link
Collaborator

koskimas commented Dec 16, 2017

I'm pretty sure pg always returns dates as Date instances by default, but you can send pretty much anything to it and it will convert it correctly. If you insert stuff using objection, it doesn't fetch the data from pg (except the primary key) and simply returns the object you inserted. Maybe that causes the inconsistent types? Maybe you can use returning('*') or ***AndFetch variants of the queries?

@IlyaSemenov
Copy link
Contributor

One doesn't need to hardcode the constants, they are already provided by pg-types:

import { DateTime } from 'luxon'
import pg from 'pg'

pg.types.setTypeParser(
	pg.types.builtins.TIMESTAMPTZ,
	val => (val === null ? null : DateTime.fromSQL(val)),
)

@javiertury
Copy link

javiertury commented Dec 5, 2019

Dates can also be transformed to objects.

class ExtendedModel extends Model {

  static get dateTimeProps() {
    const attributes = [];
    const props = Model.jsonSchema.properties || {};

    for (const propName of Object.keys(props)) {
      const prop = props[propName];

      if (prop.type === 'string' && prop.format === 'date-time') {
        attributes.push(propName);
      }
    }
    return attributes;
  }

  $afterInsert(ctx) {
    if (super.$afterInsert) super.$afterInsert(ctx);

    const attrs = this.constructor.dateTimeProps;
    for (const attr of attrs) {
      this[attr] = this[attr] && new Date(this[attr]);
    }
  }
};

However this doesn't work for Model.fromJson. Is there any hook to modify the instance just after it is created? If tried to extend the constructor, but this was still empty.

@koskimas
Copy link
Collaborator

koskimas commented Dec 9, 2019

@javiertury $parseDatabaseJson is the hook you are looking for.

https://vincit.github.io/objection.js/guide/hooks.html#model-data-lifecycle-hooks

@javiertury
Copy link

javiertury commented Dec 21, 2019

@koskimas unfortunately that suggestion doesn't work.

Insert sometimes returns an incomplete object(id only) which is the only thing $parseDatabaseJson can work with. Whereas $afterInsert gives you access to all of the instance fields.

Also, Model.fromJson(data) fails under any alternative. If you use a date object, validation fails. If you use a string in the input JSON, you cannot transform the property to a date object later on. The reason is that $afterValidate() cannot modify instance properties.

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

4 participants