commitUnitOfWorkAndContinue

40 views
Skip to first unread message

jtuchel

unread,
Sep 11, 2012, 4:57:05 AM9/11/12
to glorp...@googlegroups.com
It seems we've found a weird bug in this method that affects to-many-relationships and deletions of objects in such a relationship.

There are more details about it here:

It seems the problem is in current versions of GLORP as well, so I wanted to let people on this group know about it.
As far as I could see in the debugger, the problem is that the collections that reference a deleted object get re-registered at the end of the method, so GLORP puts the deleted objects back into the caches, even though it correctly deleted them during commit.

So the next commit issues an INSERT of these objects. 

The issue is gone when we don't use commitUnitOfWorkAndContinue and do a full commit and begin.

Joachim

Alan Knight

unread,
Sep 12, 2012, 2:25:12 PM9/12/12
to glorp...@googlegroups.com
Hmm. That is a nasty one. Thanks for tracking that down.

So, is it that you deleted the objects, but they were still present in
collections, so when the collection was re-registered it came back? Or
was the object also deleted from the collection, but the older version
of the collection was re-registered, or otherwise phantom state came
back. If it's the former, then I think you really ought to be removing
the objects from the collections as well, although silently
re-registering them is not a good behaviour.
> --
> You received this message because you are subscribed to the Google Groups
> "glorp-group" group.
> To view this discussion on the web visit
> https://groups.google.com/d/msg/glorp-group/-/DoGQIVfX4ggJ.
> To post to this group, send email to glorp...@googlegroups.com.
> To unsubscribe from this group, send email to
> glorp-group...@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/glorp-group?hl=en.

jtuchel

unread,
Sep 12, 2012, 4:11:59 PM9/12/12
to glorp...@googlegroups.com
Alan,

I didn't track the problem down deep enough to tell exactly. All I know is I've seen that in the collection of objects to be re-registered, the object in question had been removed, but there were collections in that collection which contained the object in question. So I guess it is the first alternative you mention: the object is still present in the collection.

I guess this is also the reason why the first commit deletes everything nicely, but the next one will re-insert the object that was deleted before.

I did not really see in the debugger when and how the deleted object got re-added to the caches, I am just guessing this happens because the collections refer to it... All I know is that it was removed from the temp collection and after in registerAll: they were registeredAsNew: . Therefor the inserts in the next commit.

Interestingly, I cannot find a real reason for why UnitOfWork>>#updateSessionCache, which is called in the normal commit method does not have the same issues. It doesn't seem to handle that collection problem at all, but the phenomenon seems to be gone since we use commitUOW and beginUOW instead of commitUOWAndContinue...

Joachim

jtuchel

unread,
Sep 12, 2012, 4:35:20 PM9/12/12
to glorp...@googlegroups.com
Alan,

re-reading your question, you probably asked me something else: did I remove the objects from the collections by hand before deleting? Well, I tried that as well and had similar issues but then the first commit would update the objects and set their foreign keys to null. So the end result is the same: data corpses with foreign keys of NULL.

But let me ask you what the right way to implement such a deletion should be.

Let'st take this example: I have an order with items and want to delete an item from an order's list of items. Should I simply do this:

self session 
  delete: theObsoleteItem; 
  commitUnitOfWorkAndContinue.

Or should I also remove the item from the Smalltalk collection before I delete it, like this:

thOrder items remove: theObsoleteItem.
theObsoleteItem order: nil.
self session 
  delete: theObsoleteItem; 
  commitUnitOfWorkAndContinue.

We've had strange results with both ways of doing it, but I must say I've played with variants of these statements' sequence like commiting first and then removing and nilling and maybe I am just too confused now after almost two days of playing with this.

As always, thanks for your time and help! 

Joachim

Alan Knight

unread,
Sep 12, 2012, 4:59:57 PM9/12/12
to glorp...@googlegroups.com
The right thing to do is definitely to both delete the object and to
remove it from Smalltalk collections that reference it. If that gives
strange behaviour, it's definitely a bug. But if you do it in a way
that the system doesn't expect, giving a reasonable error message
rather than silently re-inserting would be a very desirable feature.
> https://groups.google.com/d/msg/glorp-group/-/6VPZl5hy0BMJ.

