Updating a record does not invalidate factories cache

[jon48]

"There are only two hard things in Computer Science: cache invalidation and naming things" Phil Karlton

As it stands today, when the methods updateFact, updateRecord or deleteRecord of a GedcomRecord object are called, they update the object itself, update it in the database, but at no point, the entry created in the cache for that record by the relevant AbstractGedcomRecordFactory is being invalidated. It means that if for whatever reason the object is being loaded through the make method after any of the update, it will load the object as it is in the cache, hence before the update, and not in line with its DB state. The cache entry should be invalided/removed when the GedcomRecord is updated.

This is more a code smell at the moment, as I have not found any occurrence in the current code base that could fall in this trap (records are usually updated as part of an Action call which is quite atomic), but I still think we should address it, as that could prove dangerous in a future development or a module.
I myself came across it when trying to create unit tests, as I was trying to create a use case by updating a gedcom record before calling the method I wanted to test, but because the latter was using the factory to retrieve the record, it would ignore any change I had made to the record (and even in the DB).

What is your opinion? I am conscious that could creep into other usages of the cache in records.
I have not thought about the details of how that could be implemented, but I can try to come up with a PR, except if you prefer to handle it.

[fisharebest]

I'm aware of this issue. My solution, as you observed, was:

records are usually updated as part of an Action call which is quite atomic

I think the complexity to fixing this comes from having a cache of the pending edits. Depending on whether our edit it auto-accepted, we may need to update/invalidate this.

I have a couple of concerns about "fixing" this issue.

It creates functionality that we do not use ourself. Bugs and edge-cases are likely to go unnoticed.

It might restrict our future options with regards to other performance tweaks, etc.

But if the solution is simple enough, then perhaps we should implement it.

(The "pending changes" system isn't very well implemented, and hasn't changed much since the early days of phpGedView. When I come to update the DB structure, I'll replace it with a system where multiple versions of data stored in the indi/fam/sour/etc. tables. It may well be easier to fix this issue then.)

[jon48]

Thanks Greg for your feedback.
I did not see the constraint on the pendingChanges initially, but looking a bit more in detail, I understand where you are coming from. I was more focusing on the creation of the records through the make method of the factories.

I have tried an implementation of the invalidation, based on tags, which I have submitted in PR #3748 . I will leave you to review and comment it.

[ric2016]

Just a note that this is a 'real' issue: I came across it in complex data fixes in one of my custom modules and had to address it there, so I'd be very interested in a proper solution.