GlorpSession>>#refresh: doesn't update the current Transaction's undoMap

[jtuchel]

Hi there,

I am almost sure I found a conceptional bug in Glorp, at least in the version that ships with VAST Platfom 10.

I have this situation:

In our business model there is a need for auto-numbering some objects. Since our database holds objects of several users, each of them having their own idea of what a "number" looks like and where to start numbering etc., we cannot use a database sequence. We use a BEFORE INESRT trigger to create and save numbers of newly created objects in the database. This way, we can be sure the numbers are consecutive and there are no holes in the numbers. But that is not extremely important, just to give you some context.

In order to reflect the newly assigne numbers in the application, we refresh: the inserted objects after a commit. This also works fine, the Instance in the image has the newly assigned number.

...until we issue a rollback after the refresh. The rollback replaces the number just read from the database with nil, as it was when we inserted the object.

Let me show you some test code which demonstrates the problem. Please ask if you are interested in this test case, i have it running here in my VAST Platform image.

We have a class and a table named Customer with a few attributes. For this test, we only need the id and customerName attributes, so building a DB, descriptor and ST class should be easy on any other platform.

This is what the code does:

• Start a UnitOfWork
  
• Create a Custome named 'Albert Hughes'
• Register it with our GlorpSession and #commitUnitOfWorkAndContinue
• We simulate the ON BEFORE INSERT TRIGGER with a simple SQL statement issued directly to the DB, updating the name of the Customer from 'Albert Hughes' to 'Harry Flutter'
• refresh: the Customer and check the name of the Customer instance. It is 'Harry Flutter' as expected
• rollbackUnitOfWorkAndContinue
• check the name of the object. It is now 'Albert Hughes' again

I thknk this is absolutely wrong. By refreshing, we learned that the database value is 'Harry Flutter', not 'Albert Hughes'. So rolling back should roll back to the last known value from the database, not the last name known by Glorp a long time ago...

If you want to take a look for yourself, here is the test code I used to isloate this

testRefreshDoesntUpdateUNdoMap

    "This is to show that GlorpSession refresh: does refresh an object, but doesn't update the current ObjectTransaction's undoMap"
    "Tha 'BUG'? leads to wrong results when rolling back after  a Refresh"
   
    |customer initialName nameUpdatedOutsideOfGlorp firstUndoMapEntry secondUndoMapEntry |
    initialName := 'Albert Hughes'.
    nameUpdatedOutsideOfGlorp := 'Harry Flutter'.
   
    session beginUnitOfWork.
    customer := Customer new customerName: initialName.
    session register: customer; commitUnitOfWorkAndContinue.
   
    firstUndoMapEntry := session privateGetCurrentUnitOfWork privateGetTransaction undoMap associations first.
    self assert: firstUndoMapEntry key customerName = initialName.
    self assert: firstUndoMapEntry value customerName = initialName.
   
    "These follwoung statements simulate an update performed by a BEFORE INSERT TRIGGER from my production code"
    session accessor executeSQLString: ('update customer set name = ''%1'' where id = %2' bindWith: nameUpdatedOutsideOfGlorp with: customer id asString ).
    session accessor commitTransaction.

    "We fetch the updated name (which we know in our prdouction system would be set by a DB trogger="
    session refresh: customer.
   
    self assert: customer customerName =  nameUpdatedOutsideOfGlorp.  "The refreshed object now has the new name, as updated in the DB by the trigger, or in this test case by our manual SQL statement"
   
    session rollbackUnitOfWorkAndContinue.

    "IMO, the name should now be rolled back to what the previous refresh:  returned (Harry Flutter), not the initial name.
     The initial name does not reflect the last known state of the DB - Glorp insists it is the value it has last saved, which I think is wrong"
    self assert: customer customerName =  nameUpdatedOutsideOfGlorp.
    secondUndoMapEntry := session privateGetCurrentUnitOfWork privateGetTransaction undoMap associations first.
    self assert: secondUndoMapEntry key customerName = nameUpdatedOutsideOfGlorp.  "It is Albert Hughes, which is not the value currently in the DB, and also not what the refreshed object in the image had just before the rollback"
    self assert: secondUndoMapEntry value customerName = nameUpdatedOutsideOfGlorp. "It is Albert Hughes, which is not the value currently in the DB"

It would be great if someone could test this on Pharo or even better on a current Version of Visualworks. Can you confirm my results?

But the most important question: is my expectation wrong or is this an actual bug in Glorp?

Thanks,

Joachim

[jtuchel]

Sorry for the strange colors of my code snippet on Google Groups. Here it comes  in a much better readable version:

