Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DATAMONGO-440 - implemented cascade operations on @DBRef fields #3

Closed

Conversation

maciejwalkowiak
Copy link

Pull request contains implementation of cascading operations: save and delete on fields annotated with @DBREF. Implementation details:

  • added new event: AfterDeleteEvent invoked in MongoTemplate after successful object removal
  • added @DBREF#cascadeType property
  • added CascadingMongoEventListener that intercepts objects before they are converted for save operation and before they are deleted for delete operation if appropriate @DBREF#cascadeType is set.
  • in order to emit AfterDeleteEvent MongoOperations#remove methods had to be generified

This implementation is inspired by my research about cascading in Spring Data MongoDB described on blog: http://maciejwalkowiak.pl/blog/2012/04/30/spring-data-mongodb-cascade-save-on-dbref-objects/ where i found that it can be done now by implementing custom event listener but without internal framework changes its a little bit hacky (used reflection instead of Spring Data persistent-entity-like classes, additional annotation, impossible to implement cascading on delete).

If you like this idea and you have suggestions how this implementation can be improved i would be happy to make appropriate changes

@@ -850,16 +845,18 @@ public WriteResult doInCollection(DBCollection collection) throws MongoException
});
}

public void remove(Object object) {
public <T> void remove(T object) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the benefit of adding generics here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to emit generic AfterDeleteEvent. CascadingMongoEventListener catches events for all types of objects so generics are not used in this case but if someone would want to create his own listener for particular type then generics are needed.

@odrotbohm
Copy link
Member

Looks quite cool again. Here are some idea's looking at the code:

  1. It might make sense to have the Cascade enum in Spring Data Commons and expose the value through the Association interface directly. I can handle this when merging your pull request back in. So no need to separate this, just a request for what you think.
  2. Just like the ValidatingMongoEventListener I'd like to see this being activated through the <mongo:mapping-converter /> element by default. Thus this has to be added to the BeanDefinitionParser. Beyond that the test cases will probably have to use the plain bean based configuration then to test the stuff in isolation.
  3. What about cyclic references? I think we'd be running into a stack overflow currently. We should either handle this case correctly or at least detect a cyclic invocation and throw an appropriate exception. We'll probably have to track the entities handled in a ThreadLocal in the event handler.

/cc @markpollack @trisberg @jbrisbin

@maciejwalkowiak
Copy link
Author

Thanks for feedback. I will handle your ideas/questions in couple of days (when i am back from Geecon conference)

@krmcbride
Copy link

Hey there. I'm a long time Doctrine Mongo ODM user and have been using Spring's Mongo support for all of 24 hours now and thought I'd share two areas of concern related to this topic:

First, this PR doesn't appear to support cascading operations on Collections. Thankfully it shouldn't be hard to add. I'd argue that you have to either support cascades across the board or not at all. If you try to explain to someone that cascades only work on 1:1 and not 1:M they'll just look at you cross-eyed.

Secondly, and more importantly, there's a serious limitation in AbstractMongoEventListener in that it only passes the persistent object itself to the handler methods rather than an event object. @maciejwalkowiak injects the MongoTemplate to perform additional operations on the object graph....but what if I have multiple Mongo connections configured? It would be better to have a BeforeConvertEvent object passed to onBeforeConvert which would have methods such as getObject and getMongoTemplate. From the moment the event is fired internally a reference to the acting connection needs to be passed along as well. That way there's no chance of writing to two or more databases during a single lifecycle event (it would be a hilarious trick to play on your friends, but unwanted nonetheless).

Like I said, I've been using this lib for less than a day so I apologize if I'm just missing something.

@odrotbohm
Copy link
Member

+1 on the collections support.

Regarding the problem of accessing a different database I agree in terms of the symptom but not on the solution side of things. First one doesn't have to extend AbstractMongoEventListener. Any ApplicationEventListener would pretty much do the job. The only reason this class exists is to ease the way to define listeners by entity type. I don't think putting the template into the event has any benefit at all. First, the events should be serializable which they wouldn't be if we added the template instance to it. Second, the tempalte is using a MongoDbFactory to obtain connections to a particular database. Thus the access to the template only gives you access to the single database anyway. So we probably have to get hold of a MongoDbFactory to be wired into the listener as well as a reference to the current converter and create a MongoTemplate instance on the fly.

@maciejwalkowiak
Copy link
Author

Collections support added. I will develop missing stuff hopefully today evening and tomorrow.

@cokila
Copy link

cokila commented Apr 4, 2013

what is the status of this issue ?

@paivaric
Copy link

+1

3 similar comments
@acactown
Copy link

+1

@romain-rossi
Copy link

+1

@whummer
Copy link

whummer commented Aug 5, 2014

+1

@tracyrobinson
Copy link

I too am searching for the latest status on this issue.

@Redhab
Copy link

Redhab commented Sep 7, 2014

+1
Any status on this PR?

@pixelcat
Copy link

+1 on this as well. Looks like there are merge conflicts?

@gembin
Copy link

gembin commented Dec 11, 2014

+1

2 similar comments
@Loki-Afro
Copy link

+1

@chembohuang
Copy link

+1

@xiaolongyuan
Copy link

+1024

@Chumper
Copy link

Chumper commented Jun 10, 2015

👍

2 similar comments
@milehimikey
Copy link

+1

@timbrauser
Copy link

+1

@huangered
Copy link

+1

2 similar comments
@aleuchanka
Copy link

+1

@odiszapc
Copy link
Contributor

+1

@maciejwalkowiak
Copy link
Author

Apologies to all who requested this feature, but I am closing it without merging for two reasons:

  • it has never been finished. It took me a while before I found time for adding requested fix for handling cyclic references, but it was anyway too late - some internal things have changed (like AfterDeleteEvent added in another pull request, incompatible with what I made) making this pull request impossible to merge.
  • it wasn't good idea anyway. I realised it after a while, but if you need cascades for Mongo, there is a high chance that something is wrong with design (there was with mine).

If you are not convinced and you would like to pick up my branch, rebase against master and fix everything what's broken - go for it, but I suggest to discuss it first with Spring Data team.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet