Add manually publishing cascade events in implementation of DAOs instead of using EntityListeners #3342

sleshchenko · 2016-12-09T16:08:10Z

What does this PR do?

Add:

rethrowing original exception while cascade operation
manually publishing cascade events in implementation of DAOs instead of using EntityListeners.
flushing of data by EntityManager#flush to produce consistency exception in right place.
CascadeEventLister that is able to throw any of Exception. If listener throws exception then all data changes should be canceled.
base events(PersistEvent, RemoveEvent, UpdateEvent). CascadeEventService throws only right exceptions for each use case. When thrown by listener exception should not be thrown by contract of this operation, then it will be wrapped into ServerException.

What issues does this PR fix or reference?

Previous behavior

Post[Persist|Remove|Update] events are thrown EntityListener, and it is not allowed to change(store new, remove or update existing) any objects on these events.
User is not able to see original exception while cascade removing.

New behavior

New approach allows us to perform any operations on post events or rollback transaction if is fails.
So post events must be thrown by implementations of DAOs. It allows to perform any operation on event and rollback transaction if one of subscriber fails.
To fails original operation event subscriber should extend CascadeEventSubscriber

voievodin · 2016-12-13T14:38:27Z

wsmaster/che-core-api-user/src/main/java/org/eclipse/che/api/user/server/jpa/JpaUserDao.java

