rowe42 / lhm_animad_admin_html5

0 stars 6 forks source link

"Echten" Cache in authorisationLib einbauen #212

Closed rowe42 closed 6 years ago

rowe42 commented 6 years ago

Die authorisationLib wird in jeden Microservice eingebettet und verwendet, um die Permissions vom User-Service (und damit letztlich vom KeyCloak) abzuholen. Damit das nicht bei jedem Aufruf des MS aufs neue erfolgt, sollten die Rechte gecached werden (mit dem Access-Token als Schlüssel, damit wir die Permissions trotzdem regelmäßig - also bei jedem Refresh - neu holen). Das ist auch schon implementiert, allerdings nur rudimentär als HashMap. Das heißt aber, dass der Cache vollläuft, da alle Key-Value-Pairs ewig dort verbleiben. Sollte also durch irgendeine Cache-Implementierung (EHCache?) ersetzt werden.

@xdoo @dragonfly28 @ejcsid @Baumfrosch

rowe42 commented 6 years ago

Habe nun einen EHCache eingebaut und auf 7 Minuten Lebenszeit der Einträge gestellt. Sollte man anpassen, wenn wir wissen, wie lange die Access-Token "leben" dürfen.

rowe42 commented 6 years ago

Zu früh gefreut: EHCache verträgt sich nicht mit Admin-Service. Habe ihn erstmal wieder entfernt, damit es wieder läuft.

rowe42 commented 6 years ago

Also: Ich habe jetzt mit 3 Cache-Varianten experimentiert: GuavaCache (Standard, aber deprecated), Caffeine (Neuimplementierung von GuavaCache) und EHCache. Nur beim GuavaCache werden die vom Admin-Service benötigten Caches (KEEPER_CACHE, ANIMAL_CACHE, ENCLOSURES_CACHE) on-the-fly erzeugt. Bei den anderen beiden musste ich diese Caches von Hand erzeugen, was natürlich wieder eine neue Aufgabe für den Generator bedeuten würde.

Habe jetzt für Admin-Service und AuthorisationLib einen neuen Branch _#212 gemacht und eine Version eingecheckt, in der alle 3 Caches funktionieren, jedoch EHCache und Caffeine sind auskommentiert. So kann man sich später noch überlegen, welchen man nimmt oder die on-the-fly Generierung noch ergänzen.

Mache dann später noch einen PR.

rowe42 commented 6 years ago

Ich kriege das leider einfach nicht hin. Mir ist aufgefallen, dass ich die folgende Fehlermeldung bekomme, wenn ich zuerst einen Keeper speichere und dann ein Animal, das eine Relation zu diesem hat:

org.hibernate.PersistentObjectException: detached entity passed to persist: de.muenchen.animad.admin.administration.service.gen.domain.Keeper_

Das passiert aber NUR, wenn ich in meiner Klasse folgendes verwende:

@Configuration
@EnableCaching
public class PermissionsCache {

