Don't break @:skip fields when resetting cached & unlocked objects

[jonasmalacofilho]

Unlike in Neko, in PHP a field can no longer be accessed after it has been removed with Reflect.deleteField. Use Reflect.setField instead.

Not bothering with clearing the fields of cache items was considered, but would make the behavior @:skip fields less predictable, besides potentially breaking user code.

Related: HaxeFoundation/haxe#8016 ("Possible PHP7 haxe.Serializer bug") Closes: #34 ("Synchronizing the fields")

[filt3rek]

Jonas,

First, thanks again to take time to test my case and for your answer. Then, sorry for Sqlite, I've never used that, I'll take care in the future.

With your fix, it works fine now only because it's a getter that check if my field is null and set it. So when it's nulled by the Manager, it's recreated of course...

I've written a smaller sample which shows that your fix (and the old code lines) doesn't work in a special case.

Here is the Main file :

import haxe.Serializer;

import sys.db.TableCreate;
import sys.db.Sqlite;
import sys.db.Manager;
import sys.db.Types;

@:table( "tag" )
class Tag extends sys.db.Object {
    public var id   : SId;
    public var name : SString<255>; 
        @:skip
    public var foo  : String;

    function hxSerialize( s : Serializer ){
        trace( foo );
        s.serialize( id );
    }

    public function new(){
        super();
    }
}

class Main {

    static function main() {
        Serializer.USE_CACHE    = true;

        Manager.cnx = Sqlite.open(":memory:");
        Manager.initialize();
        Manager.cleanup();

        TableCreate.create(Tag.manager);

        var tag = new Tag();
        tag.name = "tag_name";
        tag.insert();

        trace( 0 );
        var tag = Tag.manager.get( 1, false );
            tag.foo = "bla";
        trace( 1 );
        Serializer.run( tag );
        trace( 2 );
        tag.lock();
        trace( 3 );
        Serializer.run( tag );
        trace( 4 );
    }
}

And I've just added some traces in the Manager too like that :

for ( f in Reflect.fields(c) ){
    trace( c, f, Reflect.field( c, f ) );
    Reflect.setField(c, f, null);
}

This are the traces output :

src/Main.hx:33: 0
src/Main.hx:37: 1
src/Main.hx:14: bla  <---------------- OK
src/Main.hx:39: 2
sys/db/Manager.hx:673: tag#1, foo, bla  <------------------- OK
sys/db/Manager.hx:673: tag#1, id, 1
sys/db/Manager.hx:673: tag#null, name, Peace spreader
sys/db/Manager.hx:673: tag#null, __cache__, { id : 1, name : tag_name }
sys/db/Manager.hx:673: tag#null, _lock, false
sys/db/Manager.hx:673: tag#null, _manager, [object sys.db.Manager]
sys/db/Manager.hx:693: tag#1, foo, null  <------------------ NULL
sys/db/Manager.hx:693: tag#1, id, 1
sys/db/Manager.hx:693: tag#1, name, Peace spreader
sys/db/Manager.hx:693: tag#1, __cache__, { id : 1, name : Peace spreader }
sys/db/Manager.hx:693: tag#1, _lock, true
sys/db/Manager.hx:693: tag#1, _manager, [object sys.db.Manager]
src/Main.hx:41: 3
src/Main.hx:14: null  <------------------- NULL
src/Main.hx:43: 4

As you can see it's the same object, I just locked it between 2 serializations and it lost its foo field.

[filt3rek]

In fact, we can reproduce this "bug" like that :

import sys.db.Manager;
import sys.db.Types;
import sys.db.TableCreate;
import sys.db.Sqlite;

@:table( "tag" )
class Tag extends sys.db.Object{
    public var id   : SId;
    public var name : SString<255>; 
        @:skip
    public var foo  : String;

    public function new(){
        super();
    }
}

class Main {

