Bug in GlorpSession>>#isNew:

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Bug in GlorpSession>>#isNew:

jtuchel
Hi there,

I think I found a bug in GlorpSession>>isNew: in the ported version that ships with VA Smalltalk 8.6. (Some Level of 7.9, I think)


What is the bug?

If you ask a GlorpSession whether an object is new and should be inserted, and the parameter to isNew: is a Proxy, you get 

Wrong Descriptor for this object!

in Descriptor>>primaryKeyForObject:.


How can I reproduce it?

Simple. 
  1. Read an object that has a 1:1 relationship to another one, but don't materialize the related object. Example: Read a woman that is married, but do not navigate to her husband.
  2. Ask the session if the woman's husband is new, like this: self glorpSession isNew: woman husband.
Since the husband is a Proxy, you should get the exception.


How can it be solved?

The reason is that primaryKeyForObject: assumes that it is working on the real object. It does not send #realObject to its parameter. Because the parameter could be a Proxy, it should be ready to handle that situation

To be honest, I am not really sure if #realObject should be sent in #isNew: or in #primaryKeyFor: or even in both. I guess both is best, but I am not sure about the implications.

Joachim

--
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 [hidden email].
To post to this group, send email to [hidden email].
Visit this group at http://groups.google.com/group/glorp-group.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: Bug in GlorpSession>>#isNew:

jtuchel
Here's my take on fixing this in GlorpSession>>#isNew:

isNew: anObject
"When registering, do we need to add this object to the collection of new objects? 
         New objects are treated specially when computing what needs to be written, since we don't have their previous state"

| key descriptor realObject |

(currentUnitOfWork notNil and: [currentUnitOfWork isRegistered: anObject]) ifTrue: [^false].
descriptor := self descriptorFor: anObject.
descriptor isNil ifTrue: [^false].
"For embedded values we assume that they are not new. This appears to work. I can't really justify it."
self needsWork: 'cross your fingers'.
descriptor mapsPrimaryKeys ifFalse: [^false].


realObject := self realObjectFor: anObject ifNone: [^self].
key := descriptor primaryKeyFor: realObject.
key isNil ifTrue: [^true].
"If the cache contains the object, but the existing entry is due to be deleted, then count this entry as a new one being added with the same primary key (ick) as the old one"
^[(self cacheContainsObject: realObject key: key) not]
on: DuplicatePrimaryKeyException
do: [:ex |
(currentUnitOfWork notNil and: [currentUnitOfWork willDelete: ex existingObject])
ifTrue: [
self cacheRemoveObject: ex existingObject.
ex return: true]
ifFalse: [ex pass]]



I am not completely sure if returning self in case there is no realObject is correct. I'd usually throw an exception, but Glorp has a bit of a special way of handling errors. I guess it would even be better to return a Boolean, most likely true. But even that I am not completely sure about ;-)
As I said, there may be a need to also add this realObject resolution thing to primaryKeyFor: as well...

Joachim

--
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 [hidden email].
To post to this group, send email to [hidden email].
Visit this group at http://groups.google.com/group/glorp-group.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: Bug in GlorpSession>>#isNew:

jtuchel
Has anybody who works on Glorp seen this post? 
Any ideas and/or comments?

Can anybody recreate the issue and confirm it as a bug?

Did anybody take a look at my suggested fix? Comments? Is there a process to feed such fixes back into Glorp (other than pm to people who are extremely busy)?

Joachim



Am Dienstag, 16. September 2014 13:29:48 UTC+2 schrieb jtuchel:
Here's my take on fixing this in GlorpSession>>#isNew:

isNew: anObject
"When registering, do we need to add this object to the collection of new objects? 
         New objects are treated specially when computing what needs to be written, since we don't have their previous state"

| key descriptor realObject |

(currentUnitOfWork notNil and: [currentUnitOfWork isRegistered: anObject]) ifTrue: [^false].
descriptor := self descriptorFor: anObject.
descriptor isNil ifTrue: [^false].
"For embedded values we assume that they are not new. This appears to work. I can't really justify it."
self needsWork: 'cross your fingers'.
descriptor mapsPrimaryKeys ifFalse: [^false].


realObject := self realObjectFor: anObject ifNone: [^self].
key := descriptor primaryKeyFor: realObject.
key isNil ifTrue: [^true].
"If the cache contains the object, but the existing entry is due to be deleted, then count this entry as a new one being added with the same primary key (ick) as the old one"
^[(self cacheContainsObject: realObject key: key) not]
on: DuplicatePrimaryKeyException
do: [:ex |
(currentUnitOfWork notNil and: [currentUnitOfWork willDelete: ex existingObject])
ifTrue: [
self cacheRemoveObject: ex existingObject.
ex return: true]
ifFalse: [ex pass]]