testRefreshDoesntUpdateUNdoMap

    "This is to show that GlorpSession refresh: does refresh an object, but doesn't update the current ObjectTransaction's undoMap"
    "Tha 'BUG'? leads to wrong results when rolling back after  a Refresh"
   
    |customer initialName nameUpdatedOutsideOfGlorp firstUndoMapEntry secondUndoMapEntry |
    initialName := 'Albert Hughes'.
    nameUpdatedOutsideOfGlorp := 'Harry Flutter'.
   
    session beginUnitOfWork.
    customer := Customer new customerName: initialName.
    session register: customer; commitUnitOfWorkAndContinue.
   
    firstUndoMapEntry := session privateGetCurrentUnitOfWork privateGetTransaction undoMap associations first.
    self assert: firstUndoMapEntry key customerName = initialName.
    self assert: firstUndoMapEntry value customerName = initialName.
   
    "These follwoung statements simulate an update performed by a BEFORE INSERT TRIGGER from my production code"
    session accessor executeSQLString: ('update customer set name = ''%1'' where id = %2' bindWith: nameUpdatedOutsideOfGlorp with: customer id asString ).
    session accessor commitTransaction.

    "We fetch the updated name (which we know in our prdouction system would be set by a DB trigger)"

    session refresh: customer.
   
    self assert: customer customerName =  nameUpdatedOutsideOfGlorp.  "The refreshed object now has the new name, as updated in the DB by the trigger, or in this test case by our manual SQL statement"
   
    session rollbackUnitOfWorkAndContinue.

    "IMO, the name should now be rolled back to what the previous refresh:  returned (Harry Flutter), not the initial name.
     The initial name does not reflect the last known state of the DB - Glorp insists it is the value it has last saved, which I think is wrong"
    self assert: customer customerName =  nameUpdatedOutsideOfGlorp.
    secondUndoMapEntry := session privateGetCurrentUnitOfWork privateGetTransaction undoMap associations first.
    self assert: secondUndoMapEntry key customerName = nameUpdatedOutsideOfGlorp.  "It is Albert Hughes, which is not the value currently in the DB, and also not what the refreshed object in the image had just before the rollback"
    self assert: secondUndoMapEntry value customerName = nameUpdatedOutsideOfGlorp. "It is Albert Hughes, which is not the value currently in the DB"

[jtuchel]

Okay, I ported my testcase to Pharo 10, and after solving a few portng issues and getting PostgreSQL and P3 to run, I see that the effect is the same on Pharo. The only difference here is that there is no rollbackUnitOfWorkAndContnue in Glorp's Pharo port. But with rollbackUnitOfWork, the result is the same....

