Move to better naming scheme of methods in repository interfaces [DATACMNS-944]

[spring-projects-issues]

Oliver Drotbohm opened DATACMNS-944 and commented

The current naming scheme for methods in CrudRepository cause quite a few problems. Especially the methods taking parameters that are generic type variables. Under bad circumstances they could effectively resolve to the same methods and cause ambiguities.

With the 2.0 release we have the chance to correct those mistakes and the following scheme is suggested (pseudo code):

// Previously ID extends Serializable
interface CrudRepository<T, ID> {

	<S extends T> … save(S entity);
	// Previously save(Iterable)
	<S extends T> … saveAll(Iterable<S> entities);

	// Previously findOne(ID)
	… findById(ID id);

	// Previously exists(…)
	… existsById(ID id);

	… findAll();
	… findAllByIds(Iterable<ID> ids);

	… count();

	// Previously delete(ID)
	… deleteById(ID id);
	… delete(T entity);

	// Previously delete(Iterable)
	… deleteAll(Iterable<? extends T> entities);
	… deleteAll();
}

The methods that are effected by a change are marked with a comment here. The new scheme is driven by the following requirements:

1. We're able to uniquely find methods by name and raw parameter type (ideally by the number of parameters in the first place). Previously, the generics of esp. the delete(…) methods could effectively resolve to the same type.
2. Methods named …All(…) affect a collection of items and/or return one.
3. Methods taking an identifier are named …ById(…).
4. With that in place, we can reliably drop the ID extends Serializable requirement, which is not possible without that change as then the delete(…) methods would be ambiguous

---

Issue Links:

• DATACASS-441 Adapt to API changes in repository interfaces
  ("is depended on by")
• DATAKV-177 Adapt to API changes in repository interfaces
  ("is depended on by")
• SGF-623 Adapt to API changes in Repository interfaces
  ("is depended on by")
• DATAMONGO-1679 Adapt to API changes in CrudRepository
  ("is depended on by")
• DATACOUCH-305 Adapt to API changes in repository interfaces
  ("is depended on by")
• DATAGRAPH-991 Adapt to API changes in repository interfaces
  ("is depended on by")
• DATAJPA-1104 Adapt to API changes in repository interfaces
  ("is depended on by")
• DATAKV-176 Adapt to API changes in repository interfaces
  ("is depended on by")
• DATALDAP-35 Adapt to API changes in repository interfaces
  ("is depended on by")
• DATAREDIS-640 Adapt to API changes in repository interfaces
  ("is depended on by")
• DATAREST-1064 Adapt to API changes in repository interfaces
  ("is depended on by")
• DATASOLR-385 Adapt to API changes in repository interfaces
  ("is depended on by")
• DATAES-352 Adapt to API changes in CrudRepository
  ("is depended on by")
• DATAREDIS-641 Adapt to API changes in CrudRepository
  ("is depended on by")
• DATAREST-1065 Adapt to API changes in CrudRepository
  ("is depended on by")
• SGF-622 Adapt to API changes in CrudRepository
  ("is depended on by")
• DATACMNS-574 Change CrudRepository to return Optional<T> as return values instead of null reference or provide alternative base interface

1 votes, 18 watchers

[spring-projects-issues]

Andy Wilkinson commented

I like the new names a lot. FWIW, I'd go with existsById rather than exists. I like the consistency of the ById suffix

[spring-projects-issues]

Heiko Scherrer commented

Looks more streamlined. I would be more precise on #3: What is an ID? An unique identifier. Is it the persistent key or is it the business key? It is the persistent key. So call it PK. findByPk(..). Not to confuse with "primary key"! Often I've used something like: findByPk(42) and findByBk("me@home.com"). The latter is of course not part of the API. In contrast to Andy I'd call the exists method existsForPk(..)

[spring-projects-issues]

Edward Beckett commented

I'm on board with existsById...ById postfix as well...

[spring-projects-issues]

Oliver Drotbohm commented

Heiko Scherrer — I don't think we're gonna go with PK as it has an inherently relational touch. The repository is supposed to abstract from that. Wouldn't you rather also have a dedicated name for the business key? like Optional<Customer> findByCustomerNumber(…). I think we should abstain from making any assumptions about those things and basically treat that method like an arbitrary query method

[spring-projects-issues]

Kai Tödter commented

+1 for the new names

[spring-projects-issues]

Milan Milanov commented

Great proposal. I'd go with existsById though, just for consistency

[spring-projects-issues]