    @Autowired
    private CacheManager cacheManager;
...

(also EnableCaching in Kombination mit einem autowired CacheManager)

@FabianWilms Habt ihr vielleicht im Rahmen von Barrakuda Erfahrung damit gemacht und wisst, wo das Problem ist? Oder @a52Team kennt ihr das Problem?

xdoo commented 6 years ago

Wo ist denn die Klasse in der du den Cache konfiguriert hast? Schön wäre auch ein Klasse, wo du das verwendet hast.

rowe42 commented 6 years ago

@xdoo: Die Klasse ist hier: https://github.com/rowe42/lhm_authorisationLib/blob/_%23212/src/main/java/de/muenchen/commons/authorisation/PermissionsCache.java (also im Branch _#212 in der authorisationLib)

ABER: Ich habe mittlerweile den Verdacht, dass das gar nichts mit meinem Code zu tun hat sondern ein Bug im Generat ist. Das Problem tritt nämlich bereits auf, wenn man "irgendwo" (z.B. in MicroServiceApplication.java) die Annotation @EnableCaching (org.springframework.cache.annotation.EnableCaching) ergänzt. Der Fehler lässt sich danach so nachstellen:

Es scheint also so, als dass in Barrakuda zwar die Caching-Annotations an die Methoden gehängt wurden (z.B. in Klasse Generated_Animal_Repository.java), aber diese nicht aktiviert sind, so lange man nicht auch @EnableCaching hat (vgl. hierzu auch Spring-Doku: https://docs.spring.io/spring-boot/docs/current/reference/html/boot-features-caching.html). Oder anders formuliert: Ggf. hat das noch nie funktioniert...

rowe42 commented 6 years ago

Ich habe eine mögliche Lösung bei Google gefunden: https://stackoverflow.com/questions/13370221/jpa-hibernate-detached-entity-passed-to-persist

Da steht, man solle bei der Relation CascadeType.MERGE verwenden.

Das wäre dann in Klasse: https://github.com/xdoo/lhm_animad_admin_service/blob/master/animad-service-administration/animad-administration-service/src/main/java/de/muenchen/animad/admin/administration/service/gen/domain/Animal_.java

Wenn man dort folgendes anpasst: ALT:

    @ManyToMany(cascade = {CascadeType.PERSIST, CascadeType.MERGE})
...
    private java.util.List<Keeper_> keeperList = new java.util.ArrayList<>();

NEU:

    @ManyToMany(cascade = {CascadeType.MERGE})
...
    private java.util.List<Keeper_> keeperList = new java.util.ArrayList<>();

scheint es zu funktioneren. Ist die Lösung wirklich so einfach?

FabianWilms commented 6 years ago

Also bringt der aktivierte Cache die Hibernate Repräsentation aus dem Tritt? Wenn du die neue Konfiguration nutzt, ist es dann noch möglich einen Keeper neu zu erstellen, wenn du ein Animal erstellst? Eventuell ist hier auch nur die Reihenfolge der CascadeTypes in der Annotation wichtig?

xdoo commented 6 years ago

Wir sollten das Problem nochmal von vorne analysieren. Was ich noch nicht verstanden habe: Weicht unsere Konfiguration von der Vorgeschlagenen ab und wenn ja warum. In den Javadocs steht bei @EnableCaching folgende Konfiguration:

 @Configuration
 @EnableCaching
 public class AppConfig {

     @Bean
     public MyService myService() {
         // configure and return a class having @Cacheable methods
         return new MyService();
     }

     @Bean
     public CacheManager cacheManager() {
         // configure and return an implementation of Spring's CacheManager SPI
         SimpleCacheManager cacheManager = new SimpleCacheManager();
         cacheManager.setCaches(Arrays.asList(new ConcurrentMapCache("default")));
         return cacheManager;
     }
 }

https://docs.spring.io/spring/docs/current/javadoc-api/org/springframework/cache/annotation/EnableCaching.html

Die konnte ich so nicht finden. Wenn wir es so nicht machen, stellt sich für mich die Frage warum.

rowe42 commented 6 years ago

@FabianWilms Habe es nochmal getestet:

Variante ausschließlich CascadeType.MERGE: Es funktioniert alles: Keeper anlegen, Animal mit gerade angelegtem Keeper anlegen, weiteren Keeper anlegen (und sogar mit diesem Keeper erneut ein Animal anlegen - allerdings muss man hier einmal Browser-refresh machen; da habe ich wohl einen Bug in die Relationen eingebaut)

Variante mit CascadeType.MERGE, CascadeType.PERSIST: (also umgedreht) Oben beschriebener Usecase funktioniert nicht.

xdoo commented 6 years ago

Ich bin mir nicht sicher, dass das so einfach geht. Wenn man ein Objekt neu anlegt, dann hat es den Status "transient". Um es in den Status "managed" zu bekommen, muss man die Methode persist nutzen (deswegen ist @FabianWilms Frage völlig berechtigt). Wenn man nun Änderungen an einem Objekt vornimmt, dann hat es den Status "detached". Um hier wiederum das Objekt in den Status "managed" zu bekommen, nimmt man die Methode merge. Betrachtet man dieses Konstrukt auf einer theoretischen Ebene, so müsste eigentlich folgende Annotation korrekt sein:

@ManyToMany(cascade = {CascadeType.PERSIST, CascadeType.MERGE})
...
    private java.util.List<Keeper_> keeperList = new java.util.ArrayList<>();
rowe42 commented 6 years ago

Extra für @xdoo habe ich es nochmal ausprobiert. So habe ich die Klasse MicroServiceApplication.java verändert:

@EnableCaching
public class MicroServiceApplication {

...    
     @Bean
     public CacheManager cacheManager() {
         // configure and return an implementation of Spring's CacheManager SPI
         SimpleCacheManager cacheManager = new SimpleCacheManager();
         cacheManager.setCaches(Arrays.asList(new ConcurrentMapCache("ANIMAL_CACHE"), 
                 new ConcurrentMapCache("ENCLOSURE_CACHE"), 
                 new ConcurrentMapCache("KEEPER_CACHE")));
         return cacheManager;
     }    
}

(die Caches müssen dann namentlich erstellt werden, weil mit dem o.g. Code das Anlegen on-the-fly abgeschaltet wird)

Ergebnis: Gleiches Problem. Ich behaupte es ist egal, welchen Cache wir verwenden. Das Problem kommt, sobald wir @CachingEnabled hinzufügen. Und es geht weg, wenn man CascaseType.PERSIST von der keeperList entfernt.

Jetzt wäre es gut, wenn ihr euch noch Gedanken macht, wozu das CascadeType.PERSIST gut war. Und wenn es keine guten Gründe gibt, nehmen wir es weg.

xdoo commented 6 years ago

Ich glaube eher, dass es am Transaktionsmanagement liegt. Was der Nutzer Zds im oben verlinkten Stackoverflow Therad sagt klingt für mich sehr einleuchtend (ohne das uns das direkt zu einer Lösung führt.):

Using merge is risky and tricky, so it's a dirty workaround in your case. You need to remember at least that when you pass an entity object to merge, it stops being attached to the transaction and instead a new, now-attached entity is returned. This means that if anyone has the old entity object still in their possession, changes to it are silently ignored and thrown away on commit.

You are not showing the complete code here, so I cannot double-check your transaction pattern. One way to get to a situation like this is if you don't have a transaction active when executing the merge and persist. In that case persistence provider is expected to open a new transaction for every JPA operation you perform and immediately commit and close it before the call returns. If this is the case, the merge would be run in a first transaction and then after the merge method returns, the transaction is completed and closed and the returned entity is now detached. The persist below it would then open a second transaction, and trying to refer to an entity that is detached, giving an exception. Always wrap your code inside a transaction unless you know very well what you are doing.

FabianWilms commented 6 years ago

Ganz kurze Frage am Rande: Mit Barrakuda wird aktuell ja bereits ein Caching generiert, welches ohne Probleme funktioniert:

https://github.com/xdoo/lhm_animad_admin_service/blob/master/animad-service-administration/animad-administration-service/src/main/java/de/muenchen/animad/admin/administration/service/gen/rest/Generated_Animal_Repository.java (siehe Annotationen @Cacheable, @CachePut, @CacheEvict)

Eventuell rührt das Problem ja daher, dass wir jetzt durch die Änderung zwei Caches Parallel laufen haben? :sweat_smile:

rowe42 commented 6 years ago

@FabianWilms wie schon weiter oben geschrieben : ich glaube der aktuell in Barrakuda eingebaute Cache ist inaktiv weil die Annotation CachingEnabled fehlt. Das Problem tritt auf sobald man die Annotation ergänzt, auch wenn sonst gar kein neuer Code dabei ist...

FabianWilms commented 6 years ago

Ah, das habe ich eben überlesen. Dann nehme ich meine Vermutung zurück. Nach dem Tutorial gehend (https://spring.io/guides/gs/caching/) müsste dann eigentlich alles passen...

rowe42 commented 6 years ago

Ja, nur dass das tutorial so lästige Details wie referenzierte Entitäten nicht behandelt :-)

FabianWilms commented 6 years ago

Eine Bitte noch für einen weiteren Versuch:

Das Caching sollte eigentlich ja nicht in die Transaktionslogik reinpfuschen. Also scheint es mir, als würde folgendes passieren:

Fall: Du speicherst ein neues Animal mit einem bestehenden Keeper

  1. Der Keeper wird gelesen, der Cache des KeeperRepository greift
  2. Keeper aus dem Cache != Keeper aus JPA
  3. Keeper detached! Fehler tritt bei Persist auf

Kannst du mal ausprobieren, die @Cachexxx-Annotationen aus dem Keeper-Repository zu löschen, den CascadeType wieder auf die Ursprünglichen Einstellungen setzen und den oben beschriebenen Fall durchzuführen? Wenn es dann funktioniert wissen wir zumindest wo der Fehler liegt.

rowe42 commented 6 years ago

@FabianWilms OK habe es getestet: Wenn man die @Cachexxx-Annotationen in Klasse Generated_Keeper_Repository auskommentiert, aber in der Klasse MicroServiceApplication.java die Annotation @EnableCaching drin hat, funktioniert es.

FabianWilms commented 6 years ago

Dann ist zumindest geklärt, warum es nicht funktioniert und warum CascadeType.MERGE abhilfe schafft. Ich muss @xdoo allerdings bzgl. seiner Bedenken (https://github.com/xdoo/lhm_animad_admin_html5/issues/212#issuecomment-369839296) recht geben :/

rowe42 commented 6 years ago

Ok, in Abstimmung mit @xdoo nehmen wir dann die Lösung mit CascadeType.MERGE

rowe42 commented 6 years ago

Ist leider auch nicht die Lösung: Wenn man ein bestehendes Animal aufruft, kommt folgende Fehlermeldung:

org.hibernate.LazyInitializationException: failed to lazily initialize a collection of role: de.muenchen.animad.admin.administration.service.gen.domain.Animal_.keeperList, could not initialize proxy - no Session

Dabei ist es total egal, welcher CascadeType bei KeeperList gesetzt ist... :-(

FabianWilms commented 6 years ago

FetchType.EAGER ausprobiert? Das sollte nämlich weder ein Problemd es Cache, noch des CascadeType sein.

rowe42 commented 6 years ago

Macht keinen großen Unterschied. Die Fehlermeldung ist kein Stack-Trace mehr, aber liest sich sehr ähnlich:

2018-03-05 15:23:12.734 WARN 6783 --- [io-39146-exec-8] .w.s.m.s.DefaultHandlerExceptionResolver : Failed to write HTTP message: org.springframework.http.converter.HttpMessageNotWritableException: Could not write content: failed to lazily initialize a collection of role: de.muenchen.animad.admin.administration.service.gen.domain.Keeper_.skill, could not initialize proxy - no Session (through reference chain: org.springframework.hateoas.Resources["_embedded"]->java.util.UnmodifiableMap["keepers"]->java.util.ArrayList[0]->org.springframework.data.rest.webmvc.json.["content"]->de.muenchen.animad.admin.administration.service.gen.domain.Keeper_["skill"]); nested exception is com.fasterxml.jackson.databind.JsonMappingException: failed to lazily initialize a collection of role: de.muenchen.animad.admin.administration.service.gen.domain.Keeper_.skill, could not initialize proxy - no Session (through reference chain: org.springframework.hateoas.Resources["_embedded"]->java.util.UnmodifiableMap["keepers"]->java.util.ArrayList[0]->org.springframework.data.rest.webmvc.json.["content"]->de.muenchen.animad.admin.administration.service.gen.domain.Keeper_["skill"])

rowe42 commented 6 years ago

Irgendwie indirekt hat es ja schon mit dem Cache zu tun - denn der Fehler kommt erst, wenn ich den Cache einschalte (selbst wenn ich alles andere so lasse wie es ist).

FabianWilms commented 6 years ago

Jetzt fehlt die gleiche Annotation noch beim Skill der Keepers:

failed to lazily initialize a collection of role: de.muenchen.animad.admin.administration.service.gen.domain.Keeper_.skill

Inwiefern jetzt der Cache die Problemen produziert erschließt sich mir nicht ganz...wahrscheinlich versucht die Cache-Implementierung das gesamte Objekt Animal zu Cachen und produziert dann den Lazy-Loading Fehler beim lesen der Keepers? Nur eine Vermutung.

rowe42 commented 6 years ago

Du hast recht! Hätte ich genau lesen sollen... Mit EAGER zusätzlich bei den Keeper-Skills und der Enclosure-AnimalList scheint es jetzt zu funktionieren!

xdoo commented 6 years ago

Wir sollten da aber etwas vorsichtig sein. "Eager" bedeutet ja, dass alle Abhängigkeiten geladen werden. Wenn wir das jetzt an alle "toMany" Beziehungen dran kleben, dann kann es passieren, dass wir bei einem entsprechenden Modell durch eine Aktion die komplette DB in den Speicher laden. Das haben wir nicht im Griff. Wenn wir das Caching nicht ohne FetchType EAGER hin bekommen, dann sollten wir diskutieren, ob wir es nicht besser lassen.

FabianWilms commented 6 years ago

Dazu vielleicht noch folgender Lösungsvorschlag: https://stackoverflow.com/a/36088992/3351543

Habe mit dem Hibernate Cache allerdings noch nicht gearbeitet.

xdoo commented 6 years ago

Ich werde versuchen das Problem in einer kleinen Anwendung nachzustellen und versuchen eine Lösung zu finden.

xdoo commented 6 years ago

@rowe42 ich habe unsere Konstellation mal in einem kleinen Showcase nachgebaut:

https://github.com/xdoo/spring_data_jpa_cache_demo

Anhand dieses Threads kann ich nicht genau nachvollziehen, was du genau getestet hast, dass es zu diesem Fehler gekommen ist. Ich konnte zumindest den Fehler mit meinen Test nicht nachstellen. Schau dir doch mal das Demo an und bau ggf. einen Testfall dazu, der das abdeckt, was du gemacht hast.

Ich konnte in dem "admin service" Repo keinen

        <dependency>
            <groupId>org.springframework.boot</groupId>
            <artifactId>spring-boot-starter-cache</artifactId>
        </dependency>

finden. Das könnte eine Ursache der Probleme sein.

Zu meinen "Tests":

    ...
    @Test
    public void saveAnimal() {
        Animal animal = this.createAnimal();
        Animal saved = this.animalRepo.save(animal);
        this.checkSave();
        for (int i = 0; i < 100; i++) {
            Animal find = this.animalRepo.findOne(saved.getOid());
            assertNotNull(find);
        }
    }
   ...

Der spannende Teil findet da nicht im Test an sich statt (dort wird schon geprüft, ob die Objekte in der DB angekommen sind), sondern in den Log Ausgaben. Dort kann man an den SQL Statements schön sehen, dass die Objekte angelegt werden (INSERT Statements), danach kommen noch zwei SELECTS, um zu prüfen, ob die Entitäten wirklich in der DB sind. Danach kommt nichts mehr. Das liegt daran, dass wir bereits auf der Methode save einen @CachePut Annotation haben, die das Objekt in den Cache legt. Die 100 lesenden Aufrufe gehen dann immer auf den Cache. Das dies wirklich so ist läßt sich relativ einfach belegen, indem man die @Cachable Annotation auf der findOne Methode auskommentiert. Hier die Logausgaben:

2018-03-08 07:46:08.273 DEBUG 10267 --- [           main] org.hibernate.SQL                        : insert into animal (name, oid) values (?, ?)
2018-03-08 07:46:08.286 DEBUG 10267 --- [           main] org.hibernate.SQL                        : insert into keeper (name, oid) values (?, ?)
2018-03-08 07:46:08.287 DEBUG 10267 --- [           main] org.hibernate.SQL                        : insert into keeper (name, oid) values (?, ?)
2018-03-08 07:46:08.288 DEBUG 10267 --- [           main] org.hibernate.SQL                        : insert into animal_keeper_list (animal_oid, order_index, keeper_list_oid) values (?, ?, ?)
2018-03-08 07:46:08.289 DEBUG 10267 --- [           main] org.hibernate.SQL                        : insert into animal_keeper_list (animal_oid, order_index, keeper_list_oid) values (?, ?, ?)
2018-03-08 07:46:08.313  INFO 10267 --- [           main] o.h.h.i.QueryTranslatorFactoryInitiator  : HHH000397: Using ASTQueryTranslatorFactory
2018-03-08 07:46:08.407 DEBUG 10267 --- [           main] org.hibernate.SQL                        : select count(*) as col_0_0_ from animal animal0_
2018-03-08 07:46:08.414 DEBUG 10267 --- [           main] org.hibernate.SQL                        : select count(*) as col_0_0_ from keeper keeper0_
Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 4.918 sec - in com.example.cachedemo.CacheDemoApplicationTests
rowe42 commented 6 years ago

@xdoo Ich habe dir einen Branch "sequentiell" in deinen Showcase hochgeladen. Das Problem entsteht dann, wenn man zunächst einen Keeper speichert und danach versucht, ein Animal mit Referenz zu diesem Keeper zu speichern. In dem neuen Branch ist dazu ein Testfall implementiert. Ein bisschen rumprobieren hat gezeigt, dass das Ganze nichts mit dem Caching zu tun hat, sondern auch passiert, wenn man alle Cache*-Annotationen wegnimmt. Es funktioniert dann wieder, wenn man das CascadeType.PERSIST wegnimmt.

xdoo commented 6 years ago

@rowe42 Ich kann den nicht sehen. Bist du sicher, dass der nicht nur lokal bei dir ist? :)

xdoo commented 6 years ago

Ok. Konnte das mit diesem Test nachstellen:

    @Test
    public void testReferenceKeeper() {
        Keeper torben = this.keeperRepo.save(new Keeper("Torben"));
        Animal nemo = new Animal("Nemo", Lists.newArrayList(torben));
        this.animalRepo.save(nemo);
    }

Das führt zu:

testReferenceKeeper(com.example.cachedemo.CacheDemoApplicationTests)  Time elapsed: 0.128 sec  <<< ERROR!
org.springframework.dao.InvalidDataAccessApiUsageException: detached entity passed to persist: com.example.cachedemo.model.Keeper; nested exception is org.hibernate.PersistentObjectException: detached entity passed to persist: com.example.cachedemo.model.Keeper
    at org.hibernate.event.internal.DefaultPersistEventListener.onPersist(DefaultPersistEventListener.java:124)
    at org.hibernate.internal.SessionImpl.firePersist(SessionImpl.java:765)
    at org.hibernate.internal.SessionImpl.persist(SessionImpl.java:758)
    at org.hibernate.jpa.event.internal.core.JpaPersistEventListener$1.cascade(JpaPersistEventListener.java:80)
    at org.hibernate.engine.internal.Cascade.cascadeToOne(Cascade.java:398)
    at org.hibernate.engine.internal.Cascade.cascadeAssociation(Cascade.java:323)
    at org.hibernate.engine.internal.Cascade.cascadeProperty(Cascade.java:162)
    at org.hibernate.engine.internal.Cascade.cascadeCollectionElements(Cascade.java:431)
    at org.hibernate.engine.internal.Cascade.cascadeCollection(Cascade.java:363)
    at org.hibernate.engine.internal.Cascade.cascadeAssociation(Cascade.java:326)
    at org.hibernate.engine.internal.Cascade.cascadeProperty(Cascade.java:162)
    at org.hibernate.engine.internal.Cascade.cascade(Cascade.java:111)
    at org.hibernate.event.internal.AbstractSaveEventListener.cascadeAfterSave(AbstractSaveEventListener.java:456)
    at org.hibernate.event.internal.AbstractSaveEventListener.performSaveOrReplicate(AbstractSaveEventListener.java:278)
    at org.hibernate.event.internal.AbstractSaveEventListener.performSave(AbstractSaveEventListener.java:178)
    at org.hibernate.event.internal.AbstractSaveEventListener.saveWithGeneratedId(AbstractSaveEventListener.java:121)
    at org.hibernate.jpa.event.internal.core.JpaPersistEventListener.saveWithGeneratedId(JpaPersistEventListener.java:67)
    at org.hibernate.event.internal.DefaultPersistEventListener.entityIsTransient(DefaultPersistEventListener.java:189)
    at org.hibernate.event.internal.DefaultPersistEventListener.onPersist(DefaultPersistEventListener.java:132)
    at org.hibernate.event.internal.DefaultPersistEventListener.onPersist(DefaultPersistEventListener.java:58)
    at org.hibernate.internal.SessionImpl.firePersist(SessionImpl.java:775)
    at org.hibernate.internal.SessionImpl.persist(SessionImpl.java:748)
    at org.hibernate.internal.SessionImpl.persist(SessionImpl.java:753)
    at org.hibernate.jpa.spi.AbstractEntityManagerImpl.persist(AbstractEntityManagerImpl.java:1146)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at org.springframework.orm.jpa.SharedEntityManagerCreator$SharedEntityManagerInvocationHandler.invoke(SharedEntityManagerCreator.java:298)
    at com.sun.proxy.$Proxy93.persist(Unknown Source)
    at org.springframework.data.jpa.repository.support.SimpleJpaRepository.save(SimpleJpaRepository.java:508)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at org.springframework.data.repository.core.support.RepositoryFactorySupport$QueryExecutorMethodInterceptor.executeMethodOn(RepositoryFactorySupport.java:513)
    at org.springframework.data.repository.core.support.RepositoryFactorySupport$QueryExecutorMethodInterceptor.doInvoke(RepositoryFactorySupport.java:498)
    at org.springframework.data.repository.core.support.RepositoryFactorySupport$QueryExecutorMethodInterceptor.invoke(RepositoryFactorySupport.java:475)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179)
    at org.springframework.data.projection.DefaultMethodInvokingMethodInterceptor.invoke(DefaultMethodInvokingMethodInterceptor.java:56)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179)
    at org.springframework.transaction.interceptor.TransactionInterceptor$1.proceedWithInvocation(TransactionInterceptor.java:99)
    at org.springframework.transaction.interceptor.TransactionAspectSupport.invokeWithinTransaction(TransactionAspectSupport.java:282)
    at org.springframework.transaction.interceptor.TransactionInterceptor.invoke(TransactionInterceptor.java:96)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179)
    at org.springframework.dao.support.PersistenceExceptionTranslationInterceptor.invoke(PersistenceExceptionTranslationInterceptor.java:136)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179)
    at org.springframework.data.jpa.repository.support.CrudMethodMetadataPostProcessor$CrudMethodMetadataPopulatingMethodInterceptor.invoke(CrudMethodMetadataPostProcessor.java:133)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179)
    at org.springframework.aop.interceptor.ExposeInvocationInterceptor.invoke(ExposeInvocationInterceptor.java:92)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179)
    at org.springframework.data.repository.core.support.SurroundingTransactionDetectorMethodInterceptor.invoke(SurroundingTransactionDetectorMethodInterceptor.java:57)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179)
    at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:213)
    at com.sun.proxy.$Proxy97.save(Unknown Source)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:333)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.invokeJoinpoint(ReflectiveMethodInvocation.java:190)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:157)
    at org.springframework.aop.interceptor.AbstractTraceInterceptor.invoke(AbstractTraceInterceptor.java:132)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179)
    at org.springframework.cache.interceptor.CacheInterceptor$1.invoke(CacheInterceptor.java:52)
    at org.springframework.cache.interceptor.CacheAspectSupport.invokeOperation(CacheAspectSupport.java:342)
    at org.springframework.cache.interceptor.CacheAspectSupport.execute(CacheAspectSupport.java:405)
    at org.springframework.cache.interceptor.CacheAspectSupport.execute(CacheAspectSupport.java:324)
    at org.springframework.cache.interceptor.CacheInterceptor.invoke(CacheInterceptor.java:61)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179)
    at org.springframework.aop.interceptor.ExposeInvocationInterceptor.invoke(ExposeInvocationInterceptor.java:92)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179)
    at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:213)
    at com.sun.proxy.$Proxy97.save(Unknown Source)
    at com.example.cachedemo.CacheDemoApplicationTests.testReferenceKeeper(CacheDemoApplicationTests.java:51)