So at least I know this is not a VAST specific problem (which I hadn't assumed anyways). I would guess even in newer Glorp versions on VW the result would be the same.

The main questions remain:

1. is my expectation wrong?
2. Should we extend/change GlorpSession>>#refresh: to make sure the transaction's undoMap is updated to the latest version we read from the DB?
   

Joachim

[Esteban Maringolo]

Hi Joachim,

I think that the refresh: should also refresh the RowMap (aka "undo
map") of the object.

So, I guess this is a bug in Glorp.


Esteban A. Maringolo

> --
> You received this message because you are subscribed to the Google Groups "glorp-group" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to glorp-group...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/glorp-group/e3d2f867-7d61-458a-bd43-71710116842cn%40googlegroups.com.

[Alan Knight]

It's a bit tricky in a more general case, I think. But it does seem reasonable that if you do a refresh of an object in the middle of a unit of work, that undo map for that particular object should be updated to the new state. Otherwise you can end up attempting to re-write values that are already there in the database.

But it's a little weird if you do a rollback in a unit of work that involves new objects. Normally I'd expect that to mean that those objects weren't going to be written, and should be discarded.

Esteban: Note that the undo map isn't the same as the row map. The undo map is the shallow(-ish) copy of the object that we make when it's read. The row map is a transient thing containing rows for all the objects in the transactions. So at commit unit of work time we create a row map for the objects, we create a rowmap from the undo map, and then we compare those two in order to determine which row values are changed/new, and that's what we have to write.

To view this discussion on the web visit https://groups.google.com/d/msgid/glorp-group/CAJMgPCKPoMemef%2BpybJEJpzYo7Fe2T92pW_uoF8vSc%3DNyjgTQQ%40mail.gmail.com.

[jtuchel]

Esteban, Alan,

thanks a lot for your comments. So my expectation wasn't wrong, that is good to know.

So I tried digging down a little deeper. GlorpSession>>refresh: does indeed privateRegisterAsOld: the newly read / updated object. I think the problem is in ObjectTransaction>>#register:.

Please take a look at this screenshot of my VAST Debugger:

As you can see, refresh: tries to register teh newly read Customer who is now named 'Harry Flutter', which is what was read from the DB.

So we have a realObject which is the correctly named one we are refreshing, and we have the old one ('Albert Hughes') as value in the undoMap:

Now register decides: to not update the value in the undoMap because teh realObject ('Harry Flutter') is already registerd as key. But it is associated with the outdated copy named 'Albert Hughes'.

So I wonder if just removing the includesKey: check would be the right solution. I understand it is more or less just a performance optimization (but maybe a big one, in case of Collections).

Or would it be better to add a check for equality on the value of the undoMap entry?  Something along the lines of:

register: anObject
    "Make anObject be a member of the current transaction. Return the object if registered, nil otherwise"

    | copy realObject |

    realObject := self realObjectFor: anObject ifNone: [^nil].
    "The next line inlines #isRegistered: - we just got the real object so need not get it again."

    

    "Suggested change by Joachim Tuchel to make sure #refresh:ed objects get updated properly "

    (undoMap at: realObject ifAbsent: []) ifNotNil: [:regObj| regObj =  realObject ifTrue: [^nil]].

    copy := self shallowCopyOf: realObject ifNotNeeded: [^nil].
    undoMap at: realObject put: copy.
    self registerTransientInternalsOfCollection: realObject.
    ^realObject

I checked my test case and this change yields the expected outcome. I thought that is it.

BUT: it breaks the Glorp Tests: I get a StackOverflows in GlorpManyToManyDBTest>>#testReadAndDeleteNodes in UnitOfWork>>register: when it tries to registerTransitiveClosureFRom: of an Array . So my change to ObjectTransaction>>#register: doesn't break, it "only" causes side effects ;-)

So I tried all kinds of extra guards for proxies or collections for realObject. I also tried using includesKey: first and only ifTrue would I access undoMap at: realObject. But still I get StackOverflows.

In short; I am lost for now and need help getting this to work.

Any comments?

Joachim

[jtuchel]

Just for completeness: removing the check for includesKey: yields the same StackOverflow

[jtuchel]

The StackOverflow happens because GlorpSession>>#register: does a #registerTransitiveClosureFrom: for each refresh:ed object (like, when instantiating a Proxy). And since the current check in ObjectTransaction>>#register: returns nil if an object is already registered, an object that has already been registered doesn't do another round of #registerTransitiveClosureFrom:.

My suggested fix, however, doesn't return nil as often but instead highers the risk of more Proxies needing to be instantiated and therefor #register:ed in the current ObjectTransactipon. So if an object is being circularly referenced, the register:ing never stops (until the stack flows over, that is).

So my suggested fix is not good.

I changed my test case a little, just to ensure this whole thing is not a problem of #rollbackUnitOfWork or #rollbackUnitOfWorkAndContinue, but really stems from the fact that #refresh: doesn't update the already loaded shalllow copy of an object in its undoMap. All I added was another round of checking the undoMap right after refresh:

testRefreshDoesntUpdateUNdoMap

    "This is to show that GlorpSession refresh: does refresh an object, but doesn't update the current ObjectTransaction's undoMap"
    "Tha 'BUG'? leads to wrong results when rolling back after  a Refresh"
   

    |customer initialName nameUpdatedOutsideOfGlorp firstUndoMapEntry secondUndoMapEntry thirdUndoMapEntry|

    initialName := 'Albert Hughes'.
    nameUpdatedOutsideOfGlorp := 'Harry Flutter'.
   
    session beginUnitOfWork.
    customer := Customer new customerName: initialName.
    session register: customer; commitUnitOfWorkAndContinue.
   
    firstUndoMapEntry := session privateGetCurrentUnitOfWork privateGetTransaction undoMap associations first.
    self assert: firstUndoMapEntry key customerName = initialName.
    self assert: firstUndoMapEntry value customerName = initialName.
   
    "These follwoung statements simulate an update performed by a BEFORE INSERT TRIGGER from my production code"
    session accessor executeSQLString: ('update customer set name = ''%1'' where id = %2' bindWith: nameUpdatedOutsideOfGlorp with: customer id asString ).
    session accessor commitTransaction.

    "We fetch the updated name (which we know in our prdouction system would be set by a DB trogger="
    session refresh: customer.
   
    self assert: customer customerName =  nameUpdatedOutsideOfGlorp.  "The refreshed object now has the new name, as updated in the DB by the trigger, or in this test case by our manual SQL statement"
    

"I just added this extra check"

    secondUndoMapEntry := session privateGetCurrentUnitOfWork privateGetTransaction undoMap associations first.
    self assert: secondUndoMapEntry key customerName = nameUpdatedOutsideOfGlorp.  

    self assert: secondUndoMapEntry value customerName = nameUpdatedOutsideOfGlorp. "<--- this is where the test fails!!!"

   
    session rollbackUnitOfWorkAndContinue.

    "IMO, the name should now be rolled back to what the previous refresh:  returned (Harry Flutter), not the initial name.
     The initial name does not reflect the last known state of the DB - Glorp insists it is the value it has last saved, which I think is wrong"
    self assert: customer customerName =  nameUpdatedOutsideOfGlorp.

    thirdUndoMapEntry := session privateGetCurrentUnitOfWork privateGetTransaction undoMap associations first.
    self assert: thirdUndoMapEntry key customerName = nameUpdatedOutsideOfGlorp.  "It is Albert Hughes, which is not the value currently in the DB, and also not what the refreshed object in the image had just before the rollback"
    self assert: thirdUndoMapEntry value customerName = nameUpdatedOutsideOfGlorp. "It is Albert Hughes, which is not the value currently in the DB"

So I need a way to not initiate an endless loop of #registerTransientClosureFrom:, but also make sure an already registered object gets updated if it has changed "outside" and we learn about it in a #refresh:

Any ideas?

Joachim

[Esteban Maringolo]

Joachim: 

Who ends up calling the register: when you refresh: an object?

Couldn't you simply force the undoMap update from the #refresh: method itself? Maybe before returning you pass the "fresh" object to the session to update the undo map.

Sorry if my question is naive, I still haven't set up something to test it exactly the way you're doing.

Regards.

Esteban A. Maringolo

To view this discussion on the web visit https://groups.google.com/d/msgid/glorp-group/076480e5-ec55-4656-a0ed-520089a0d1b1n%40googlegroups.com.

[jtu...@objektfabrik.de]

Esteban,

good question. #refresh:  does a #privateRegisterAsOld:, which registers the refreshed object in the ObjectTransaction and then does a registertTransitiveClosureFrom: for this object.

In my simple test case the Customer object has only a name and an id and no associated objects/collections, so the test runs fine for my simple case. But tests where objects are part of a net with circular references (like in most O/R  models) end in desaster / StackOverflow. 

If you are interested and are willing to invest a few minutes (haha, I am on my second day...), I can provide you with a VAST App with my TestResource and code. All you need is a database. My tests are configured for DB2, but they should easily work with Postgres. In order to run the test, you create the DB, make a TestResource availably by sending it #makeAvailable, and your're ready to go.

In order to get the Stack Overflow, you simply run the Tests provided with Glorp (getting them to run is a bit tricky, but I can halb you with that).

Joachim

Am 15.09.22 um 17:18 schrieb Esteban Maringolo:

You received this message because you are subscribed to a topic in the Google Groups "glorp-group" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/glorp-group/59zHoYVMwQ8/unsubscribe.
To unsubscribe from this group and all its topics, send an email to glorp-group...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/glorp-group/CAJMgPC%2BOF2qOS5Tiww48PdS3AL_fbxbwNYwL_TSzf16-hQrjDg%40mail.gmail.com.

-- 

----------------------------------------------------------------------- 
Objektfabrik Joachim Tuchel              mailto:jtu...@objektfabrik.de 
Fliederweg 1                                 http://www.objektfabrik.de
D-71640 Ludwigsburg                  http://joachimtuchel.wordpress.com
Telefon: +49 7141 56 10 86 0                    Fax: +49 7141 56 10 86 1

[jtuchel]

Esteban,

the problem with your suggestion is that it is not like GlorpSession>>#refresh: diectly initiates the registration of an object in the Transaction's undoMap. It initiates a normal Query with readOneOf: where: and the registration (in this case re-registration) is done deep down in GlorpCursoredStream>>next. This Stream sends privateRegisterAsOld: to the Session - which in turn sends register: to the UOW and thus ends up in #register: in the underlying ObjectTransaction. So I don't think there is a place to change the way you suggest without risking bigger damage...

Nevertheless, I thought a bit about your idea.  refresh: is always an operation that updates a single object from the database. As far as I understand, this sepcific case of query never ends up updating referenced objects, so this "ending a traversal when an object is already in the undoMap" is not necessary in case of a refresh: operation. If you need associated objects refresh:ed as well, you have to manually refresh: them anyways. So there could be a special case for registering a refresh:ed object that doesn't #registerTransitiveClosureFrom:.

So GlorpSession>>refresh: could possibly use a new variant of all the underlying machinery starting at AbstractReadQuery which doesn't end up in the "normal" register: method in ObjectTransaction, but some #registerShallow: which forces an update of the shallowCopy stored in the undoMap. So a possible Solution would involve methods like: GlorpSession>>registerShallow: (no registerTransitiveClosurFrom:) and ObjectTransaction>>#registerShallow:. An alternative implementation would involve adding a new parameter to the existing methods.

But thinking a bit further, there is a problem: you can set shouldRefresh: to true for any Query. So you can construct a query for many objects all of which you want refreshed: . These queries also should use the shallow variants of registering, no matter if they were constructed by GlorpSession>>refresh: or in your application code by setting shouldRefresh: to true.

So since the actual call to #privateRegisterAsOld:  is initiated by GlorpCursoredStream in #next, this Stream should be aware that this privateRegisterAsOld: may be special in case of a query that shouldRefresh. AFAIC, the information would be available in #next, since the CursoredStream knows its #query...

I need to chew on all this a lttle more...

[jtuchel]

Hello again,

as you can see, this problem is keeping me busy ;-)

I have found a cowardish workaround for this problem, although I think it is critical for cases where we have the possibility of objects being modified outside of the current Smalltalk image...

Glorp provides a hook for persistent objects for post fetch called: glorPostFetch:, and since I know this only affects one of my classes, this can be used to update the undoMap of the current transaction with brute force.

glorpPostFetch: session

    (session isNew: self)
        ifFalse: [
            session privateGetCurrentUnitOfWork privateGetTransaction ifNotNil: [:objTrx |
                (objTrx undoMap at: self ifAbsent: []) ifNotNil: [:regObj |
                    regObj ~= self
                        ifTrue: [| copy |
                            copy := objTrx shallowCopyOf: self ifNotNeeded: [^self].
                            objTrx undoMap at: self put: copy]]
                ]
            ]

My initial testing indicates this is exactly what solves my actaul problem, without breaking Glorp. As you can see, this is more or less a brute force copy of GlorpSession>>#register: without the transitiveClosure part...

This is neither an elegant nor a clever fix for the problem. It just gives me the chance to go on with my daily work. I spent a good  part of 3 days just staring at the code and trying to understand what's (not) going on, and for now I have the feeling my mental horizon is too narrow to provide a real fix.

I will try to get in touch with somebody at Cincom and evoke their interest in the problem. Maybe they see a simpler way to solve the problem than I do. I thnk it should definitely be solved if Glorp is to be used in Mult-User apps or in combination with DB Triggers like in my case.

Joachim

[jtuchel]

For anybody interested, here is a test method that can easily be added to GlorpReadingTest from the Glorp Tests. So if you have the Glorp-Tests runinng, you can simply add this method to GlorpReadingTest and see for yourself. No extra setup or installation needed - just get the test suite to run and add this test method. (I am not sure if the name and Class are the best categorization for this test, but it looks good to me)

testRefreshAddressUpdatesUndoMap
    "Check that we not only refresh the working copy of an Address correctly when the refresh flag is set, but also the ObjextTransaction's undoMap"
    "otherwise, following rollbacks could rollback the Address to the initial state instead of the one we retrieved in the refresh"

    | address undoMapEntry |

    [
   
        "First we write some Address"
        session accessor executeSQLString: 'DELETE FROM GR_ADDRESS WHERE ID=888'.
        session commitTransaction.

        session beginUnitOfWork.
   
        address := GlorpAddress new.
        address id: 888; street:  'Initial Plaza'; number: '10'.
        session register: address; commitUnitOfWorkAndContinue.


        address := session readOneOf: GlorpAddress where: [:each | each id = 888].        
        self assert: address street =  'Initial Plaza'.
       
        session  accessor executeSQLString: 'UPDATE GR_ADDRESS SET STREET = ''Something Else'' where id=888;'; commitTransaction.
       
        session refresh: address.
        self assert: address street = 'Something Else'.
       
        undoMapEntry := session privateGetCurrentUnitOfWork privateGetTransaction undoMap associationAt: address.
        self assert: undoMapEntry key street = 'Something Else'. .
        self assert: undoMapEntry value street = 'Something Else'.
       
        session rollbackUnitOfWorkAndContinue.
       
        undoMapEntry := session privateGetCurrentUnitOfWork privateGetTransaction undoMap associationAt: address.
        self assert: undoMapEntry key street = 'Something Else'. .
        self assert: undoMapEntry value street = 'Something Else'.]
            ensure: [
                session  accessor executeSQLString: 'DELETE FROM GR_ADDRESS where id=888; commit;'.
                session rollbackUnitOfWork]

Joachim