I am not completely sure if returning self in case there is no realObject is correct. I'd usually throw an exception, but Glorp has a bit of a special way of handling errors. I guess it would even be better to return a Boolean, most likely true. But even that I am not completely sure about ;-)
As I said, there may be a need to also add this realObject resolution thing to primaryKeyFor: as well...

Joachim

--
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 [hidden email].
To post to this group, send email to [hidden email].
Visit this group at http://groups.google.com/group/glorp-group.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: Bug in GlorpSession>>#isNew:

Maarten Mostert

Jochaim,

 

What is the bottom line here, why do you want to change this method.

 

Dis you encounter primary key duplicate errors ?

 

Regards,

 

@+Maarten,

 

 

 

 

> "jtuchel" <[hidden email]> |

Has anybody who works on Glorp seen this post? 
Any ideas and/or comments?
Can anybody recreate the issue and confirm it as a bug?
Did anybody take a look at my suggested fix? Comments? Is there a process to feed such fixes back into Glorp (other than pm to people who are extremely busy)?
Joachim


Am Dienstag, 16. September 2014 13:29:48 UTC+2 schrieb jtuchel:
Here's my take on fixing this in GlorpSession>>#isNew:
isNew: anObject
"When registering, do we need to add this object to the collection of new objects? 
         New objects are treated specially when computing what needs to be written, since we don't have their previous state"
 
| key descriptor realObject |
 
(currentUnitOfWork notNil and: [currentUnitOfWork isRegistered: anObject]) ifTrue: [^false].
descriptor := self descriptorFor: anObject.
descriptor isNil ifTrue: [^false].
"For embedded values we assume that they are not new. This appears to work. I can't really justify it."
self needsWork: 'cross your fingers'.
descriptor mapsPrimaryKeys ifFalse: [^false].
 
 
realObject := self realObjectFor: anObject ifNone: [^self].
key := descriptor primaryKeyFor: realObject.
key isNil ifTrue: [^true].
"If the cache contains the object, but the existing entry is due to be deleted, then count this entry as a new one being added with the same primary key (ick) as the old one"
^[(self cacheContainsObject: realObject key: key) not]
on: DuplicatePrimaryKeyException
do: [:ex |
(currentUnitOfWork notNil and: [currentUnitOfWork willDelete: ex existingObject])
ifTrue: [
self cacheRemoveObject: ex existingObject.
ex return: true]
ifFalse: [ex pass]]
I am not completely sure if returning self in case there is no realObject is correct. I'd usually throw an exception, but Glorp has a bit of a special way of handling errors. I guess it would even be better to return a Boolean, most likely true. But even that I am not completely sure about ;-)
As I said, there may be a need to also add this realObject resolution thing to primaryKeyFor: as well...
Joachim
--
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 [hidden email].
To post to this group, send email to [hidden email].
Visit this group at http://groups.google.com/group/glorp-group.
For more options, visit https://groups.google.com/d/optout.

--
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 [hidden email].
To post to this group, send email to [hidden email].
Visit this group at http://groups.google.com/group/glorp-group.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: Bug in GlorpSession>>#isNew:

jtuchel
Hi Maarten,

If you ask a Session whether an object isNew: and the object is a Proxy, the operation fails instead of simply answering false.

Without a fix (like the one I suggest), you'd have to always ask:

(myObject isProxy or: [dbSession isNew: myObject]) ifTrue: ...

I hope this explanation is a bit easier to understand...

Background:
I needed this fix to clean up a Collection's contents for the Commit of a Dialog in which you can add and remove many objects in one single transaction. Removed objects need to be deleted from the session, and new ones sometimes have to be registered. In some cases, registering objects for deletion led to confusion and objects were deleted and tried to be updated afterwards. So I had to help Glorp here and needed a test whether an object is new or not. But it failed if some object was still a Proxy.

Joachim

--
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 [hidden email].
To post to this group, send email to [hidden email].
Visit this group at http://groups.google.com/group/glorp-group.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: Bug in GlorpSession>>#isNew:

Maarten Mostert

Hi Joachim,

Personally I consider objects to be new only when their primarykey = nil, so I don't approach the problem the way you do.

However I certainly know about the delete, and register problem which asks for different behaviour if an object is or is not in cache, and lets be honest it'll drive you nuts.

In order to adress this I now use two methods which have proven to be reliable (registerExisting: and deleteExisting: ) source code below.

I use them to register any object which has a known primary key and for all deletes.

I only use the classic register: for "new" Objects with a primaryKey which is nil.

It has proven reliable to the point where I can delete > 10000 Objects spanning 16 different tables in a single unitOfWork.

 

This reliability does however come with a philosophy and some cost.

 

You only read the root Object of your model (and never something else).

You'll ban refresh (which like readings, brakes the identity relationships of proxies).

You never assign an Object to a variable except for the root object of your model (you can use parameters, arrays or valueHolders but no assignments).

