[clarification]: Should non-standard query by methods be supported by user defined repositories

[KyleAure]

Specification

https://github.com/jakartaee/data/blob/e7dbeab00950ed6bd14d4d0cd90df595ef9c908e/api/src/main/java/module-info.java#L245-L248

I need clarification on ...

Problem

While writing tests for Validation in the TCK I copied methods from the BasicRepsoitory interface and added validation annotations. Later while running the TCK I ran into issues because of the non-standard query by methods that exist in the BasicRepository.

The BasicRepsoitory has the following methods that are presumably query by method: https://github.com/jakartaee/data/blob/e7dbeab00950ed6bd14d4d0cd90df595ef9c908e/api/src/main/java/jakarta/data/repository/BasicRepository.java#L178 https://github.com/jakartaee/data/blob/e7dbeab00950ed6bd14d4d0cd90df595ef9c908e/api/src/main/java/jakarta/data/repository/BasicRepository.java#L231

I might write my own Repository interface which has:

public interface MyRepository<MyEntity, Long> extends DataRepository<MyEntity, Long> {
  long count();
}

Ambiguity

Currently the javadoc / spec fails to define behavior around this:

If a custom repository is written with the method count() should the implementation respect this method because it also exists within the built in repositories, or should a DataException be thrown to alert the user that their query is invalid?

Additional information

I can think of three solutions here:

Documentation

• Update documentation to define behavior when these methods are used in custom repositories
• Update the javadoc for these methods to point out that they are special uses of invalid syntax that need to be supported by implementations.

Use proper syntax

• Update these methods to use proper syntax:

long countBy();
void deleteAllBy();

Swap to query by parameter

• Create @Count annotation
• Modify @Delete annotation to accept no parameters
• Update these methods to use these annotations:

@Count
long count();

@Delete
void deleteAll();

[njr-11]

It's a good point that count() and deleteAll() from BasicRepository don't follow the specification's own rules per the BNF for Query by Method Name, and thus when users copy them to their own repositories that don't inherit from BasicRepository, they won't behave the same as they would if the repository had inherited from it.

[njr-11]

A simple way to fix half of the problem (the deleteAll() part) is covered by #413 The count() method will need a different solution.

[otaviojava]

IMHO: those are methods from the interface. Checking the spec:

The Query by method mechanism allows for creating query commands by naming convention. https://github.com/jakartaee/data/blob/main/spec/src/main/asciidoc/repository.asciidoc#query-by-method-name

Thus, BasicRepository is not a name convention but a from buil-it repository.

I mean, this method comes from the interface that has clear documentation about the result.

[njr-11]

Thus, BasicRepository is not a name convention but a from buil-it repository.

@otaviojava While it is true that count() and deleteAll() are built-in interface methods, it is also the case that Jakarta Data requires its built-in interface methods be consistent with the same patterns by which a user writes custom repository methods, so that a user can copy them and rely on getting the same behavior just as Kyle was attempting to do. This is documented here:

https://github.com/jakartaee/data/blob/394809de67569e117641f24e5898c3a5628a40f2/api/src/main/java/module-info.java#L118-L122

This is a good requirement to have because users will want to do the exact same thing Kyle expected would work in his TCK test. I figured out how to solve it an rather trivial manner for deleteAll(), and if that is acceptable, that only leaves figuring out how to solve it for count().

[otaviojava]

Thus, BasicRepository is not a name convention but a from buil-it repository.

@otaviojava While it is true that count() and deleteAll() are built-in interface methods, it is also the case that Jakarta Data requires its built-in interface methods be consistent with the same patterns by which a user writes custom repository methods, so that a user can copy them and rely on getting the same behavior just as Kyle was attempting to do. This is documented here:

https://github.com/jakartaee/data/blob/394809de67569e117641f24e5898c3a5628a40f2/api/src/main/java/module-info.java#L118-L122

