Repository vs. DAO (again)

Question

In general this back-story does not matter but just to explain the code below:

The server handles users and user groups. User groups are able to "discover" places - at this point in time these places are coming exclusively from the Google Places API.

Current Implementation

Currently, I have a lot of JpaRepository objects, which I call Repository, in my Service Layer. I am stressing "Repository" because in my proposed solution below, they'd be downgraded to DAOs.

However, what I do not like in my current code, and also the reason for my question here, is the amount of repositories one can find in the UserGroupService.

@Service
public class UserGroupService {

    private final static Logger LOGGER = LogManager.getLogger(UserGroupService.class);

    @Autowired
    private UserGroupRepository userGroupRepository;

    @Autowired
    private UserGroupPlaceRepository userGroupPlaceRepository;

    @Autowired
    private PlaceRepository placeRepository;

    @Autowired
    private GooglePlaceRepository googlePlaceRepository;

    @Autowired
    private GooglePlaces googlePlaces;

    public UserGroupService() {
    }

    @Transactional
    public void discoverPlaces(Long groupId) {

        final UserGroup userGroup = this.userGroupRepository.findById(groupId).orElse(null);

        if (userGroup == null) {
            throw new EntityNotFoundException(String.format("User group with id %s not found.", groupId));
        }

        List<PlacesSearchResult> allPlaces = this.googlePlaces.findPlaces(
                userGroup.getLatitude(),
                userGroup.getLongitude(),
                userGroup.getSearchRadius());

        allPlaces.forEach(googlePlaceResult -> {

            GooglePlace googlePlace = this.googlePlaceRepository.findByGooglePlaceId(googlePlaceResult.placeId);

            if (googlePlace != null) {
                return;
            }

            Place place = new Place();
            place.setLatitude(googlePlaceResult.geometry.location.lat);
            place.setLongitude(googlePlaceResult.geometry.location.lng);
            place.setPlaceType(Place.PlaceType.GOOGLE_PLACE);
            place.setName(googlePlaceResult.name);
            place.setVicinity(googlePlaceResult.vicinity);

            place = this.placeRepository.save(place);

            UserGroupPlace.UserGroupPlaceId userGroupPlaceId = new UserGroupPlace.UserGroupPlaceId();
            userGroupPlaceId.setUserGroup(userGroup);
            userGroupPlaceId.setPlace(place);

            UserGroupPlace userGroupPlace = new UserGroupPlace();
            userGroupPlace.setUserGroupPlaceId(userGroupPlaceId);

            this.userGroupPlaceRepository.save(userGroupPlace);

            googlePlace = new GooglePlace();
            googlePlace.setPlace(place);
            googlePlace.setGooglePlaceId(googlePlaceResult.placeId);

            this.googlePlaceRepository.save(googlePlace);
        });
    }
}

A Solution That Does Not Work

What could make this code a lot simpler and had the potential to resolve this mess up there, would be @Inheritance:

@Entity
@Table(name = "place")
@Inheritance(strategy InheritanceType.JOINED)
public class Place { /* .. */ }

@Entity
@Table(name = "google_place")
public class GooglePlace extends Place { /* .. */ }

However, this is not an option because then I cannot have a PlaceRepository which saves just a place. Hibernate does not seem to like it..

My proposal

I think my confusion starts with the names that Spring is using. E.g. JpaRepository - I am not so sure if this is actually "the right" name. Because as far as I understood, these objects actually work like data access objects (DAOs). I think it should actually look something like this:

public interface PlaceDao extends JpaRepository<Place, Long> {
}

public interface GooglePlaceDao extends JpaRepository<Place, Long> {
}

@Repository
public class GooglePlaceRepository {

    @Autowired
    private PlaceDao placeDao;

    @Autowired
    private GooglePlaceDao googlePlaceDao;

    public List<GooglePlace> findByGroupId(Long groupId) {
    // ..
    }

    public void save(GooglePlace googlePlace) {
    // ..
    }

    public void saveAll(List<GooglePlace> googlePlaces) {
    // ..
    }
}

@Service
public class UserGroupService {

    @Autowired
    private GooglePlaceRepository googlePlaceRepository;

    @Autowired
    private UserGroupRepository userGroupRepository;

