Transform Layer Issues

[jsdream]

Today I've tried to apply Transform decorator to my User model.

@Collection("users")
@Index({username: 1}, {unique: true})
@Index({email: 1}, {unique: true})
@Transform((document) => {
    console.log("From DB");
    return document;
}, (document, property, model) => {
    console.log("To DB");
    return document;
})
export class User extends Instance<IUserDocument, User> implements IUserDocument { ... }

But this resulted in the following TS error:

TS2345: Argument of type 'typeof User' is not assignable to parameter of type 'Instance<any, any>'.

Then I took a look at the Decorators.d.ts file and found Transform decorator factory function declaration, which is following:

export declare function Transform(fromDB: (value: any, property: string, model: Model<any, any>) => any, toDB: (value: any, property: string, model: Model<any, any>) => any): (target: Instance<any, any>, property?: string) => void;

it returns (target: Instance<any, any>, property?: string) => void;.
I tried to replace Instance<any, any> with InstanceImplementation<any, any> and that fixed the TS error. However, I doubt that is a correct solution.

Although, TS error has gone after I changed target type, added transform functions are not being executed.

Then I also tried to add transform functions via transforms static property, like this:

export class User extends Instance<IUserDocument, User> implements IUserDocument {
    static transforms = {
        $document: {
            fromDB: (document) => {
                console.log("From DB");
                return document;
            },
            toDB: (document) => {
                console.log("To DB");
                return document;
            }
        }
    };
}

And it started partially working, partially is because only fromDB function is being executed as expected. toDB is triggered by insert() method, but update() method doesn't trigger toDB.

A bit about my environment:
node 7.6.0
typescript 2.2.1

Am I missing something or there is a bug in Iridium? Can someone please confirm or disconfirm the issues I've described above?

[notheotherben]

Hi Vladyslav,
I'll look into this and get back to you.

One note, the triggering of transforms only occurs on insert() and save() operations, update() doesn't trigger it because there's no good mapping from an update query (like { $set: { x: 1 } }) to a transform function unfortunately. It's probably worth me updating the documentation to make that clear.

[jsdream]

Hi Benjamin and thanks for your reply.

Looking forward to your next response.
Let me know if you need any more info from me.

And thanks for explanation on update() method. Yeah, I also think it is worth to update the documentation to make it clear for people like me ;-)

[notheotherben]

Hi Vladyslav,

I've just fixed the type definition issue - an oversight on my behalf which was lacking tests to ensure it worked correctly. Your approach is pretty much spot on correct, however I've made the fix in a backwards-compatible manner.

Please let me know if the document level transform isn't working for you still, I've got some tests which should be checking that aspect but if they aren't I'll need to restructure them to catch whatever is causing it to break for you.

I'll also get round to updating the documentation as soon as we've sorted out the functional issues.

[jsdream]

Hi Benjamin!

Thank you for such a quick fix of type definition issue.

However, I'm still experiencing issues with transform functions are not being registered.
I did a little of debugging and found that transform functions are not being added to _transforms property of Model because of this statement.
If I replace target.constructor with just target it starts working properly.
Not sure why target.constructor is used here. Hope you can take a look and shed some light on the issue.

I don't mind doing a PR if you can guide me.

Regarding the tests: maybe it is worth to do some document altering here and then check in tests if document has been actually altered? So we can ensure that transform functions have been really executed. Or at least add chai.expect(Test.transforms).to.exist.and.have.property("$document").with.property("toDB").which.is.a("function"); here.

[notheotherben]

Ah, well spotted. The .constructor is used to enable the decorator to be applied to properties (which receive an instance's prototype as the target rather than the constructor). Since the transforms property is actually on the constructor, we need to de-reference that. It looks like the test I was doing target.constructor || target was just way too naive in that it assumed the constructor would be null|undefined for classes, which it obviously won't because Function has a prototype as well...

I've since added tests for this (the latter option you suggested) and have made the fixes in v7.1.6. If you wouldn't mind taking a peek and letting me know if it all works for you, I'd appreciate it.

Sorry about that oversight and thanks so much for helping me debug and fix this - it certainly shouldn't have slipped through the cracks, but alas that seems to be the nature of things. I can only hope it didn't impact you too heavily time wise and please feel free to get in touch if you run into anything else which isn't behaving as you expect it to.

[jsdream]

Thank you very much for so quick fix. The Transform decorator works just fine both on classes and on properties now. You are cool, Benjamin!

However, I guess I have spotted another problem while testing Transform decorator on a property.
I've noticed that while toDB is being called, the fromDB isn't. I again did some debugging and found that problem is here - the options argument of transformFromDB method is { document: true }, but it should be { document: true, properties: true } in order execute transform function on a property. Is this intentional or it's just another tiny bug?

[notheotherben]

Hi Vladyslav, glad to hear it's working correctly now and no worries.

The property transforms are actually lazily evaluated, somewhat differently to the $document transform, you'll see the implementation here. If you want to confirm whether they're working, you'll need to read and write the requisite property. Please let me know if you run into any issues with this.

[jsdream]

Hi Benjamin!

Thank you once more for explanation.

I think it is worth to put this in documentation? Because at the moment documentation says the following:

Property transforms work the same, except that they are presented with, and expected to return, the value of a single top-level property.

But it turns out that actually they do not work the same.

[notheotherben]

Hi Vladyslav,

Can you please have a glance over the updated README and let me know if the changes there (under Transform Gotchas) provide enough information to help someone unfamiliar with the implementation navigate that functionality effectively? I'm, unfortunately, not in a great position to tell - having spent a lot of time working on the codebase and using it on other projects.

[jsdream]

Awesome, Benjamin!

Looks very descriptive to me.

Thank you for all your support. I'm enjoying using your library.
I think this issue can be closed now.

[notheotherben]

Cool stuff, not a problem at all and glad you're enjoying it. Feel free to ask if you run into anything else.