SourceForge.net Logo
Main Overview Wiki Issues Forum Build Fisheye
Issue Details (XML | Word | Printable)

Key: CMP-737
Type: Bug Bug
Status: Open Open
Priority: Major Major
Assignee: Shay Banon
Reporter: Maurice Nicholson
Votes: 1
Watchers: 1
Operations

If you were logged in you would be able to see more operations.
Compass

NPE when deleting indirect owner of simple (String) collection property

Created: 11/Oct/08 02:50 AM   Updated: 10/Nov/08 04:14 PM
Component/s: Compass::Gps
Affects Version/s: 2.1.0 M4
Fix Version/s: None

File Attachments: 1. File hibernate_delete_collection_npe.tar (60 kB)



 Description  « Hide
This issue was discovered by a user of the Grails Searchable Plugin: http://jira.codehaus.org/browse/GRAILSPLUGINS-482

What happens is that we have 2 classes: Owner and Ownee, where Owner-(1-1)->Ownee. Ownee additionally has a collection (Map or Set in Hibenate sense) which just contains strings. In the Hib mapping, we define set cascade=all on Owner#ownee.

Only Ownee is mapped as Searchable - not sure if this is important.

Creating and saving the Owner is fine.

Try to delete the Owner and you get a NPE in the marshalling code that is ultimately called by the PostCollectionDeleteEvent handler (ie, Compass Hibernate GPS mirroring). This is because the Owner#ownee collection property is one of Hibernate's PersistentSet/PersistentMap classes, but the collection it internally wraps is null, so any method invocations throw NPE.

The attached test cases reproduce the bug for both a Map and Set collection type. (They are "within" the context of the Compass source tree, but actually the package/test-case names are awful, and I did not intend that these should become part of added to Compass's own tests - it was just a quick way for me to write a test case)

Not sure where the blame for this lies with really - seems unlikely that this should be intended Hibernate behaviour as client code shouldn't have to know about Hib-specific collections and check for such things.

Then again maybe Compass shouldn't be trying to a full re-index given that this is a post-delete event.



 All   Comments   Change History      Sort Order: Ascending order - Click to sort in descending order
Shay Banon added a comment - 11/Oct/08 03:47 PM
I committed a fix that overrides the NPE exception in Hibernate, where I basically don't call any operation in Compass in case the load persister is null (which is what causing the NPE).

I think this should be ok for now, what do you think?


Maurice Nicholson added a comment - 12/Oct/08 11:30 AM
Hmm... what if the collection is deleted but the owning entity (in this case "Ownee") is not?

Presumably the re-index of Ownee would be skipped, and then the resource in the index would be out of sync with the DB, since in the DB the collection was deleted but it is still referenced from the index.


Shay Banon added a comment - 12/Oct/08 12:16 PM
Yes, but most of the times there will be another event (as is the case in the test you provided). In any case, the current state is better in terms of not raising the exception. Note, that this event was added by Hibernate and they can't even always know what is the owning entity of the collection.

We can have a better handling if we add special handling for this in the Collection converter and check for specific Hibernate collection and ignore this collection (as if it is null) in such cases. Will see if this can be done as well.


Jean-Noel Rivasseau added a comment - 17/Oct/08 05:55 AM
I am the original reporter. Happy to see the issue is somehow fixed - I am still a bit worried by the out-of-sync problem, but in my case I think it won't happen, since the Ownee will always be deleted (Shay seems to be saying that in this case another event will be thrown that will correctly process the reindexing). Would still be nice to have the better handling mentionned can be implemented.

Jean-Noel Rivasseau added a comment - 10/Nov/08 02:52 PM
Is the (current) fix in 2.1 GA ?

Shay Banon added a comment - 10/Nov/08 04:14 PM
Yes, the mid term fix has been applied so atleast the NPE won't break everything.