rowe42 commented 6 years ago

Ja, so ähnlich habe ich es auch gemacht. Der Branch ist nicht bei dir, weil ich offenbar keine Berechtigung habe Branches hochzuladen. Hatte die Fehlermeldung übersehen...

xdoo commented 6 years ago

Ok. Ich konnte das jetzt nachstellen:

    @Test
    public void testReferenceKeeper() {
        Keeper torben = this.keeperRepo.save(new Keeper("Torben"));
        Animal nemo = this.animalRepo.save(new Animal("Nemo", Lists.newArrayList(torben)));
        assertNotNull(nemo);
    }

funktioniert, wenn die Liste in Animal so aussieht:

@Data
@Entity
@Table(name = "Animal")
@NoArgsConstructor
@AllArgsConstructor
public class Animal extends BaseEntity {

    @Column(name = "name")
    String name;

    @OrderColumn(name = "order_index")
    @JoinTable(name = "Animal_KeeperList", joinColumns = {
        @JoinColumn(name = "animal_oid")}, inverseJoinColumns = {
        @JoinColumn(name = "keeperList_oid")})
    @ManyToMany(cascade = {CascadeType.MERGE})
    List<Keeper> keepers = new ArrayList<>();

}

Eine weitere Möglichkeit wäre, die ID nicht autogenerieren zu lassen. Dann funktioniert auch @ManyToMany(cascade = {CascadeType.PERSIST, CascadeType.MERGE}).