    @Transactional
    public void discoverPlaces(Long groupId) {

    final UserGroup userGroup = this.userGroupRepository.findById(groupId).orElse(null)
        .orElseThrow(throw new EntityNotFoundException(String.format("User group with id %s not found.", groupId)));


    List<PlacesSearchResult> fetched = this.googlePlaces.findPlaces(
            userGroup.getLatitude(),
            userGroup.getLongitude(),
            userGroup.getSearchRadius());

    // Either do the mapping here or let GooglePlaces return 
    // List<GooglePlace> instead of List<PlacesSearchResult>

    List<GooglePlace> places = fetched.stream().map(googlePlaceResult -> {
        GooglePlace googlePlace = this.googlePlaceRepository.findByGooglePlaceId(googlePlaceResult.placeId);

        if (googlePlace != null) {
            return googlePlace;
        }

        Place place = new Place();
        place.setLatitude(googlePlaceResult.geometry.location.lat);
        place.setLongitude(googlePlaceResult.geometry.location.lng);
        place.setPlaceType(Place.PlaceType.GOOGLE_PLACE);
        place.setName(googlePlaceResult.name);
        place.setVicinity(googlePlaceResult.vicinity);
        googlePlace = new GooglePlace();
        googlePlace.setPlace(place);
        googlePlace.setGooglePlaceId(googlePlaceResult.placeId);
        return googlePlace;
    }).collect(Collectors.toList());

    this.googlePlaceRepository.saveAll(places);        

    // Add places to group..
    }

}

Summary

I would like to know what I don't see. Am I fighting the framework, or does my data model not make sense and this is why I find myself struggling with this? Or am I still having issues on how the two patterns "Repository" and "DAO" are supposed to be used?

How would one implement this?

Good question, shallow answers. Hope Code Review was a better fit. — oligofren, Sep 26 '22 at 08:45

score 2 · Answer 1 · edited Oct 16 '19 at 14:04

2

I would say you are correct that there are too many repository dependencies in your service. Personally, I try to keep the number of @Autowired dependencies to a minimum and I try to use a repository only in one service and expose its higher level functionality via that service. At our company we call that data sovereignty (in German: Datenhoheit) and its purpose is to ensure that there is only one place in the application where those entities are modified.

From what I understand from your code I would introduce a PlacesService which has all the Dependencies to the PlaceRepository, GooglePlaceRepository and GooglePlaces. If you feel like Service is not the right name you could also call it the PlacesDao, mark it with a Spring @Component annotation and inject all the Repositories, which are by definition collections of things

@Component
public class PlacesDao {

    @Autowired
    private PlaceRepository placeRepository;

