sylvainpolletvillard / ObjectModel

Strong Dynamically Typed Object Modeling for JavaScript
http://objectmodel.js.org
MIT License
467 stars 30 forks source link

OM with private prop cannot be nested in another OM #87

Closed gotjoshua closed 6 years ago

gotjoshua commented 6 years ago

https://codepen.io/gotjoshua/pen/PBmyEQ?editors=1111

class OM extends ObjectModel({
  _privString: [String],
  pubNum: [Number]
}) {}
class VM extends ObjectModel({
  _id: [String],
  om: [OM]
}) {}

let normalOM = new OM({
  _privString: "only for me",
  pubNum: 42
});

console.log(normalOM);

let nestedVM =  new VM({
  om: normalOM
})
// Uncaught TypeError: cannot access to private property _privString
console.log(nestedVM); // does not happen
sylvainpolletvillard commented 6 years ago

Okay, the last minor fix I pushed actually prevent parent model definition checker to check if a submodel private property is valid regarding its definition.

I think we need to define clear rules regarding what can and what cannot access private variables, which is something I probably underthought.

Read permission for private props :

Write permission for private props :

Write permission for constant props:

Fix incoming, stay tuned

sylvainpolletvillard commented 6 years ago

Should be fixed in 3.5.4 published 10 minutes ago, can you confirm ?

gotjoshua commented 6 years ago

i can give it a try soon... but I have a more fundamental question: Why does the parent model need to access the private variables of the already cast and validated child model? couldn't instanceof cover that check?

sylvainpolletvillard commented 6 years ago

It could, that could be a possible optimization, but instanceof can be easily tricked:

A = Model({ a: String }),
B = Model({ b: String }),

a = A({ a: "test" });
Object.setPrototypeOf(a, B.prototype);

a instanceof B // true !

So validating the whole definition tree is considered safer. And this is something we need to do anyway in case of duck typing. Maybe I could add an optional flag that makes the submodel check rely on instanceof , but I would prefer to keep things simple and avoid adding new options if possible.

gotjoshua commented 6 years ago

Hmmm... still thinking about instanceof...

For now I can confirm that 3.5.4 seems to fix the bug (both in my more complex app and in the above linked codepen)! nice work!

sylvainpolletvillard commented 6 years ago

Another example. If we would only rely on instanceof, then this would be considered valid:

new VM({
  om: { __proto__: OM.prototype }
})
gotjoshua commented 6 years ago

about instanceof... i agree an option does not sound cozy, and the most recent example (albeit seemingly malicious) feels a bit too lenient.

Perhaps instanceof is not correct, but if the nested model is an ObjectModel(or extension of) then I don't get why it would need to be externally validated (property by property) upon nesting. It exists, and therefore was pre-validated, right? For maximum appropriate strictness, i could imagine to first try: NestedModelWithPrivates.test(supposedModelInstance)... if that fails then duck typing attempts could follow, but if it succeeds, it is clear that the duck is already quacking.

What do you think?

sylvainpolletvillard commented 6 years ago

Actually, model.test() just calls validate internally, checking the whole definition tree and assertions, and return false if it catched some type error. So this is already how it works at the moment. Duck typing goes through the same process and calls validate during the model instanciation. The idea behind this was also to prevent having a different behaviour depending on whether you are using duck typing or not. Otherwise it would be a mess.

btw, sorry for my english mistakes, it's quite complicated to discuss on such complex issues when English is not my native language.

gotjoshua commented 6 years ago

Oh, Ok. I assumed that the model would be permitted to test/validate itself in a way that would not seem like a violation of privacy... but i blatently have a limited understanding of the inner workings of ObjectModel and the various scopes at play.

btw, no apology needed! Honestly, I already began to wonder if/why you are bilingual or how you learned such sharp precise English... cheers, and kudos...

sylvainpolletvillard commented 6 years ago

I assumed that the model would be permitted to test/validate itself in a way that would not seem like a violation of privacy

yes, that's exactly what I fixed in v3.5.4, I grant temporary private access during the definition checking step.