Bartłomiej Tartanus commented

Great! I love the new names!

[spring-projects-issues]

André Schäfer commented

How about backwards compatibility?

[spring-projects-issues]

Juergen Zimmermann commented

(y) for the names, especially the *ById *suffix

[spring-projects-issues]

Oliver Drotbohm commented

I've updated exists(…) to become existsById(…).

André Schäfer — This is a proposal for 2.0 in which we're gonna introduce some breaking change anyway as we will hardly ever have the chance to do so any time after that. So we were rather thinking that if we make those anyway, let's get more things right this time

[spring-projects-issues]

František Hartman commented

What about cases where internal id is different than id property/field (hence findOne and findById have different sematics)?

E.g.: I could map mongo _id to different field than id using @Id annotation and then have id field, would I use derived finder for that?

Similarly in Spring Data Neo4j findOne searches by internal graph id and people may have another field id (that translates to property id) - it is actually quite common there because people are discouraged to use the internal id directly as it is offset in database file and may change between restarts.

I don't know about other Spring Data projects, there might be more examples..

[spring-projects-issues]

Oliver Drotbohm commented

In this case "id" doesn't refer to a property named "id" as we — just as before — would obtain the identifier from whatever the store implementation considers to be one. Actually, that's one of the reasons we have abstained from using …ById methods before as that sort of blurs the line between query methods and CRUD ones. However, it turns out that the newly gained advantages seem to outweigh that tiny semantic glitch. WDYT?

So summarizing: if you declare a method findById(…) and don't even extend CRUD repository, you assume a property named ID, which the CRUD implementation would then automatically resolve to (as it transparently looks up the identifier property no matter its name). If your identifier property is named different, you'd still get findById(…) to use that one, but you could also declare a findByMyId(…) and it would be just executed as a query method

[spring-projects-issues]

František Hartman commented

That all makes sense, but will eventually result in not being able to do following:

class MyDocument {
@Id
String mongoId;

String id; // my domain specific id

}

interface MyDocumentRepository extends MongoRepository<String, MyDocument> {

// I can't do following because it already exists in crud, right?
Collection<MyDocument> findById(String id); // or just MyDocument as return type...
}

[spring-projects-issues]

Oliver Drotbohm commented

I think it should work if you annotate the method with @Query as that indicates you want this thing to be a query method explicitly. Would that be acceptable?

[spring-projects-issues]

Marcos Abel commented

(y) all the proposed changes make sense to me

[spring-projects-issues]

Heiko Scherrer commented

+1 František Hartman fully agree!

[spring-projects-issues]

Ali Shahbour commented

What about supporting Slice<T> instead of Page<T> , to get bypass the count query ? DATAJPA-961

[spring-projects-issues]

Oliver Drotbohm commented

We can certainly look into options of introducing new methods to support Slice more explicitly. I'd like to focus on core naming decisions here, first, though

[spring-projects-issues]

Benjamin Schmeling commented

ById suffix makes sense for me, it is more consistent. What about findAll(Iterable ids)? Shouldn't we rename it to findAllByIds(Iterable ids)?

[spring-projects-issues]

Jens Schauder commented

What about

... delete(Iterable<? extends T> entities)

... id	delete(T entity)

Wouldn't they also cause conflicts, when T is an Iterable? Maybe the first should be renamed to deleteAll?

[spring-projects-issues]

Oliver Drotbohm commented

We're gonna stick to the …ById convention. Escape hatch for potential shadowing is findFooById(…). We're gonna introduce temporary environment property evaluation to stop bootstrap log a report about potential naming conflicts

[spring-projects-issues]

Gary Hodgson commented

I realise this issue is closed, and released in GA, but I just wanted to say that the whilst it may have solved some problems it introduces others (e.g. my SO question here).

It seems a sub-optimal design decision to use method names in the parent interface that are valid queries for child repositories, thus resulting in either conflicts or (worse) inadvertently overriding behaviour. Having existsById or findById acting on natural keys (rather than e.g. surrogate primary keys) seems like a valid and reasonable decision for a child repository but it will now creates problems in 2.0.

The suggestion from Oliver above is sufficient to resolve the problem, but now our team has to be ever vigilant that the ..ById methods are overridden and annotated with @Query, otherwise there will be hard-to-diagnose bugs in our future.

It would have been better if this Jira had resulted in method names that are not valid query methods

[spring-projects-issues]

Gary Hodgson commented

Something else worth noting regarding the shadow method workaround: calls do not hit the L2 Cache