allow multi-valued parameters to @Find & @Delete methods

[gavinking]

• interpreted as defining an "in" condition
• allows a methods of BasicRepository to be redefined in terms of @Find & @Delete

This change is completely obvious, now that it occurs to me.

And it leaves existsById() as the only method we can't currently define in terms of annotations.

[gavinking]

(Rebased to pick up Otavio's changes .)

[gavinking]

After implementing this I decided that Iterable is just so painful to work with that we should migrate to either List or Collection as the type we support here and also in lifecycle methods. I understand the desire to be as general as possible, but Iterable is just useless: you can't stream it, you can't easily convert it to an array or list, and there are almost no useful types which implement Iterable and not Collection. There's a good reason people rarely use it in APIs.

Therefore: 8b26ecb

[njr-11]

I thought about the possibility of doing something like this as well, but I didn't feel comfortable with adding it in version 1.0 because it has some overlap with the possibility of collection attributes in the future. I'll admit that I don't see use ever having an equals capability for collections, but I don't want to see the programming model be defined in an inconsistent way that we might later regret.

[gavinking]

@njr-11 Read the way I've specified this more carefully. There's no ambiguity.

If Thing has a field is of type String[], then this:

@Find Thing find(String[] field)

is an equals-style condition, and this:

@Find Thing find(String[][] field)

is an in-style condition.

We know the type of the entity field.

[gavinking]

I'll admit that I don't see use ever having an equals capability for collections

Just to clarify, what is allowed in JPA is an AttributeConverter for a persistent field of type List<String> or String[] or whatever. So I can totally have a field of "collection" type in my entity. Hell, Hibernate even allows this as a default mapping for native SQL array types.

So yes, we do need to cover this case. But it's covered :-)

[gavinking]

Rebased on main to pick up merged changes the repository interfaces.

[njr-11]

This proposal would have me doing something like,

    @Find
    List<Product> sized(@By(_Product.SIZE) List<Size> sizes);

versus what we might consider adding in the future, which could be something like,

    @Find
    List<Product> sized(@By(_Product.SIZE) @In List<Size> sizes);

The meaning of the latter is clearer. I might naturally read the former as looking for a product that is available in all of the sizes listed. The latter makes it clear that the product need only be available in any of the sizes in the list to match.

Either way, we will eventually have a solution that we could put on BasicRepository.find/deleteByIdIn, so I would be okay with adding those methods now relying on Query by Method Name, and we can add annotations to them in the future. I do not want to add the multi-valued parameters in 1.0 because I expect we will regret it later.

[gavinking]

The meaning of the latter is clearer.

I mean, to me it's just unnecessarily more verbose. The meaning is already perfectly clear from the type of the parameter, and, in your code example, by the name of the parameter. I really don't see where the confusion could possibly arise.

What I mean is, when I see this method signature:

@Find
List<Product> sized(@By(_Product.SIZE) List<Size> sizes);

I know exactly what it does and so do you. And so does everyone else, even Java developers who don't know Jakarta Data. Adding @In to the signature here is just adding noise.

But, OK, let's say we aren't going to do this now. That's in principle completely fine by me.

But then I insist we need to remove the methods of BasicRepository which depend on this functionality, which are:

void deleteByIdIn(Iterable<K> ids);

Stream<T> findByIdIn(Iterable<K> ids);

Now:

• the loss of deleteByIdIn() doesn't seem to be any great loss at all—I struggle to see this how this is generally useful (except perhaps in tests) [FTR I would also happily remove existsById() which similarly only looks useful for tests],
• but findByIdIn() looks really quite useful to me, and enables optimized fetching, so it would be a at least a bit annoying to see it go.

And the issue is that, as you showed the other day, we can't really come back and add these operations later because that would be a breaking change.

now relying on Query by Method Name

No, that doesn't work. Because deleteByIdIn() and existsById() are no longer legal Query by Method Name methods. As we agreed on the last call (you yourself told me explicitly to make this change), we removed the special-casing of Id. (And I think this was the right thing to do, and I don't think we should roll back that change.)

[gavinking]

(There's a third option, of course: we could solve the problem by going ahead and adding an @In annotation, against my best judgment.)

[njr-11]

No, that doesn't work. Because deleteByIdIn() and existsById() are no longer legal Query by Method Name methods. As we agreed on the last call (you yourself told me explicitly to make this change), we removed the special-casing of Id. (And I think this was the right thing to do, and I don't think we should roll back that change.)

Yes, good point. Somehow I managed to forget that we had removed the Id alias when writing that. So temporarily relying on Query by Method Name is not an option.

[gavinking]

Yeah, to be clear, and it's totally my fault for not making this explicit in the issue description: I'm proposing this change now, rather than later, only because it's needed to rescue those methods of BasicRepository.

Otherwise this simply would not have come up.