Issue with observer self

[AdamSGit]

This pull request address issue discussed in this forum thread : https://fuelphp.com/forums/discussion/15221/issue-with-observer-self

This was happening because the after_load observer event was triggered in the Model consutructor for non-new models, before relations are all set up.

After some analysis of the code, the way I fixed it was :

• Remove it from the constructor
• Add a method that would trigger it, and call the method recursively in all the relations
• Call this method at the end of the get() method of the Orm\Query class.

I did some tests, didn't find any issue with this behavior. Let me know if anything more is needed, or if I forgot some use cases.

Cheers,

[WanWizard]

I've applied the patch locally in a few apps, and I have issues with Observer_Typing, which is called multiple times on a model object.

I assume this is because the recursive code assumes all model objects are newly loaded, which is not necessarily the case. If an object from cache used, it runs the observers twice.

[WanWizard]

I can not merge this, as I think you've solved the problem the wrong way, it is not caused by the location from where the after_load observer is called.

It is in the constructor because you want to make sure it is called only once, after the object has been created, no matter what creates the object.

Your solution causes observers to be called multiple times because you simply recurse, which will also run the observers on objects in cache, on which the observers have already run.

The problem you have in Observer_Self is that you try to reference data outside the scope of the current model object, and parent objects are created before child objects. To deal with that probably means rewriting the hydration process, which is not going to be easy.

[AdamSGit]

I understand. If the issue is only for cached objects, why not adding a private property that would keep track that the object had this observer observed already and not trigger it more than once?

[WanWizard]

That is still a workaround, and complex too, as you have to keep track of all observers for all possible triggers (Observer_Typing is called after_load and before_save for example).

[AdamSGit]

You are right that the best thing to do would be refactoring hydration. But it's a complex and tricky task, and maybe not the top priority. For the solution we are discussing, only the after_load event is concerned by the issue, and it only need to be observed once in a model lifecycle. So yes, it's a workaround, but it doesn't seem that complex. There is no need to keep track of differents observers, as the $model->observe('after_load') would set a boolean property to true. With this behavior, cached models which already observed the after_load event won't do it again. Do you see use cases where this could cause issues ?

[WanWizard]

It won't work, because it would also block legitimate calls. For example, is_changed() calls "before_save" before comparing, and "after_load" after comparing, to make sure the comparison happens on "database row" formatted data, and not transposed data.

I've made an attempt at rewriting hydration. Can you try with this query.php?

edit: forget this one, doesn't work...

[AdamSGit]

In term of architecture, what the solution would be ? Moving the hydration process into the model constructor, with related models being recursively hydrated at the same time ? Or more like dispatching an event to the model when full hydration is complete ? Let's agree on the direction and I can give it a try.

[WanWizard]

No wories, working on it, nearly done... ;-)

[AdamSGit]

Nice to hear.

[WanWizard]

Second attempt: https://github.com/fuel/orm/commit/da2bb0ea6aa9af01a02688d73f1a4f3ddae398d5

[AdamSGit]

Seems good. I will take the time to read the code and test it more later as I'm at work, but I quickly tested it on some test app with two nested relation. This is working on the original model with the first relation, but the first relation which access the second in it's after_laod still fetch it before hydration. Good job anyway, thanks for your reactivity, as I said I will dive deeper in this as soon as I have time.

[WanWizard]

Can you post the query you run, and what you do in the observer, as I can't reproduce that. Child objects are now created from the constructor of their parent, before observers run.

[AdamSGit]

Sure. The hierachy of relations is Model_Director -> Model_Movie -> Model_Actor (both relation are hasmany, and in my table I have 1 director row, 1 movie row and 100 actors, all related to the same movie). The event_after_load of Director simply access $this->movies, and the one of Movie access $this->actors.

The first query is this one :

SELECT `t0`.`id` AS `t0_c0`, `t0`.`name` AS `t0_c1`, `t0`.`created_at` AS `t0_c2`, `t0`.`updated_at` AS `t0_c3`, `t1`.`id` AS `t1_c0`, `t1`.`director_id` AS `t1_c1`, `t1`.`title` AS `t1_c2`, `t1`.`created_at` AS `t1_c3`, `t1`.`updated_at` AS `t1_c4`, `t2`.`id` AS `t2_c0`, `t2`.`name` AS `t2_c1`, `t2`.`movie_id` AS `t2_c2`, `t2`.`created_at` AS `t2_c3`, `t2`.`updated_at` AS `t2_c4` FROM `directors` AS `t0` LEFT JOIN `movies` AS `t1` ON (`t0`.`id` = `t1`.`director_id`) LEFT JOIN `actors` AS `t2` ON (`t1`.`id` = `t2`.`movie_id`) WHERE `t0`.`id` = 1

Followed with 99 others queries of this one :

SELECT `t0`.`id` AS `t0_c0`, `t0`.`name` AS `t0_c1`, `t0`.`movie_id` AS `t0_c2`, `t0`.`created_at` AS `t0_c3`, `t0`.`updated_at` AS `t0_c4` FROM `actors` AS `t0` WHERE `t0`.`movie_id` = 2 LIMIT 1

The fact that there are 100 queries in total, and 99 for the relation is quite interesting. It should be 100 for the relation instead of 99 (I had 100 for this relation with the previous issue), if that help.

Edit : My guess is that one of the 100 actors doesn't generate a query and work as intended, and the 99 others do. So you could say... I got 99 queries, but this model ain't one :sunglasses:

[AdamSGit]

Sorry, I'm stupid. The actor relation was actually has_one. So with with my brain plugged on, I do have one query as expected.

Also, you will be pleased to hear that I tested with 2 nested has_many relations, the first having 100 row, and the seconds 100 rows per row as well, so 10.001 models in total being created, cache disabled. The test with your recent commit gave me ~630ms to load, and ~8.4mb of ram. The test with the current 1.8 branch gave me maximum execution time of 30 seconds exceeded, so we can say it's a good perf improvment. 😄

On a sidenote, I noticed that the relations indexes now start at 0. Do I remember correctly that it was supposed to be the relating model id ?

[WanWizard]

Just noticed that too, on the todo list...

[WanWizard]

Fixed: https://github.com/fuel/orm/commit/8189898e7482c3917c8ea64323f0b501376df808

[AdamSGit]

Well done for the rewriting. Make much more sense. Will continue to test it and if I don't find any issue, I will test a deployment on the dev version of our main website at work.

Do you plan to merge it to 1.8, and if so will you bump minor version for this, or wait for other stuff ?

[WanWizard]

Haven't made any plans yet.

We've deployed it to all our apps and the staging of all client apps yesterday. We run everything on develop ( we eat our own dogfood :rofl: ). So far, no issues reported.

[AdamSGit]

Good ! Since you're using it on reallife apps, did you made benchmarks or noticed substancial performance improvements ?

[WanWizard]

On my laptop, not very scientific.

Just got a report in that on some pages more queries are generated, so there seems to be more work to do,

[WanWizard]

Looks like a cache population issue on related objects, causing lazy loads for objects that should already be in cache. Needs further debugging...

[WanWizard]

Caching issue found and fixed.

In a quick test of a few complicated pages I see no real significant change in speed (it looks slightly slower), but there is a reduction in memory usage.

[AdamSGit]

Good to hear.