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

other types of comparisons for parameter based automatic query #892

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

njr-11
Copy link
Contributor

@njr-11 njr-11 commented Nov 5, 2024

Here is what a solution to #857 might look like that enables comparisons other than equality for parameter based automatic query, making that pattern more complete and better able to match what Query by Method Name can do. It is very concise and is all isolated to a single annotation in which the comparison types are specified by enum constants that read very naturally when used.

This allows us to switch basic code examples from Query by Method Name over to this more explicit pattern that does not seem like magic or require already having familiarity with query language. I did so for several examples under this PR, although it might be best to view them in generated JavaDoc rather than source due to all of the escaping needed for @, <, > characters.

I didn't go through and comprehensively switch everything over because this approach will likely be debated for a while.

@gavinking
Copy link
Contributor

gavinking commented Nov 5, 2024

Honestly this isn't as bad as I had feared in terms of blowing up the API surface.

Nevertheless, I think we should seriously consider the alternative proposed by @beikov where Pattern is a class, and you can flexibly instantiate it, for example:

  • Pattern.literal("literal string").ignoreCase()
  • Pattern.of("some%pattern")
  • Pattern.of("some*pattern").wildcard('*')

And for > and < I would just add a Range class:

  • Range.from(0)
  • Range.to(100)
  • Range.between('a', 'z')

But if you really want typed upper and lower bounds, we could have From and To (or Max/Min or UpperBound/LowerBound, whatever) classes as well.

It seems to me that this just fits much more comfortably into the whole framework we've already established. It's consistent with how we handle Limits, Sorts, Restrictions, etc, etc. And in certain ways it's more flexible.

You could write:

List<Book> byTitleAndDate(Pattern title, Range<LocalDate> publicationDate);

And call it like this:

var books = bookshop.byTitleAndDate(Pattern.of("Jakarta %")).ignoreCase(), 
                                    Range.between(LocalDate.now().minusYears(3), LocalDate.now()))

@gavinking
Copy link
Contributor

Ooooh, I also just realized that Pattern and Range might work rather nicely with Restrictions, in the sense that they simplify the number of distinct operations. Consider:

_Book.title.like(Pattern.of("some%pattern").ignoreCase())

We now don't have to worry about distinct like() and likeIgnoreCase() operations.

Similarly:

_Book.pages.in(Range.from(100))
_Book.publicationDate.in(Range.between(LocalDate.now().minusYears(3), LocalDate.now()))

I love the composability of this, and the potential for future extensibility.

And it nicely brings together and unifies the two things into one facility.

@njr-11
Copy link
Contributor Author

njr-11 commented Nov 5, 2024

This PR/issue is aiming for a statically defined query alternative to Query by Method Name by filling in some gaps in parameter-based automatic methods. The two comments above, which are good ideas to explore, are for dynamically / at-runtime composed queries, which we do also want to cover in Jakarta Data, but I think we should discuss them in the issue(s) for dynamic.

@gavinking
Copy link
Contributor

This PR/issue is aiming for a statically defined query alternative to Query by Method Name by filling in some gaps in parameter-based automatic methods. The two comments above, which are good ideas to explore, are for dynamically / at-runtime composed queries, which we do also want to cover in Jakarta Data, but I think we should discuss them in the issue(s) for dynamic.

I understand that. But I guess I'm implicitly suggesting that this is not a very useful distinction to make in the APIs, and is leading us into fragmentation and Perl-like TMTOWTDI. What I've been looking for is solutions which don't involve fragmentation, but rather unification of things we already have with things that we want to add.

@njr-11
Copy link
Contributor Author

njr-11 commented Nov 5, 2024

This does unify with what we already have. It fits in perfectly with parameter-based automatic query and fills in a gaping hole where that pattern currently only offers the static pattern for equality comparisons and no static way of defining other useful comparisons like Query by Method Name does.

@njr-11
Copy link
Contributor Author

njr-11 commented Nov 7, 2024

Per discussion in issue #857, I removed the Op enumeration, and made the constants into Strings on the Is annotation. This allows for vendor extensions. It also makes the spec-provided options even more self documenting and obvious to the user.

@Target(ElementType.PARAMETER)
public @interface Is {
// TODO add JavaDoc with examples to these
String EQUAL = "EQUAL";
Copy link
Contributor

@otaviojava otaviojava Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the power of flexibility of String; however, a typo can be a disaster; plus, the enum can show the available options by IDE.

In this case, I still prefer the enum; if a vendor wants to externalize this behavior, it can easily create its annotation.

I like the Effective Java item 34 in this case, thus, Enum instead of constant.

Copy link
Contributor Author

@njr-11 njr-11 Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this to make it more agreeable to Gavin. My own preference was for enum which I do still prefer, but I don't think it is a big deal one way or the other and would be happy with either. If we did leave it this way, I'm not at all worried about typos because the user doesn't write these, they use the constants we define in the spec (or that a vendor defines in their API). And yes, having a vendor extend behavior with an annotation of their seems perfectly reasonable to me, but I don't think Gavin is going to be on board with that and he was the one who brought up not being extensible as a drawback. In any case, if the two of you can agree on one way or the other (enum or String constant), I'll switch it to that because I really am fine with either.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would vote to keep an enum or reuse the By annotation to give the provider more flexibility.

#857 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine, I put the enum back for now. I was only anticipating that Gavin would prefer it with String constants to enable vendor extensions without an annotation, but I'm not actually sure of that. This is also now updated with the shorter ANY_CASE, and I added in the full list of possible enum constants, which I realize might need to be pared down later to gain agreement on getting this in.

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

Successfully merging this pull request may close these issues.

3 participants