Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
spring-projects-issues opened this issue Nov 24, 2016 · 23 comments

Comments

@spring-projects-issues
Copy link

@spring-projects-issues spring-projects-issues commented Nov 24, 2016

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
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 24, 2016

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
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 24, 2016

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
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 24, 2016

Edward Beckett commented

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

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 24, 2016

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
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 24, 2016

Kai Tödter commented

+1 for the new names

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 24, 2016

Milan Milanov commented

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

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 24, 2016

Bartłomiej Tartanus commented

Great! I love the new names!

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 24, 2016

André Schäfer commented

How about backwards compatibility?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 25, 2016

Juergen Zimmermann commented

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

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 25, 2016

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
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 25, 2016

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
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 25, 2016

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
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 25, 2016

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
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 25, 2016

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
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 25, 2016

Marcos Abel commented

(y) all the proposed changes make sense to me

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 25, 2016

Heiko Scherrer commented

+1 František Hartman fully agree!

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 25, 2016

Ali Shahbour commented

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

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 25, 2016

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
Copy link
Author

@spring-projects-issues spring-projects-issues commented Dec 5, 2016

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
Copy link
Author

@spring-projects-issues spring-projects-issues commented Feb 13, 2017

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
Copy link
Author

@spring-projects-issues spring-projects-issues commented Apr 7, 2017

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
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 20, 2017

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
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 7, 2017

Gary Hodgson commented

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants