SierraSoftworks / Iridium

A high performance MongoDB ORM for Node.js
http://sierrasoftworks.github.io/Iridium/
Other
570 stars 25 forks source link

Updating Instance Properties Doesn't Save #110

Open CatGuardian opened 6 years ago

CatGuardian commented 6 years ago

Installed version: 8.0.0-alpha.7

The problem is that interacting with an Instance that is not created using the corresponding Model doesn't bind the Instance's properties and doesn't save them.

For example:

   // User extends Instance
  // db is an instance of a subclass of Core.
  // db.Users is a Model

   const user = new User(db.Users, {
      name: 'First User',
    });

    user.name = 'changed name before saved';

    const savedUser = await user.save(); // Actually ends up saving { name: 'First User' } to database.

    const retrievedUser = await db.Users.get();

    console.log(`_id: ${retrievedUser._id}   name: ${retrievedUser.name}`);
    // _id: 5a4ac39959d5d83858a0910d   name: First User

But it should end up saving the name as 'changed name before saved'

Right?

notheotherben commented 6 years ago

Hi @CatGuardian,

You are absolutely correct, however what's happening is that you're using the raw, unprocessed, model before it has been prepared by Iridium - leading to the issues you're seeing.

Before I go into why this is happening, the fix is to replace new User(...) with new db.Users.Instance(...) at which point everything should work correctly (I'll push a test case for this to the repository for you to look at if you are interested).

As for why this happens, you'll perhaps get some insight by looking at the ModelSpecificInstance.ts file. Essentially it creates a pseudo class for a model which proceeds to map each of the schema properties, as top-level object fields, to the underlying document representation for a given Iridium.Instance. This approach enables us to do lazy evaluation of transforms while maintaining V8's virtual class support and avoiding the need for de-optimization which would otherwise be present if you built this dynamically on each request/for each document.

Without this model specific instance proxy, you don't have the field mapping - meaning that your assignment to user.name creates a new name field on your user instance, without actually updating the underlying user._modified.name field or running any transforms. Similarly, if you attempted to access user.name prior to its assignment, it would not retrieve First User, instead returning undefined.

I hope that clears things up for you, please feel free to ask if you have any other questions.

CatGuardian commented 6 years ago

Thank you for the response.

Yes that makes sense and clears things up for me.

The thing is, is that I am trying to have my database layer in its own separate npm package. That way any of my projects can add a dependency for my 'my-db' package and start using it. So ideally I was hoping to only export the various Instance classes so I could import them individually as each consumer file needed instead of getting the entire database because certain files won't need to concern themselves with certain Instances/Models.

It does seem a bit awkward to have to go through a roundabout way to instantiate an Instance class when we already define the Instance class as its own class. Meaning I think programmers would much rather do 'new User(...)' than 'new db.Users.Instance(...)'. But if it simply isn't possible to do what I'm asking then I guess I will have to adapt.

Maybe in theory I can do something like this:

class MyDatabase extends Core {
    Houses = new Model<HouseDocument, House>(this, House);
}

var myDb = new MyDatabase({ database: 'houses_test' });

export const HouseModel = myDb.Houses;

And just interact with HouseModel to create proper instances.

And your test case did help. Thank you so much for your time.

It would be nice to see something addressing my use case I asked about up front in the ReadMe portion of this repo. Because the only way mentioned to create a record in the DB is by using the myDb.Houses.insert(...) method.