    static function main() {
        Manager.cnx = Sqlite.open(":memory:");
        Manager.initialize();
        Manager.cleanup();

        TableCreate.create(Tag.manager);

        var tag = new Tag();
        tag.name = "tag_name";
        tag.insert();

        var tag = Tag.manager.get( 1, false );
        tag.foo = "bla";
        trace( tag.foo );
        tag.lock();
        trace( tag.foo );
    }
}

But for my needs I used the serializer because I do things with a @:skip ed field inside hxSerialize function...

[filt3rek]

@jonasmalacofilho Hej, do you think is it normal please ?

[jonasmalacofilho]

@filt3rek I'm still considering what to do here...

Your last example showed an important point: an apparently "simple" lock() call can cause the field to be cleared.

At the same time, we still need to consider the issue the current code tries to solve: what happens if the object changes on the DB and then gets reloaded by a later SPOD call?

I think that simply keeping the field would be very bad (and worse than the current behavior): the field would still get out of sync, but there would no longer be a way for the owner of the object to test that had happened.

So my question to you is: in your real world use case, can you simply replace the physical field by a property, or add one to wrap the field and handle the null case (like you did in the examples)?

(I also plan to search for users of @:skip feature on GitHub and see what they're using it for...)

[filt3rek]

@jonasmalacofilho thanks for your answer, I didn't know you have taken in account my examples. What about me is not very important because for me I almost always find a way to do what I want but it was in order to have a better behaviour for the lib for the future for all of us :) I don't find logical to reset the entire object when doing a lock. Maybe we should have a kind of public method like "synchronize" or something like that which do the stuff explicitly ? And even here I still don't find logical to touch fields that are not SPOD fields. When synchronizing data, we should only touch the fields that are used in DB not these with @:skip. It's my opinion, maybe I miss something. And here in my concrete application I use a getter as you have seen get_xfields. Thanks anyway for your work

[bablukid]

Hello, I agree with @jonasmalacofilho ... locking an object makes a request like "SELECT * FROM MyTable FOR UPDATE" which will resync all the fields from the database.

Then this seems logical to also clear the @:skip fields in that case, because the values of this @:skip fields will - in most cases- be related to the other regular fields values.

Don't forget that SPOD has been designed to be used with InnoDB transactions.

[filt3rek]

Yeah all the fields from database, all is said, not these which are not used in DB. I disagree with this kind of behaviour, it's not logical for me, but if everybody is ok with that, at least we should say in the doc that locking an object make it completly reset. Where is it told that SPOD has to be used with InnoDB transaction ? It works with InnoDB but also with MyISAM. When used with InnoDB, you can define cascading behaviour for example. But I've never seen that it must be used with transactions. And if as you say, in most cases, @:skip fields are related to the regular fields values, if we synchronize only these regular fields, the related @:skip fields will point to the new synchronized values. I don't see any problems here. What is no logical at all is to have the same object that loses its @:skipp fields values when doing a lock, the same object not a new one ! So maybe add another public method called "reset" (not "synchronize" as I suggested befoire because it's still confusing), would be better solution.

[bablukid]

Well you're right, its not written in the Readme.md that it should be used with InnoDB transactions, but using "FOR UPDATE" statements makes no sense outside InnoDB transactions ( https://dev.mysql.com/doc/refman/8.0/en/innodb-locking-reads.html ) ...

I guess it has no effect for a MyIsam table. There is a workaround for your case : you can always get objects with a lock since the beginning then you won't have any field cleared later. ( User.manager.get(1,true) )

Maybe we should talk about this in the documentation.

[filt3rek]

Yes for InnoDB constraints like cascading, etc., when used with InnoDB it works like in MySQL. But for simple relations for example, you can also use MyISAM, it will bring you the relations in SPOD, not simply the "id". I know there are workarounds, I always succes in what I want to do (yeaaaah :sunglasses: :laughing: ) but I'm talking for other users, to be more clear about the behaviour.