Wie wollen wir es machen, @rowe42 @FabianWilms? Der Code dazu ist im Repo.

rowe42 commented 6 years ago

Die ID selbst generieren finde ich unschön, wenn das auch automatisiert ginge. Selbst wenn wir einfach UUIDs nehmen.

Andererseits kann ich nicht einschätzen wie dramatisch die Probleme mit MERGE sind, die du weiter oben rein kopiert hattest. Sind das Probleme die uns konkret betreffen, z. B. wenn mehrere User gleichzeitig ein Objekt verändern wollen?

xdoo commented 6 years ago

Ich habe mir jetzt nochmal die Spring Data JPA Doku angeschaut. Die schreiben dort, dass die Methode save bei neuen Entitäten entityManager.persist und bei bereits vorhandenen entityManager.merge aufruft. Meiner Meinung nach ist damit die Angabe eines CascadeTypes hinfällig:

4.2.1. Saving entities Saving an entity can be performed via the CrudRepository.save(…)-Method. It will persist or merge the given entity using the underlying JPA EntityManager. If the entity has not been persisted yet Spring Data JPA will save the entity via a call to the entityManager.persist(…) method, otherwise the entityManager.merge(…) method will be called.

Wie erkennen sie, ob eine Entität neu oder gebraucht ist:

