UserServiceImpl.save() Rückgabe

Byte-Welt - Projekte / Projects fjorum - Java based Forum
1

Ein kleiner Hinweis, bzw. eine Frage ob das so gut ist.

Die save()-Methode in UserServiceImpl hat den Rückgabetyp void. Ist also schonmal suspekt für einen Funktional-Denkenden Programmierer.

Das ganze ist ein Delegate (eine Weiterleitung) zur UserRepository.save()-Methode, welche von org.springframework.data.jpa.repository.JpaRepository.save(), bzw. CrudRepository, abgeleitet ist und einen Rückgabetyp User hat.

Und dort findet sich folgendes JavaDoc:

Saves a given entity. Use the returned instance for further operations as the save operation might have changed the entity instance completely.

Momentan findet sich noch nicht soviel Anwendungscode, bzw. die save()-Methode wird außerhalb einer init()-Methode noch nicht so oft aufgerufen.

Aber ich kann mir vorstellen das es zu unerwarteten Effekten kommen kann, wenn man irgendwann mal sowas ähnliches aufruft:

User user = createUserFromForm();
userService.save(user);
redirectTo("users/"+user.getId()); //=> users/null

// good
User user = createUserFromForm();
User savedUser = userService.save(user);
redirectTo("users/"+savedUser.getId()); //=> users/12345```

Kann natürlich auch nur ein absolut hypothetisches Problem sein, dass so nie vorkommen wird oder bei Bedarf behoben werden kann.

Und falls dies doch in Betracht käme geändert zu werden, dann könnte auch ein Blick auf `RoleServiceImpl.addRole()`, sowie `CategoryServiceImpl.createNewCategory()` nicht schaden.


Wie gesagt, rein hypothetisch.

`assert user == userRepository.save(user)` kann, muss aber laut JavaDoc nicht funktionieren.
2

IIRC hängt es damit zusammen, wie die IDs generiert werden. Dein Einwand ist auf jeden Fall berechtigt, sodass ich die (ggf. ausgetauschte) Entity vom UserService zurückgeben lasse.
Danke für den Hinweis!

3

und bei

        Category category = new Category(name, parent);
        parent.getChildren().add(category);
        categoryRepository.save(parent);
        return category;
    }

ist sichergestellt, dass category bleibt was es ist?
vielleicht auch dort den Rückgabewert von parent nehmen und die Category mit dem gegebenen Namen neu heraussuchen…

etwas bedenkliches Modell, Hibernate gibt bei save-Methode immerhin nur evtl. generierte Id zurück,
bei einem merge freilich auch nicht zu vermeiden

4

[quote=SlaterB]etwas bedenkliches Modell, Hibernate gibt bei save-Methode immerhin nur evtl. generierte Id zurück,
bei einem merge freilich auch nicht zu vermeiden[/quote]
Die save-Methode ist im SimpleJpaRepository implementiert und wrappt sowohl persist als auch merge vom EntityManager:

    if(entityInformation.isNew(entity)){
        em.persist(entity);
        return entity;
    } else {
        return em.merge(entity);
    }
}```

Was in dem konkreten Fall in `createNewCategory` zurückkommen wird, weiß ich nicht - der Code ist offensichtlich noch in der Entstehungsphase. Aber @Landei  (ping) wird die Beiträge hier sicherlich lesen :-)
5

dann funktioniert es hier ja auch,
für Neuanlegen wird durch persist der Parameter geändert, keine Rückgabe,
allein die SimpleJpaRepository erfindet die Rückgabe und gibt dann auch den Parameter selber zurück,

dass der else-Fall mit merge drankäme wäre wohl ein größeres Problem, als alles was beim Aufrufer mit Weiterarbeit nur mit Parameter geschehen könnte

ist EntityManager.persist() als schlecht angesehen so dass lieber das Modell mit Rückgabe aus Prinzip eingebaut?
falls doch ok, dann ist es eine Sache, die save-Methode von SimpleJpaRepository zu benutzen, aber dann kann man ja bei allen echten Saves lieber beim EntityManagemer-Standard, Hibernate-Standard, hoffentlich Alle-Welt-Standard bleiben, Parameter reicht

auch für sonstige merge/ update-Methoden usw. kann es evtl. standardmäßig bleiben,
wäre auch dort bedenklich, wenn mit einem detached-Objekt komplizierte Änderungen durchführen wollte, sollte doch lieber vorher wieder aktiviert werden

der Einsatz der JpaRepository etwas verzichtbar im Moment, manche Methoden wie das findOne() eh auf Optional umzubauen
(allerdings mit Name get, etwa getUserById, gemacht, welcher in JpaRepository/ EntityManager die Entity oder Exception liefert?
find steht für optionale Variante, Entity oder null…),

und wenn das Repository Attribut ist, dann sind sogar die einfachen Methoden als Delegate zu implementieren?

        return userRepository.save(user);
    }```

wie wärs mit einer Basisklasse mit etwa EntityManager als AutoWire?
man soll ja nicht alles selber implementieren aber bis auf die exists()-Methode mit Prüfung aller Attribute ist nicht viel dabei

gemeinsame Methoden wie getUserById() allgemein formulieren, also getById() und nur einmal für alle implementieren,
in der save()-Methode EntityManager.persist() nutzen und wieder selber entscheiden, ob unbedingt Rückgabetyp, wofür nur der Parameter zurückgegeben werden könnte, usw.

gemeinsame Methoden immer gut für Logging

evtl. in der Basisklasse mit einem Attribut die Implementierung austauschbar machen, auch Mocking für Tests, dann dort Delegate-Methoden wie
```    public void save(T entity) {
        return xy.save(entity);
    }```
aber dann zumindest das nur einmal zu schreiben, nicht in jedem Service neu
6

Dann hat man das Problem, dass man u.U. Methoden in Services hat, die nicht gebraucht werden. Außerdem erhebt sich die Frage, wie man dann z.B. Security-Annotations an die Methode bekommt.

Ich meditiere nochmal drüber, vielleicht geht was mit Interfaces, ungefähr so (aus’m Ärmel):

public interface FooService {
   void save(Foo foo);
   void remove(Foo foo);
}

public interface Saving<T> {
   JpaRepository<T, Long> getRepo();
   default void save(T t) {
       getRepo().save(t);
   }
}

public interface Deleting<T> {
   //analog
}

public class FooServiceImpl implements FooService, Saving<Foo>, Deleting<Foo>{
   @Autowire
   private FooRepository fooRepo;

   public FooRepository getRepo(){ return fooRepo; }
}

So wäre getRepo zwar public, was aber nicht ins Gewicht fallen sollte, da die Klasse normalerweise als FooService verwendet werden sollte.