Bad example of N+1 in current implementation

[lukaseder]

This implementation here should be improved by not fetching single items at a time:

for (JdbcPet pet : pets) {
    owner.addPet(pet);
    pet.setType(EntityUtils.getById(getPetTypes(), PetType.class, pet.getTypeId()));
    List<Visit> visits = this.visitRepository.findByPetId(pet.getId()); // Problematic call
    for (Visit visit : visits) {
        pet.addVisit(visit);
    }
}

I'm not 100% convinced that the JDBC implementation of the pet clinic is very sensible in general. With SQL, I might be doing things a bit differently from JPA. It looks as though the whole example has been shoehorned into some pre-existing repositories.

But anyway, this one should be a no-brainer. There should be a method like:

interface VisitRepository {
    Map<Integer, List<Visit>> findByPetIds(Integer... ids);
}

[michaelisvy]

Thanks @lukaseder, we will look into it. The whole piece of code is indeed complicated.

[michaelisvy]

There is a good example of what we should do here: http://stackoverflow.com/questions/6521937/using-spring-jdbc-template-for-populating-results
(proposed by @jnizet)

[lukaseder]

Hmm, that all seems very backwards (= too many LoC for little value) to me. With Java 8 (and e.g. jOOQ), I'm sure there's a more functional approach to generating nested collections in non-JPA environments...

[michaelisvy]

Hi Lukas, sure please feel free to propose something. In general, it's better if we don't have to add an additional dependency in the POM (there are already a lot...)

[lukaseder]

You won't like my proposition :) (because I don't think a repository with multiple implementations is a good idea in the first place)

[michaelisvy]

Sure, please feel free to share. I can't guarantee that it will be suitable but it's always good to discuss.

Cheers,
Michael.

[lukaseder]

Sure.

In a JPA / Repository approach, you mostly operate on what a Java developer prefers to work on, i.e. domain objects written in Java. It is natural to navigate those objects in a rather imperative style, i.e. fetching the parent, and then iterating its children, possibly letting the ORM figure out what to fetch eager or lazy (hint: it's mostly wrong by default and needs tuning).

In a SQL approach (JDBC / SpringJDBC / jOOQ / etc.), you would do things very differently. You would think of your data as sets (or bags) of tuples (or rows), which you materialise all at once as required. You wouldn't fetch the pets and visits individually. You'd either:

• Join them.
• Fetch both sets in 2 queries and join them in memory.
• Nest sets via the MULTISET operator (unfortunately, not supported in many databases)

In fact, you probably wouldn't even make use of any extra DTOs which are just boilerplate in a SQL context. You'd pass the sets of tuples directly to the UI. If writing back to the database is required (it is here), then you'd probably use an active record pattern as implemented by jOOQ, to store the results that are modified by the UI directly, instead of expressing INSERT and / or UPDATE statements.

I'm aware that such a change would be a lot of work, but Spring has a lot of responsibility as well. You guys are an inspiration for a lot of programmers, many of which are not yet very experienced.

My take here is that if you're showing them the JDBC solution as it is now, they will get things wrong. They will think that JPA and JDBC are "similar", instead of learning to think in terms of sets and set theory, as they should when working with SQL...

[michaelisvy]

Thanks for your feedback.
It makes sense, however the only way we could do that is to remove JDBC from the master branch and have a dedicated JDBC branch (that we will mention clearly on the Petclinic github homepage).
I haven't really used JDBC for years and I'm not the right person to create that branch. In case we would like to go that way, would you be interested to do it?

[lukaseder]

In case we would like to go that way, would you be interested to do it?

Only if I can contribute a jOOQ branch, too :) (disclaimer: I work for Data Geekery / jOOQ)

[michaelisvy]

I'll have to check internally, given that jOOQ is a commercial product. Let me get back to you on that a bit later.

[lukaseder]

Note that jOOQ is commercial, but ASL 2.0 (dual-)licensed.

Here's some help for your argumentation showing how other commercial products can add value to Spring :)
https://spring.io/blog/2014/10/01/webinar-building-bootful-uis-with-spring-boot-and-vaadin

[arey]

@michaelisvy and @lukaseder, to map the one-to-many relationships between Pet and Visit we may extends the OneToManyResultSetExtractor class provided by the Spring Data Core JDBC Extensions:
http://docs.spring.io/spring-data/jdbc/docs/1.1.0.RELEASE/reference/html/core.support.html#core.support.onetomany

The JdbcVisitRowMapper anonymous class used in the JdbcVisitRepositoryImpl has been reused by the JdbcOwnerRepositoryImpl

Here the corresponding pull request : arey@818529b