You'll use count on every registration to find out wheter the Object should be added in cache.

 

If all of the above is ok for you, you can end up with a very reliable model.

 

Hope this helps.

 

Regards,

 

@+Maarten,

 

 

registerExisting: anObject

            "Register an existing object which complies to full identity. First Check wheter the object exist on the database by counting, if it no longer exist set its primary key to nil and register.

            If it does exist on the database check whether its in cache, if not adds it to the cache to avoids any primarykey error."

 

            anObject class = Proxy

                       ifTrue: [self registerExistingFrom: anObject getValue]

                       ifFalse: [self registerExistingFrom: anObject]

 

 

registerExistingFrom: anObject

            "Register an existing object which complies to full identity. First Check wheter the object exist on the database by counting, if it no longer exist set its primary key to nil and register.

            If it does exist on the databe check wheter its in cache, if not adds it to the cache to avoids any primarykey error. "

 

            | aCounter primKeys fields queryCount |

            anObject isNil

                       ifFalse:

                                   [fields := (self descriptorFor: anObject) primaryTable primaryKeyFields.

                                   primKeys := OrderedCollection new.

 

                                   "Check wheter primarykeys are not nil "

                                   (self descriptorFor: anObject) primaryKeyMappings

                                               do: [:eachField | primKeys add: eachField attribute name].

                                   ((primKeys collect: [:each | anObject perform: each]) includes: nil)

                                               ifFalse:

                                                          [fields size = 1

                                                                      ifTrue:

                                                                                  [queryCount := Glorp.Query count: anObject class

                                                                                                                     where: [:each | (each perform: primKeys first) = (anObject perform: primKeys first)]]

                                                                      ifFalse:

                                                                                  [queryCount := Glorp.Query count: anObject class

                                                                                                                     where: [:each | (each perform: primKeys first) = (anObject perform: primKeys first)].

                                                                                  2 to: fields size

                                                                                             do:

                                                                                                         [:n |

                                                                                                         queryCount := queryCount

                                                                                                                                            AND: [:each | (each perform: (primKeys at: n)) = (anObject perform: (primKeys at: n))]]].

                                                          aCounter := self execute: queryCount.

                                                          aCounter = 1

                                                                      ifTrue:

                                                                                  [((self cacheFor: anObject)

                                                                                             includesKey: ((self descriptorFor: anObject) primaryKeyFor: anObject)

                                                                                             withClass: anObject class)

                                                                                                         ifFalse:

                                                                                                                     [(self cacheFor: anObject)

                                                                                                                                at: ((self descriptorFor: anObject) primaryKeyFor: anObject)

                                                                                                                                ifAbsentPut: [anObject]]]

                                                                      ifFalse:

                                                                                  ["Primary key has a valid number"

                                                                                  aCounter = 0

                                                                                             ifFalse: [self error: ' no multiple instances with the same primary keys']]]].

            ^self register: anObject

 

 

deleteExisting: anObject

            "Get the real object, instantiating if necessary"

 

            | realObject |

            realObject := anObject glorpRealObject.

            self hasUnitOfWork

                       ifTrue: [currentUnitOfWork delete: realObject]

                       ifFalse: [self inUnitOfWorkDo: [currentUnitOfWork delete: realObject]].

            "Next Added

            Works also with multiple keys - presented as an Array"

            (self cacheFor: anObject)

                       removeKey: ((self descriptorFor: anObject) primaryKeyFor: realObject)

                       ifAbsent:

                                   [Transcript

                                               show: 'Glorp Delete was not in Cache';

                                               cr]

 

 

 

 

 

 

 

> "jtuchel" <[hidden email]> |

> Hi Maarten,


>
> If you ask a Session whether an object isNew: and the object is a Proxy, the
> operation fails instead of simply answering false.
>
> Without a fix (like the one I suggest), you'd have to always ask:
>
> (myObject isProxy or: [dbSession isNew: myObject]) ifTrue: ...
>
> I hope this explanation is a bit easier to understand...
>
> Background:
> I needed this fix to clean up a Collection's contents for the Commit of a Dialog
> in which you can add and remove many objects in one single transaction. Removed
> objects need to be deleted from the session, and new ones sometimes have to be
> registered. In some cases, registering objects for deletion led to confusion and
> objects were deleted and tried to be updated afterwards. So I had to help Glorp
> here and needed a test whether an object is new or not. But it failed if some
> object was still a Proxy.
>
> Joachim
>
> --
> 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
> [hidden email].
> To post to this group, send email to [hidden email].
> Visit this group at http://groups.google.com/group/glorp-group.
> For more options, visit https://groups.google.com/d/optout.
>

--
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 [hidden email].
To post to this group, send email to [hidden email].
Visit this group at http://groups.google.com/group/glorp-group.
For more options, visit https://groups.google.com/d/optout.