Id-Property inspection (default) | By default Spring Data JPA inspects the identifier property of the given entity. If the identifier property is null, then the entity will be assumed as new, otherwise as not new.

Das trifft auf uns zu. Deshalb würde ich das wie folgt machen:

@Data
@Entity
@Table(name = "Animal")
@NoArgsConstructor
@AllArgsConstructor
public class Animal extends BaseEntity {

    @Column(name = "name")
    String name;

    @OrderColumn(name = "order_index")
    @JoinTable(name = "Animal_KeeperList", joinColumns = {
        @JoinColumn(name = "animal_oid")}, inverseJoinColumns = {
        @JoinColumn(name = "keeperList_oid")})
    @ManyToMany
    List<Keeper> keepers = new ArrayList<>();

}

Folgender Test ist damit (wenig überraschend) erfolgreich:

    @Test
    public void testReferenceKeeper() {
        Keeper torben = this.keeperRepo.save(new Keeper("Torben"));
        Animal nemo = this.animalRepo.save(new Animal("Nemo", Lists.newArrayList(torben)));
        assertNotNull(nemo);
        assertEquals(1, this.animalRepo.count());
        assertEquals(1, this.keeperRepo.count());
        Keeper hans = this.keeperRepo.save(new Keeper("Hans"));
        Keeper peter = this.keeperRepo.save(new Keeper("Peter"));
        Keeper maja = this.keeperRepo.save(new Keeper("Maja"));
        assertEquals(4, this.keeperRepo.count());
        peter.setName("Petra");
        this.keeperRepo.save(peter);
        assertEquals(4, this.keeperRepo.count());
    }

