Astronomy 2.3.0 - Casting values

[lukejagodzinski]

From now I will be releasing one feature per minor version (0.X.0). In Astronomy 2.3.0 I'm going to implement values casting feature. It will allow developers to define casting function for each schema field and perform automatic casting of values coming from form or other data sources.

const User = Class.create({
  name: 'User',
  collection: Users,
  fields: {
    age: {
      type: Number,
      // It's default casting function but it's shown here as an example.
      cast(value) {
        return Number(value);
      }
    }
  }
});

It will automatically cast values like "123" to 123 for fields of the Number type. You will be able to cast values using the set() method.

const user = new User();
user.set(formData, {
  cast: true
});

I also consider implementing automatic casting on save.

const Post = Class.create({
  name: 'Post',
  collection: Posts,
  fields: {
    publishedAt: {
      type: Date,
      // Automatically cast values on the `save()` or `validate()` method call.
      autoCast: true
    }
  }
});

as proposed by @s7dhansh in this #373 issue.

I'm open to discussion about this feature.

If you want to support development of this feature, you can donate on my Patreon page or just create PR :). Thanks!

[lukejagodzinski]

@s7dhansh @talha-asad I'm working on casting feature and there are several issues. However, right now I'm facing a problem with default String casting function. It looks like:

cast(value) {
  if (isString(value)) {
    return value;
  }
  return String(value);
}

So conversion will look as follow:

• 123 -> "123"
• true -> "true"
• new Date() -> "Fri Oct 28 2016 02:20:55 GMT+0200 (CEST)"
• new Object() -> "[object Object]"

and as 2 first conversions looks pretty good. Then the last 2 are probably not the best - especially the last one. It might look like not big deal. But accidental wrong assignment might result in properly validated document stored in collection.

I was thinking about turning on casting by default but right now I'm not so sure. What do you think about this problem? How would you solve it?

[s7dhansh]

That IS a problematic case. I can't think of anything other than a hard-coded exception for objects, during conversion to String. In such cases the user must manually cast his object (any object, including date, excluding null) before setting/saving.

[lukejagodzinski]

@s7dhansh There are several things to take into account:

• When talking about casting, we probably mean setting values coming from the form.
• Values coming from form are mostly plain values like strings, as long as you don't call date.valueAsDate on a date input field.
• We should probably exclude casting objects as you said. So the casting functions should look like:

// String
cast(value) {
  if (isString(value) || isObject(value)) {
    return value;
  }
  return String(value);
}
// Number
cast(value) {
  if (isNumber(value) || isObject(value)) {
    return value;
  }
  return Number(value);
}
// Boolean
cast(value) {
  if (isBoolean(value) || isObject(value)) {
    return value;
  }
  else if (isString(value) && value.toLowerCase() === 'false' || value === '0') {
    return false;
  }
  return Boolean(value);
}
// Date
cast(value) {
  if (isNumber(value)) {
    return new Date(value);
  }
  else if (isString(value)) {
    if (/^[0-9]+$/.test(value)) {
      return new Date(parseInt(value, 10));
    }
    else {
      const time = Date.parse(value);
      if (!isNaN(time)) {
        return new Date(time);
      }
    }
  }
  return value;
}

Such casting functions would only cast plain values. Of course developer can still pass custom casting function and cast object if he/she wants to.

The other problem is automatic casting. At the beginning I was agains, later I was for it and right now I'm one more time against :). My current view is: If I would turn on automatic casting by default it could probably break some applications. In deed it would be nice to have automatic casting like this:

doc.set(formData);

But adding one extra line of code shouldn't be a big deal for most people.

doc.set(formData, {
  cast: true
});

Moreover, if you would like to cast values on validation or save operation, you have to explicitly tell Astronomy to do so.

doc.validate({
  cast: true
});
doc.save({
  cast: true
});

It's a common interface that developers will quickly get used to. There is also a problem with how to treat nested classes. If we have a field address of the Address class type. It should always cast address to Address instance even if cast is set to false. Which looks like inconsistency. So that's why I'm still opened for discussion as I might not see the whole picture.

[s7dhansh]

@jagi Yes excluding object seems good. For Number, I don't think its required though. Those extra timestamp conversion lines are great, I wouldn't have thought of them in the first go.