-        return user;
-
+        final UserImpl user = manager.find(UserImpl.class, id);
+        if (user != null) {


i'd change it to

if (user == null) { return Optional.empty(); } eventService.publish(new BeforeUserRemovedEvent(user)); manager.remove(user); return Optional.of(user);

as Optional.ofNullable(user) causes another null check

voievodin

This approach won't propagate the original exception

mshaposhnik · 2016-12-22T13:16:22Z

core/che-core-db/src/main/java/org/eclipse/che/core/db/cascade/CascadeContext.java

-public class RemovalContext {
-    private Exception cause;
+public class CascadeContext {
+    private ApiException cause;


For me, REST-api oriented exception looks a bit strange here.

The same as throwing REST-api oriented exception by Dao. But we do it. :) And we save original REST-api oriented exception that is thrown by listener for rethrowing

mshaposhnik · 2016-12-22T13:44:03Z

...che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/jpa/JpaFactoryDao.java

@@ -159,7 +161,9 @@ protected FactoryImpl doUpdate(FactoryImpl update) throws NotFoundException {
        if (update.getWorkspace() != null) {
            update.getWorkspace().getProjects().forEach(ProjectConfigImpl::prePersistAttributes);
        }
-        return manager.merge(update);
+        FactoryImpl merged = manager.merge(update);
+        manager.flush();


Is those changes are necessary if we didn't publish any events here?

Yes, because this operation can be performed by listener. And then integrity exception will be catched in wrong place

I can hardly imagine the listener which will create factory in some case :)) But for purposes of uniformity it's ok.

voievodin · 2016-12-26T12:33:01Z

core/che-core-db/src/main/java/org/eclipse/che/core/db/cascade/CascadeEventSubscriber.java

+     * <p>If the method throws an exception it will be set to context
+     * to break event publishing and rethrow exception.
+     * Original exception can be rethrown to publisher
+     * or it can be wrapped in {@link SecurityException} instance.


SecurityException ?

Oooops 😄

codenvy-ci · 2017-01-10T18:28:14Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1563/

…ration as cascade. Also contains following changes: - Fixes rethrowing original exception while cascade operation. - Add manually publishing cascade events in implementation of DAOs instead of using EntityListeners. - Add flushing of data by EntityManager#flush to produce consistency exception in right place. - Add CascadeEventLister that is able to throw any of Exception. If listener throws exception then all data changes should be canceled. - Add base events(PersistEvent, RemoveEvent, UpdateEvent). CascadeEventService throws only right exceptions for each use case. When thrown by listener exception should not be thrown by contract of this operation, then it will be wrapped into ServerException.

codenvy-ci · 2017-01-12T18:32:54Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1606/

sleshchenko force-pushed the cascadeEvents branch 3 times, most recently from 31bdcd1 to 7aaeb81 Compare December 12, 2016 16:08

sleshchenko changed the title ~~[WIP] Replace manually publishing cascade operation instead of using EntityListeners~~ [WIP] Add manually publishing cascade events in implementation of DAOs instead of using EntityListeners Dec 12, 2016

sleshchenko force-pushed the cascadeEvents branch 2 times, most recently from f186ffa to 95c0cd1 Compare December 13, 2016 13:55

sleshchenko changed the title ~~[WIP] Add manually publishing cascade events in implementation of DAOs instead of using EntityListeners~~ Add manually publishing cascade events in implementation of DAOs instead of using EntityListeners Dec 13, 2016

voievodin reviewed Dec 13, 2016

View reviewed changes

akorneta approved these changes Dec 13, 2016

View reviewed changes

voievodin approved these changes Dec 13, 2016

View reviewed changes

sleshchenko mentioned this pull request Dec 13, 2016

Add manually publishing cascade events in implementation of DAOs instead of using EntityListeners codenvy/codenvy#1338

Merged

voievodin suggested changes Dec 14, 2016

View reviewed changes

sleshchenko force-pushed the cascadeEvents branch 3 times, most recently from 941287e to bae2759 Compare December 16, 2016 13:51

benoitf force-pushed the master branch from f0ee59a to aba9a87 Compare December 20, 2016 09:31

sleshchenko force-pushed the cascadeEvents branch 2 times, most recently from f22e6a6 to 3d5e5d5 Compare December 21, 2016 14:45

mshaposhnik reviewed Dec 22, 2016

View reviewed changes

sleshchenko requested a review from skabashnyuk December 26, 2016 12:00

voievodin reviewed Dec 26, 2016

View reviewed changes

sleshchenko force-pushed the cascadeEvents branch 8 times, most recently from 2076f6d to 8d6e87d Compare December 27, 2016 15:47

voievodin approved these changes Dec 27, 2016

View reviewed changes

akorneta approved these changes Dec 27, 2016

View reviewed changes

sleshchenko force-pushed the cascadeEvents branch 2 times, most recently from 064ed10 to f0bd904 Compare January 5, 2017 08:59

sleshchenko force-pushed the cascadeEvents branch 3 times, most recently from c77e373 to 77f1da4 Compare January 10, 2017 14:47

sleshchenko force-pushed the cascadeEvents branch 2 times, most recently from d9bf21c to 2200464 Compare January 12, 2017 13:53

skabashnyuk approved these changes Jan 12, 2017

View reviewed changes

sleshchenko added this to the 5.1.0 milestone Jan 12, 2017

sleshchenko force-pushed the cascadeEvents branch 4 times, most recently from a61907b to 2289e59 Compare January 12, 2017 15:15

sleshchenko force-pushed the cascadeEvents branch from 2289e59 to 359bd73 Compare January 12, 2017 15:15

sleshchenko merged commit 0675a62 into eclipse-che:master Jan 12, 2017

sleshchenko deleted the cascadeEvents branch January 12, 2017 16:06

JamesDrummond mentioned this pull request Jan 31, 2017

5.1.0 Milestone Overview #3972

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add manually publishing cascade events in implementation of DAOs instead of using EntityListeners #3342

Add manually publishing cascade events in implementation of DAOs instead of using EntityListeners #3342

sleshchenko commented Dec 9, 2016 •

edited

Loading

voievodin Dec 13, 2016 •

edited

Loading

voievodin left a comment

mshaposhnik Dec 22, 2016

sleshchenko Dec 22, 2016 •

edited

Loading

mshaposhnik Dec 22, 2016

sleshchenko Dec 22, 2016

mshaposhnik Dec 22, 2016

voievodin Dec 26, 2016

sleshchenko Dec 26, 2016

codenvy-ci commented Jan 10, 2017

codenvy-ci commented Jan 12, 2017

Add manually publishing cascade events in implementation of DAOs instead of using EntityListeners #3342

Add manually publishing cascade events in implementation of DAOs instead of using EntityListeners #3342

Conversation

sleshchenko commented Dec 9, 2016 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Previous behavior

New behavior

voievodin Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

voievodin left a comment

Choose a reason for hiding this comment

mshaposhnik Dec 22, 2016

Choose a reason for hiding this comment

sleshchenko Dec 22, 2016 • edited Loading

Choose a reason for hiding this comment

mshaposhnik Dec 22, 2016

Choose a reason for hiding this comment

sleshchenko Dec 22, 2016

Choose a reason for hiding this comment

mshaposhnik Dec 22, 2016

Choose a reason for hiding this comment

voievodin Dec 26, 2016

Choose a reason for hiding this comment

sleshchenko Dec 26, 2016

Choose a reason for hiding this comment

codenvy-ci commented Jan 10, 2017

codenvy-ci commented Jan 12, 2017

sleshchenko commented Dec 9, 2016 •

edited

Loading

voievodin Dec 13, 2016 •

edited

Loading

sleshchenko Dec 22, 2016 •

edited

Loading