Was sagt ihr @rowe42 und @FabianWilms ?

xdoo commented 6 years ago

Einen Punkt noch (inzwischen komplett offtopic ;):

Bei bidirektionalen Beziehungen müssen wir darauf achten, dass nicht eine der beiden Entitäten null als Referenz hat. Ich weiß nicht, ob das schon so implementiert ist. Weißt du das @FabianWilms ?

rowe42 commented 6 years ago

Wenn das komplett ohne CascadeType korrekt funktioniert ist das natürlich super. Ich probiere das morgen mal am echten Beispiel mit meinem Code zusammen aus.

FabianWilms commented 6 years ago

Finde die Lösung so auch gut!

Bei bidirektionalen Beziehungen müssen wir darauf achten, dass nicht eine der beiden Entitäten null als Referenz hat. Ich weiß nicht, ob das schon so implementiert ist. Weißt du das @FabianWilms ?

In Barrakuda haben wir bisher keine Bidirektionalen Beziehungen generieren lassen. Habe ich bisher mit Spring auch nicht ausprobiert.

xdoo commented 6 years ago

In Barrakuda haben wir bisher keine Bidirektionalen Beziehungen generieren lassen.

Heißt das wir können die (noch) nicht generieren, oder du hast es nur noch nie gemacht? Im Fall Keeper / Animal wäre das doch auch fachlich sinnvoll. Manchmal will man ja auch wissen, welche Tiere von einem bestimmten Keeper gepflegt werden.