This is a good requirement to have because users will want to do the exact same thing Kyle expected would work in his TCK test. I figured out how to solve it an rather trivial manner for deleteAll(), and if that is acceptable, that only leaves figuring out how to solve it for count().

Does this requirement make sense? I mean, the whole goal of the non-built-in repository, I am assuming, is to have the freedom to implement and go for the Ubiquitous Language of their domain.

If the user wants all the methods at BasicRepository, why not just use the interface?

Do you have any case of this? Should we worry about these premises or update with: use BasicRepository if you want to use their method.

[KyleAure]

Does this requirement make sense? I mean, the whole goal of the non-built-in repository, I am assuming, is to have the freedom to implement and go for the Ubiquitous Language of their domain.

If the user wants all the methods at BasicRepository, why not just use the interface?

Do you have any case of this? Should we worry about these premises or update with: use BasicRepository if you want to use their method.

Yes this requirement does make sense if you want to add validation to the built-in methods. Something like:

@Max(90)
long count();

You can't inherit BasicRepository because you want to add the annotation to the method, and if you just copy this method out of BasicRepository it is invalid. (The exact thing I did without realizing it and I've been following the development of the spec 😨)

[otaviojava]

So, you you want to do count, plus, put the validation, isn't?

In that case, why not use the method by query?

@Max(90)//extenal validation
long countBy();

[KyleAure]

I can do that, but think about the user experience.

• I'm going to look at the method from the built-in BasicRepository an see long count(); and attempt to use that first.
• Get failures and hope the implementation gave me a good error message.
• If not I need to go read through the specification to find that long count(); is invalid query by method language.
• Change my query to long countBy();.
• Open an issue asking for an explination for why long count() exists in the basic repository if it isn't valid anywhere else.

Why not avoid this all together and use the correct query by method to begin with?

Not to mention if I do end up extending BasicRepository AND adding in @Max(90) long countBy(); now I have two counting methods that do the same thing, have a cluttered namespace, and will have to fight with my IDE to use countBy instead of count each time I want a validated count.

[otaviojava]

I can do that, but think about the user experience.

• I'm going to look at the method from the built-in BasicRepository an see long count(); and attempt to use that first.
• Get failures and hope the implementation gave me a good error message.
• If not I need to go read through the specification to find that long count(); is invalid query by method language.
• Change my query to long countBy();.
• Open an issue asking for an explination for why long count() exists in the basic repository if it isn't valid anywhere else.

Why not avoid this all together and use the correct query by method to begin with?

Not to mention if I do end up extending BasicRepository AND adding in @Max(90) long countBy(); now I have two counting methods that do the same thing and have a cluttered namespace.

My point is to put this as a restriction because those are two different types of interfaces:

• built-in (the interfaces that we provide) and the Javadoc explaining each behavior.
• the possibility for the user to create the operation using: method by query, param matcher, or the operation annotation.

1. Does the user want to use the methods on the built-in interfaces? R. They can use the methods over there.
2. Do they want to keep the same semantics and overwrite the behavior? R. They can specialize as we have specific databases, for example, Cassandra, MongoDB. That's why we have OOP, right?

IMHO: My point is putting this restriction without a good case or real scenario for it. I mean, why will we put a Max validation in the count result?

[KyleAure]

IMHO: My point is putting this restriction without a good case or real scenario for it.

What is the restriction?
Updating the method in BasicRepository to long countBy(); will not restrict anyone it's just a different method name which follows the query by method documentation and making the API easier to use/extend.

I mean, why will we put a Max validation in the count result?

This was just an example, a user could have other annotation processors they want to use. The point is we are creating a poor user experience.

[njr-11]

I mean, why will we put a Max validation in the count result?

To enforce that the database doesn't reach an unexpected amount of records -- That's probably not very common.

Here's the more likely scenario that I'm sure will happen because I just saw Kyle run into it: The user is happily using BasicRepository for a while, and some point decides they want to add validation when saving entities to keep bad data out of the database. First, they try overriding the save method to be save(@Valid MyEntity entity). CDI complains about that and won't let them do it,