Automatic casting is a convenience, and since anyways casting is not happening on assignment (if I remember the last discussion correctly), using cast: true is not so bad.

But considering your consistency point, I would suggest: We auto cast, but also have cast: false option in the Class (just like secure: false), which will disable autocast, and would help those marginal applications which may get affected.

[joefru]

When casting Object to String, why not just use JSON representation? Could you use something like JSON.stringify()?

[s7dhansh]

@joefru IMO, it makes not much sense. Why would someone want to store an Object as a String? And if someone does, he should write his own function. Giving an error, seems to be the correct approach, so that accidental saves with bad data do not happen.

[joefru]

JSON.stringify() would work for casting any type to String. I can't predict why developers would require this functionality, but throwing an error seems harsh when there is a simple, elegant solution for delivering the functionality.

[lukejagodzinski]

@joefru There were situations when developers accidentally stored objects as JSON strings and they were surprised that Astronomy didn't throw an error. So I think it's quite good solution to just throw an error. In JavaScript you could actually cast any value to some other plain value but it's not always intended. In most cases it's a result of a bug in a code. In most cases you want to cast plain value to another plain value like: String to Number, or String to Boolean.

@s7dhansh Ok so I will try solution without auto casting. However I will consider adding the top level cast option to the schema, so all field will be casted automatically if set. I will give more details as feature is implemented, so you can test it out and give feedback. I've already implemented most of the features. I will try doing something tomorrow.

[lukejagodzinski]

Ok, feature implemented and you can test it out by cloning the feature/casting branch into your local packages directory. Here are two test files for the cast and merge options, so you can see how to use them: https://github.com/jagi/meteor-astronomy/blob/feature/casting/test/modules/fields/cast.js https://github.com/jagi/meteor-astronomy/blob/feature/casting/test/modules/fields/merge.js I would be glad if you could clone the branch and check if this feature works for you and if you would change or add some feature.

[s7dhansh]

Awesome! Just checked the tests and code, looks good. Will try to test it over the weekend.

[lukejagodzinski]

Great! Waiting for feedback :)

[lukejagodzinski]

I've also added option for casting values on save.

var user = User.findOne();
user.firstName = 123;
user.save(); // Throws validation error
user.save({cast: true}); // Casts fields and does not throw any error

[lukejagodzinski]

Astronomy 2.3 released

[zachariahtimothy]

Just converted all my castings from afterInit event to cast and working GREAT! Thank you so much for the fast turnaround and even documentation on this. LOVING Astronomy over SimpleSchema (no offense to that great library).

[lukejagodzinski]

@zachariahtimothy good to hear that you like it :-). New features to come soon :-)

[s7dhansh]

@jagi sorry i was not able to take out enough time to test it. still stuck, will do an upgrade and let you know whenever i can. but from the feedback, it looks its working great. kudos! @zachariahtimothy i am a bit curious, why were you doing casts in afterInit?

[talha-asad]

@jagi Thanks alot for implementing this. Sorry I have not been very active lately, been a bit over occupied at work these days. Will test this out and get back to you.

[lukejagodzinski]

@s7dhansh @talha-asad no problem :) just take your time and test it in a free time :). I just wanted to release it. But if there is any bug I can always release bugfix version :). Waiting for feedback :)

[talha-asad]

I upgraded to the latest, and found this isn't working anymore Geofence.find({}, {fields: {_id: 1}}).fetch()

It works on the previous version. This is on the client-side. Maybe this has something to do with Mini mongo being upgraded as well. Works fine if I use Meteor default collection class.

[lukejagodzinski]

@talha-asad Hmmm yes it might be a bug in a code. I will take a look at it.

[talha-asad]

It's very weird, it happens only with that collection at one place. I have no idea why it is generating an error.

I am on Meteor 1.4.2.1 and astronomy 2.3.0

[lukejagodzinski]

@talha-asad I've found a bug. I'm just fixing it. A new version should be available in several minutes.

[lukejagodzinski]

@talha-asad Ok fixed in 2.3.2 version

[talha-asad]

That was quick 👍 Was looking at the last few commits and saw the commit message "renaming helpers back to methods". Should I make this change back in the schema, or is it internal only?

[lukejagodzinski]

@talha-asad it's internal only :) so you don't have to change anything