xdoo commented 6 years ago

Ich trenne jetzt mal die Issues auf. Ich sehe folgende Dinge:

FabianWilms commented 6 years ago

👍 Claus Straube notifications@github.com schrieb am Fr. 9. März 2018 um 07:30:

Ich trenne jetzt mal die Issues auf. Ich sehe folgende Dinge:

  • @enablecaching einbauen und probieren. Dabei muss auch eine konfigurierbare Cache Implementierung verwendet werden.
  • @manytomany https://github.com/manytomany ohne CascadeTypes ausprobieren und ggf. implementieren
  • Bidirektionale Beziehung modellieren, generieren und ggf. fixen.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/xdoo/lhm_animad_admin_html5/issues/212#issuecomment-371725893, or mute the thread https://github.com/notifications/unsubscribe-auth/AKTMHt8SHvDgqH_gqs3lijgZGQdWDDqSks5tciFrgaJpZM4SQdgc .

rowe42 commented 6 years ago

Ok. Und das ursprüngliche issue lassen wir noch offen bis dann der Cache auch für die permissions scharf geschaltet ist.

rowe42 commented 6 years ago

Gibt jetzt eine neue Version in Branch _#212 in admin_service (die den zugehörigen Branch in lhm_authorisationLib benötigt), siehe auch PR https://github.com/xdoo/lhm_animad_admin_service/pull/19

Wenn der PR durch ist, kann ich das Issue schließen.

ejcsid commented 6 years ago

Merged.