org.jboss.weld.exceptions.DefinitionException: HV000151: A method overriding another method must not redefine the parameter constraint configuration, but method MyRepository#save(MyEntity) redefines the configuration of BasicRepository#save(Object).

So naturally, they stop inheriting from BasicRepository, switch to inherit from DataRepository instead and copy the method signatures that they were using from BasicRepository because they don't want to rewrite all of their code. If count() was one of those, then it fails.

Jakarta Data is doing the right thing by stating the requirement that users should be able to copy over the method signatures and get the same behavior. I can't see changing that. Nearly the entire spec compiles with it, except these couple of methods that Kyle discovered. The best course of action is to correct both of them so that Jakarta Data is everywhere following its own rules and being a good example to users instead of misleading them.

[otaviojava]

IMHO: My point is putting this restriction without a good case or real scenario for it.

What is the restriction? Updating the method in BasicRepository to long countBy(); will not restrict anyone it's just a different method name which follows the query by method documentation and making the API easier to use/extend.

I mean, why will we put a Max validation in the count result?

This was just an example, a user could have other annotation processors they want to use. The point is we are creating a poor user experience.

Good point, please, let me know what you think:

https://github.com/jakartaee/data/pull/414

[otaviojava]

I mean, why will we put a Max validation in the count result?

To enforce that the database doesn't reach an unexpected amount of records -- That's probably not very common.

Here's the more likely scenario that I'm sure will happen because I just saw Kyle run into it: The user is happily using BasicRepository for a while, and some point decides they want to add validation when saving entities to keep bad data out of the database. First, they try overriding the save method to be save(@Valid MyEntity entity). CDI complains about that and won't let them do it,

org.jboss.weld.exceptions.DefinitionException: HV000151: A method overriding another method must not redefine the parameter constraint configuration, but method MyRepository#save(MyEntity) redefines the configuration of BasicRepository#save(Object).

So naturally, they stop inheriting from BasicRepository, switch to inherit from DataRepository instead and copy the method signatures that they were using from BasicRepository because they don't want to rewrite all of their code. If count() was one of those, then it fails.

Jakarta Data is doing the right thing by stating the requirement that users should be able to copy over the method signatures and get the same behavior. I can't see changing that. Nearly the entire spec compiles with it, except these couple of methods that Kyle discovered. The best course of action is to correct both of them so that Jakarta Data is everywhere following its own rules and being a good example to users instead of misleading them.

Good sample, Thus, renaming by method by query convention is fine. Please, let me know your thoughts

[otaviojava]

ps: It will not solve the issue with "findAll" methods, that will face the same issue.

[njr-11]

ps: It will not solve the issue with "findAll" methods, that will face the same issue.

findAll() has no issue. It is validly interpreted under parameter matching with 0 parameters, which always performs find operations.

[otaviojava]

ps: It will not solve the issue with "findAll" methods, that will face the same issue.

findAll() has no issue. It is validly interpreted under parameter matching with 0 parameters, which always performs find operations.

Remember, that we do have the PageableRepository that has it with a parameter.

[njr-11]

ps: It will not solve the issue with "findAll" methods, that will face the same issue.

findAll() has no issue. It is validly interpreted under parameter matching with 0 parameters, which always performs find operations.

Remember, that we do have the PageableRepository that has it with a parameter.

Yes, but findAll(Pageable pageable) is also validly interpreted under parameter matching. The spec defines how special parameters (such as Pageable) are used, and the preceding parameters, of which there happen to be none, are used for the automatic query method : either Query by Method Name (it is not that pattern here) or by matching parameter based conditions. The latter is chosen and there are 0 conditions against which to match, causing all records to be returned (as controlled by the Pageable).

[otaviojava]

Good, we are coverted it. I might find another issue, but I will create a discussion in another issue.

[KyleAure]

Fixed by #413 and #414