    @Autowired
    private GooglePlaceRepository googlePlaceRepository;

This service/DAO could offer an API findPlacesForGroup(userGroup) and createNewPlace(...) and thus making your for Loop smaller and more elegant.

On a side note: you can merge your first four lines into just one. Java Optionals support a orElseThrow() method:

UserGroup userGroup = userGroupRepository.findById(groupId).orElseThrow(() -> 
     new EntityNotFoundException(String.format("User group with id %s not found.", groupId));

edited Oct 16 '19 at 14:04

Boris Pavlović

63,078
28
122
148

answered Apr 12 '18 at 06:42

phisch

4,571
2
34
52

Hm. Well I agree, but I do not really like the naming "*service*" here - because at this point we are already in the Service-Layer. So from my point of view, this can only be a business object, right? The question here is how to abstract a business object which works with models (`Place`, `GooglePlace`) using repositories. The thing here is - I think what I am calling repository in my code (e.g. `placeRepository`) is not a repository but actually a DAO and that I am just confusing this because the annotation in Spring is called `@Reposiroty` and/or the interfaces are called `CrudRepository` .. – Stefan Falk Apr 13 '18 at 18:15
.. and so on. However, I am getting the feeling that each repository is actually just a DAO and that all these DAOs should actually life in a *actual* repository which is supposed to do the heavy lifting here. – Stefan Falk Apr 13 '18 at 18:16
I guess it depends on what code you move. If you move business logic I would call it a _service_, because that is where business logic belongs. Calling one service from an other is fine, imho. If you are only moving low level data operations, you could create a `PlaceAdapter` that wraps away all Repositories (I am deliberately not calling it a DAO). Here is a Stackoverflow Post on [Repository vs DAO](https://stackoverflow.com/questions/8550124/what-is-the-difference-between-dao-and-repository-patterns) – phisch Apr 14 '18 at 07:43
1

Unfortunately I have already seen this post - and I've studied the answers as well. The problem is that they always talk about simple objects - in my opinion everything breaks as one would require object inheritance. I have updated my question and made things more clear hopefully. – Stefan Falk Apr 14 '18 at 13:14
I agree, most examples use very simple objects. Not sure, if inheritance is the solution. From how I understand your use case, it looks more like your business object is compound of several smaller entities. Similar in a way where one would store a binary file in an object storage and at the same time its meta data in a DB. I'll update my answer. – phisch Apr 16 '18 at 09:15
Hm, here is the thing a DAO is considered closer to the database than a repository (see e.g. [here](https://stackoverflow.com/a/8550228/826983)) - hence my question particularly aims at whether it would make sense to call my repositories `PlaceDao extends `JpaRepository` - there's a naming conflict. I am looking for a much more detailed explanation. I want - finally - once and for all - have an idea how these abstraction layers are actually supposed to work together. The major problem imho is that there is a ton of simple examples out there but no corner cases where you'd need inheritance etc. – Stefan Falk Apr 16 '18 at 13:59

score 0 · Answer 2 · answered Apr 10 '18 at 08:48

I think the foreach does not look like a good approach to me. You're doing way to much for just a single responsibility of a function. I would refactor this to a standart for loop.

        Place place = new Place();
        place.setLatitude(googlePlaceResult.geometry.location.lat);
        place.setLongitude(googlePlaceResult.geometry.location.lng);
        place.setPlaceType(Place.PlaceType.GOOGLE_PLACE);
        place.setName(googlePlaceResult.name);
        place.setVicinity(googlePlaceResult.vicinity);

        place = this.placeRepository.save(place);

This part can easily be a method in a service.

        UserGroupPlace.UserGroupPlaceId userGroupPlaceId = new 
        UserGroupPlace.UserGroupPlaceId();
        userGroupPlaceId.setUserGroup(userGroup);
        userGroupPlaceId.setPlace(place);

        UserGroupPlace userGroupPlace = new UserGroupPlace();
        userGroupPlace.setUserGroupPlaceId(userGroupPlaceId);

        this.userGroupPlaceRepository.save(userGroupPlace);

That part as well.

        googlePlace = new GooglePlace();
        googlePlace.setPlace(place);
        googlePlace.setGooglePlaceId(googlePlaceResult.placeId);

        this.googlePlaceRepository.save(googlePlace);

And this part: I don't understand why your doing this. You could just update the googlePlace instance you loaded from the repo. Hibernate/Transactions are doing the rest for you.

Hi! Thanks for your answer. So the thing is this: I have different types of places (another is `CUSTOM_PLACE` (CP)). A `UserGroupPlace` therefore can display a CP and `GOOGLE_PLACE` (GP). A GP however takes additional information (`placeId`) which means I have to write a new `Place`, `UserGroupPlace` and a `GooglePlace`. What I *could* do is create a `UserGroupGooglePlaceDao` which wraps the functionality and only deals with this type of place by ensuring that such a place gets stored in the database correctly. However, I'd also need a `UserGroupCustomPlaceDao` for this particular type. — Stefan Falk, Apr 10 '18 at 09:59
At this point I am not sure if this is really the way to go. To have a `*Dao` for every type of entity I want to store in the database. Another way would be to de-normalize my tables in the database and allow nullable values - but would that be "cleaner"? — Stefan Falk, Apr 10 '18 at 10:01
To be honest this is getting really detailed. It can be reasonable to hold nullable fields in combination with a dtype. For example when the usage of the child classes does not change per child class and you can work only on the parent class. Otherwise I would not hold nullable fields. — Jan Schumacher, Apr 11 '18 at 15:23

Repository vs. DAO (again)

Current Implementation

A Solution That Does Not Work

My proposal

Summary

2 Answers2

Linked