jtuchel

unread,
Sep 13, 2012, 5:49:32 AM9/13/12
to glorp...@googlegroups.com
Hi Alan,

hmm. What you say makes sense. Remove the object frm collections in my Smalltalk code by hand and then have the Glorp session delete the object.

This will, however not work in some situations. Mine is such a situation. Let me explain ( I was hoping I can keep this whole thing simple, but I can't):

Take an object model of a typical Bill-of-materials kind:

Part --- 1:n, beExclusive --> CompositeSubpart -- 1:n, beExclusive --> Subpart

Say I want to delete one of the CompositeSubparts from a Part and want Glorp to handle the deletion of all Subparts in that CompositeSubpart.Something like this:

myCar subparts remove: dashboard.
self session delete: dashboard.
self session commitUnitOfWorkAndContinue.

This will issue delete statements for the dashboard and all its subparts. Good.

BUT: When is the right point in time to remove the subparts from the dashboard's (CompositeSubpart) collection of subparts?
If I remove them before the commit, Glorp will not delete them, because at the very moment Glorp tries to find out which objects to delete, there is no entry for the subparts in the dashboard's subparts collection. Am I right?

If I remove them after the Commit, Glorp will re-register them, because the dashboard's subparts collection will still include the supparts.

So I see no way around Glorp removing all deleted objects from all collections that get re-registered in commitUnitOfWorkAndContinue and thereby changing the state of model objects. There is a naive approach to it: not only remove the deleted objects from the registeredObjects collection, but also form all collections inside registeredObjects. The cleaner approach would surely be to walk through the descriptor system. It would be slower and th outcome would probably not be any better.

So I suggest something along the lines of:

commitUnitOfWorkAndContinue
   "Commit the current unit of work, but then keep going with the same set of registered objects, with their state updated to reflect current values."
   | registeredObjects |
   currentUnitOfWork isNil ifTrue: [^self error: 'Not in unit of work'].
   registeredObjects := currentUnitOfWork registeredObjects.
   currentUnitOfWork deletedObjects do: [:each | 
registeredObjects remove: each.
registeredObjects do: [:rO| (rO isKindOf: Collection) ifTrue: [rO remove: each] ]]. self commitUnitOfWork. self beginUnitOfWork. self registerAll: registeredObjects.


But is that enough? Will it change the original collections in the business objects (meaning: are the registeredObjects the objects I am using in my business code?)?

What do you think about this? Am I wrong with my dashboard->subparts hypothesis?

Joachim

BTW: in the meantime, I found out that commitUOW + beginUOW will not really solve my problem, because it introduces exactly the problem which commitUnitOfWorkAndContinue aims to solve: objects that were registered before the commit will not be registered afterwards, so consecutive commits will not see any changes to objects that were read in the same session but before the first commit. I'd have to reregister (or maybe refresh?) all objects that I want to change in consectutive commits in the same session. That is not a nice approach...

jtuchel

unread,
Sep 13, 2012, 6:37:00 AM9/13/12
to glorp...@googlegroups.com
My suggested fix/extension ignores Dictionaries, but I'd like to see what you think of it in general before I look deeper into this.

jtuchel

unread,
Sep 13, 2012, 10:43:28 AM9/13/12
to glorp...@googlegroups.com
Okay, now I am frustrated.

My suggestion doesn't work (it is not as simple as I wrote before, but very similar).
I'm getting INSERTS of previously deleted objects at next commit. Both dashoboard and subparts get re-inserted.


I also tried the manual approach:
I removed the CompositeSubpart (dashboard) from the Part's collection of subparts and removed all subparts from the dashboard's subparts collection. Then I did a self session delete: for each of the subparts and the dashboard. The deletes get executed on commit, and the next commit INSERTs all of them again. When I also set the backpointer instance variables to nil (before commit), the situation doesn't change.

Do you have any idea as to what I am doing wrong?

Thanks

Joachim

Alan Knight

unread,
Sep 13, 2012, 8:36:23 PM9/13/12
to glorp...@googlegroups.com
Hmm. I think the right thing is that when you delete Glorp should
traverse the exclusive relationship based on its state at the
beginning of the unit of work. So if you removed something from a
collection, then it automatically gets deleted. I think that's the way
it works now. And similarly, if you delete the whole collection, it
should know what to traverse.

That does open up an interesting case. If I start a unit of work, add
an existing object A to an exclusive collection, then delete the
parent of the exclusive collection, you'd want A to be deleted as
well. I'm suspicious that wouldn't happen with the current
implementation.

It's possible that the fix in your case is that
commitUnitOfWorkAndContinue should understand exclusive relationships
better, and when it goes over the objects to re-register, things that
are reached only by exclusive relationships from deleted things should
not be re-registered. Which might be more easily stated as deleting
everything that's in an exclusive relationship, and not re-registering
deleted objects. But collections might be a special case. So it might
be the case that a fix for this is to mark the exclusive collections
as deleted. That wouldn't actually cause anything to happen in the
database, but might make re-registering know to ignore them.

Sorry, I'm speculating in the absence of code here.
> https://groups.google.com/d/msg/glorp-group/-/FNQXhlt8FQAJ.

jtuchel

unread,
Sep 14, 2012, 4:11:00 AM9/14/12
to glorp...@googlegroups.com
Hi Alan,

hmm. I'm probably in a special case here.
I'm removing an object from a 1:n Relationship. This object, in turn, deletes all its subObjects from an exclusive 1:n relationship. Only in this scenario I see those INSERTs in the next transaction, for both the deleted element and its subElements.

This may have to do with the fact that the way commitUnitOfWorkAndContinue works now, it doesn't know about these subelements until the actual commitUnitOfWork happens (because the detection of the subelement happens in the course of commitUnitOfWork). That means when the deletedObjects are being removed from the registereObjects collection, the sub-Objects of the deleted object are not yet in the list of deletedObjects in the current Unit of Work, they'll be added there in #propagateDeletes.

Unfortunately, this also doesn't work (for reasons I am about to search for now):

    | registeredObjects currentUnit |


    currentUnitOfWork isNil ifTrue: [^self error: 'Not in unit of work'].
    registeredObjects := (currentUnit := currentUnitOfWork) registeredObjects.
 
  self commitUnitOfWork.
   
"moved down here to also handle sub-elements and remove them"

    currentUnit deletedObjects do: [:each |

        registeredObjects remove: each.
        registeredObjects do: [:rO |
            (rO isGlorpProxy not or: [rO isInstantiated])
                ifTrue: [
                    (rO isCollection and: [rO isArray not and: [rO includes: each]]) ifTrue: [rO remove: each ifAbsent: []]]]].

    self beginUnitOfWork.
    self registerAll: registeredObjects

I still get inserts on the next commit. So there seems to be more that needs to be done.

Maybe I really need to avoid commitUnitOfWorkAndContinue and find clever ways to register some "root objects" after a beginUnitOfWork. This has to be done in my application code then and cannot be done by Glorp, because it has no idea which objects could be "roots". If I can register the right root objects, registerTransitiveClosureFrom: should do the job, right?

Joachim

jtuchel

unread,
Sep 14, 2012, 5:03:23 AM9/14/12
to glorp...@googlegroups.com
Hi again.

I now tried not using commitUOWAndContinue but instead call commitUOW and beginUOW and register my "root objects" in the new uow by hand. Since register: does a registerTransitiveClosureFrom:, all reachable and instantiated objects are registered for further updates and deletes. So far, all seems good with this approach.

Unfortunately, not everywhere in an application you can tell what "root objects" should be re-registered. Glorp surely can't tell either, because a root object is not a concept in the relational world. So there is no way to handle this re-registration in Glorp without going through all mappings and stuff.

For now, I think I can clearly say that commitUnitOfWorkAndContinue is broken for propagated deletes, and it seems we need somebody with very deep knowledge in Glorp to fix that. I seem not to qualify here but I'd be happy to help isolate the problem in a testcase.

So, Alan, can we still consider you as core maintainer, or does anybody else feel he/she could do it?
What is the primary development platform for GLORP? Is it still VisualWorks?

Joachim
Reply all
Reply to author
Forward
0 new messages