meteor-astronomy icon indicating copy to clipboard operation
meteor-astronomy copied to clipboard

Model validation always fails when having duplicate classes

Open dmitry-salnikov opened this issue 6 years ago • 3 comments

Astronomy doesn't handle the attempt to Class.create() with the name of already existing class. Example: we have the next three files in project:

// file: MyModel.js
export const MyModel = Class.create({
  name: 'MyModel',
  fields: {
    foo: String
  }
});

// file: OtherModel.js
const MyModel = Class.create({
  name: 'MyModel',
  fields: {
    bar: String
  }
});

export const OtherModel = Class.create({
  name: 'MyModel',
  fields: {
    myModel: MyModel,
    default: function() { return new MyModel(); }
  }
});

// file: app.js
import { OtherModel } from 'OtherModel';

let otherModel = new OtherModel();
otherModel.myModel.bar = "123";
myModel.validate();

The validate() call on the last line will throw Error saying "myModel.bar has to be MyModel".

Looking at the Astronomy source /lib/core/class.js:140

let Class = this.classes[className] = class Class extends this {};

it's pretty clear that in such case the second declared class will simply overwrite the previous one.

Could you please take a look?

Thanks

dmitry-salnikov avatar Apr 20 '18 00:04 dmitry-salnikov

One thing first, always check your examples before posting. You have several errors and first I had to figure out what you wanted to achieve, to actually start thinking about the answer to your problem. Don't give me extra work and always prepare reproduction repository. Preparing such a reproduction is a matter of a few minutes.

Why would anyone want to override class? If you do so, then you're probably doing something wrong. In the real-life application, no one would risk introducing two classes with the same name, because it's causing more confusion. So your problem can probably be solved just by renaming class.

However, after fixing your example everything works as expected so it's not a problem with Astronomy but with your code. Here is fixed code:

// MyModel.js
import { Class } from "meteor/jagi:astronomy";

export const MyModel = Class.create({
  name: "MyModel",
  fields: {
    foo: String
  }
});
// OtherModel.js
import { Class } from "meteor/jagi:astronomy";

const MyModel = Class.create({
  name: "MyModel",
  fields: {
    bar: String
  }
});

export const OtherModel = Class.create({
  name: "OtherModel",
  fields: {
    myModel: {
      type: MyModel,
      default() {
        return new MyModel();
      }
    }
  }
});
// app.js
import { MyModel } from "./MyModel"; // Just to demonstrate that it's being imported
import { OtherModel } from "./OtherModel";

const otherModel = new OtherModel();
otherModel.myModel.bar = "123";
otherModel.validate(); // No error thrown

You can also made this ugly check to make sure that indeed "otherModel.myModel.bar" requires string:

otherModel.constructor.schema.fields.myModel.type.class.schema.fields.bar.type.name // Returns "String"

lukejagodzinski avatar Apr 20 '18 09:04 lukejagodzinski

@lukejagodzinski I also have such issue. Of course I didn't have intent to override class with another class with same name. It was copy-paste mistake but your framework doesn't have check to prevent such kind of mistakes. It will be really good if you add it in next versions.

DarthRumata avatar Apr 20 '18 09:04 DarthRumata

@DarthRumata indeed such a check would be a good idea to do just to prevent accidental overrides. I will fix that in the next release.

lukejagodzinski avatar Apr 20 '18 10:04 lukejagodzinski