dynamic query restrictions #895

njr-11 · 2024-11-05T22:49:53Z

This PR contains some ideas that I was experimenting with locally for dynamic query restrictions #829 and hadn't been planning to propose until after a lot more discussion occurred. However, because code changes are already being proposed and I wanted to illustrate how this should tie into the static metamodel, I decided to share this prematurely so everyone can see what I was originally aiming for with the pattern.

@Repository
public interface Products extends CrudRepository<Product, Long> {
    List<Product> search(String namePattern,
                         Restriction<Product> restriction,
                         Order<Product> order);
}

Usage with single restriction,

    List<Product> found = products.search(pattern,
                                          _Product.price.lessThan(50.0f),
                                          Order.by(_Product.price.desc(),
                                                   _Product.name.asc()));

Usage with multiple restrictions,

    List<Product> found = products.search(pattern,
                                          Restrict.all(_Product.price.greaterThan(25.0f),
                                                       _Product.price.lessThan(50.0f),
                                                       _Product.name.startsWith(pattern).ignoreCase(),
                                                       _Product.image.notNull()),
                                          Order.by(_Product.price.desc(),
                                                   _Product.name.asc()));

This was just the basics, and doesn't yet include not/negation, or Pattern/Range, which might make parts of it unnecessary. It also just occurred to me that if we had a TextRestriction subclass, which I did not include in this, we could omit the *IgnoreCase methods and have .ignoreCase() instead.

Signed-off-by: Nathan Rauh <[email protected]>

njr-11 · 2024-11-08T23:15:46Z

After a lot of difficulty reviewing another PR that combines several different new parts of Jakarta Data, I decided to just finish off updating this PR with how I expect Restrictions to work.

I was originally thinking that Restrictions would only come from the static metamodel, but others have expressed a desire that users be able to create them like they can for Sort. I'm okay with that, but not by giving users access to record/implementation class constructors where they can populate them with any combinations of things they like. With this PR, all implementation classes are made non-public and kept out of the API. There are only 2 places where users obtain restrictions:

static metamodel attributes (basic restrictions only)
the Restrict class (both composite and basic restrictions)

This also aims to de-emphasize interfaces that the end user shouldn't care about (and only Data Providers need, such as Operator and the Composite restriction) out of the main view by placing them in inner classes.

There is no JavaDoc in this on purpose. The idea is to iterate over the design first, and later spend time on JavaDoc and other things.

Other spec function like Pattern, Range, Is, ... are intentionally kept out so we can focus only on getting Restriction correct.

Restriction could justifiably be placed in the jakarta.data package alongside some other special classes that are used as repository parameters (Sort, Order, Limit) although we might want to consider if it is worth it to have a separate jakarta.data.restrict package to separate it out like we do with jakarta.data.page for PageRequest, which is the other special parameter type. I think the latter is a good place for it, so that's where I put it in this PR.

greater/lessThan methods require a Comparable parameter (lots of things can be sortable, but I think they are all Comparable). TextAttribute methods, and their equivalents on Restrict, require a String parameter. I added a commented out method showing what it might be appropriate to do with a Pattern parameter as well if that gets added. But ignore that if you don't want to think about Pattern yet.

I would like to see more error checking for values supplied to Restriction factory methods in general, and identify if we can use any more precise types that would help keep invalid objects out, but we can save that for when we are further along.

api/src/main/java/jakarta/data/metamodel/Attribute.java

api/src/main/java/jakarta/data/metamodel/SortableAttribute.java

api/src/main/java/jakarta/data/restrict/Restrict.java

gavinking · 2024-11-09T00:03:38Z

Restriction could justifiably be placed in the jakarta.data package

This is where I would have put it.

api/src/main/java/jakarta/data/restrict/Restrict.java

njr-11

Added a commit to cover most of Gavin's review comments.
Also, I made an update to ensure ignoreCase is only available to String values, and not to numbers, dates, and so forth.

api/src/main/java/jakarta/data/metamodel/Attribute.java

api/src/main/java/jakarta/data/metamodel/SortableAttribute.java

api/src/main/java/jakarta/data/restrict/Restrict.java

njr-11 · 2024-11-09T13:44:17Z

Restriction could justifiably be placed in the jakarta.data package

This is where I would have put it.

That's fine. I have moved it to jakarta.data in the review fixes commit.

gavinking · 2024-11-09T13:47:18Z

Added a commit to cover most of Gavin's review comments.

Thanks!

njr-11 · 2024-11-13T20:29:53Z

Switched methods such as startsWith(prefixPattern) to startsWith(prefix) and automatically add escaping to spare the user from that level of detail/complexity.

api/src/main/java/jakarta/data/Restriction.java

Co-authored-by: Gavin King <[email protected]>

api/src/main/java/jakarta/data/TextRestrictionRecord.java

otaviojava · 2024-11-21T05:43:20Z

api/src/main/java/jakarta/data/Restrict.java

+        return new TextRestrictionRecord<>(field, Operator.LIKE, ESCAPED, p);
+    }
+
+    public static <T> Restriction<T> not(Object value, String field) {


It is not clear that not means, notEquals´. Please either, use neorNotEquals`

Sure, I can update that. Given that we have a equalTo method, the equivalent here will be notEqualTo

otaviojava · 2024-11-21T05:45:07Z

api/src/main/java/jakarta/data/Restrict.java

+        return new TextRestrictionRecord<>(field, NOT, Operator.EQUAL, value);
+    }
+
+    public static <T> TextRestriction<T> notContains(String substring, String field) {


Can we remove all of those no method factory sequences?

It would be great if we have only two options: by the method instead negateand a single method factory not.

It is pretty similar to the Java Predicate: https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/function/Predicate.html

TextRestriction<Book> = TextRestriction.not(TextRestriction.contains(...)); TextRestriction<Book> = TextRestriction.contains(...).negate();

@otaviojava JDQL has operators like not between, not like, etc, so it makes sense to me for such things to be reflected here. And it's significantly less verbose.

I agree with Gavin on this.

Being able to write code such as,

products.search(_Product.description.notContains("refurbished"));

is much more straightforward and readable than

products.search(_Product.description.contains("refurbished").negated());

or

products.search(Restrict.not(_Product.description.contains("refurbished")));

otaviojava · 2024-11-21T05:47:12Z

api/src/main/java/jakarta/data/Restrict.java

+     * @throws IllegalArgumentException if the same character is supplied for
+     *                                  both wildcard types.
+     */
+    private static String toLikeEscaped(char charWildcard,


Once everybody agreed with Gavin's Pattern class, why not centralize this behavior in that class?

Yes, I think it's much better to have a nice class which models this basic concept. It's a much semantically stronger construct.

I agree, if Pattern gets added, this utility method can move to there. Also note that the method is private, so the documentation on it is only for our own benefit and will not show in the API, so users won't even notice once it is moved.

otaviojava · 2024-11-21T05:47:54Z

api/src/main/java/jakarta/data/Restriction.java

+public interface Restriction<T> {
+    boolean isNegated();
+
+    enum Operator {


Please remove this one from the inner class; it is good, mainly for the Javadoc.

I suppose I could move it to a top level class, although note that it currently is not something that applications use. Its current purpose is for Jakarta Data providers to know which type of restriction it is, so I had intentionally made in an inner class to keep it more out of the way of application developers. If you are looking ahead to the possibility of reusing it for an @Is annotation, then it would become something that is directly used by applications.

@is annotation

That is what I do have on my mind.

api/src/main/java/jakarta/data/metamodel/SortableAttribute.java

Signed-off-by: Otavio Santana <[email protected]>

otaviojava · 2024-11-21T19:27:12Z

@njr-11 done the first step: njr-11#1

Create test scenarions

njr-11 · 2024-11-21T20:00:18Z

Thanks for contributing the unit tests. That helps to confirm the correctness of the changes.
I was thinking maybe we should wait on adding JavaDoc until after this PR is merged, so that we don't need to rewrite it if anyone asks for further changes to the initial PR.

otaviojava · 2024-11-21T20:21:48Z

Thanks for contributing the unit tests. That helps to confirm the correctness of the changes. I was thinking maybe we should wait on adding JavaDoc until after this PR is merged, so that we don't need to rewrite it if anyone asks for further changes to the initial PR.

It does make sense.
I will focus on the Unit test to find some issues on it.

…mposite restriction

Signed-off-by: Otavio Santana <[email protected]>

otaviojava · 2024-11-23T09:02:47Z

@njr-11 one more PR against yours.

njr-11 · 2024-11-25T14:32:49Z

@njr-11 one more PR against yours.

@otaviojava I don't see another PR against this anywhere.

otaviojava · 2024-11-26T09:57:41Z

@njr-11 one more PR against yours.

@otaviojava I don't see another PR against this anywhere.

This one here:
njr-11#2

Create more test scenarios to the new interfaces

njr-11 · 2024-11-26T15:59:45Z

@njr-11 one more PR against yours.

@otaviojava I don't see another PR against this anywhere.

This one here: njr-11#2

I see it now and have accepted your PR/commits for more unit tests. Thanks for adding the test coverage.

Signed-off-by: Otavio Santana <[email protected]>

otaviojava · 2024-11-27T19:43:53Z

@njr-11 one more PR against yours.

@otaviojava I don't see another PR against this anywhere.

This one here: njr-11#2

I see it now and have accepted your PR/commits for more unit tests. Thanks for adding the test coverage.

@njr-11

This is the last one:

njr-11#3

I put the validation on field making the field mandatory

829 dynamic query restrictions

otaviojava · 2024-11-27T20:20:23Z

As a first step, IMHO, it is good to go.

@njr-11 @gavinking, what do you think?

njr-11 force-pushed the 829-dynamic-query-restrictions branch 2 times, most recently from da5b807 to 52d42e9 Compare November 6, 2024 19:11

njr-11 mentioned this pull request Nov 6, 2024

Add Criteria-Based Filtering #894

Closed

njr-11 force-pushed the 829-dynamic-query-restrictions branch from 59e75cc to 0f9b1d3 Compare November 8, 2024 22:35

Issue jakartaee#829 dynamic query restrictions

d41c721

Signed-off-by: Nathan Rauh <[email protected]>

njr-11 force-pushed the 829-dynamic-query-restrictions branch from 0f9b1d3 to d41c721 Compare November 8, 2024 23:12

gavinking reviewed Nov 9, 2024

View reviewed changes

api/src/main/java/jakarta/data/restrict/Restrict.java Outdated Show resolved Hide resolved

njr-11 added 2 commits November 9, 2024 07:14

Address most of the review comments from Gavin

ef9e462

prevent ignoreCase from being available to non-text restrictions

8ba74d5

njr-11 commented Nov 9, 2024

View reviewed changes

Move enum out of Restrict

9a72101

Switch from pattern to literal in methods such as startsWith

2b8a88c

njr-11 force-pushed the 829-dynamic-query-restrictions branch from a910b1f to 2b8a88c Compare November 13, 2024 20:35

add wildcards

3867d24

otaviojava reviewed Nov 20, 2024

View reviewed changes

api/src/main/java/jakarta/data/Restriction.java Outdated Show resolved Hide resolved

gavinking reviewed Nov 20, 2024

View reviewed changes

api/src/main/java/jakarta/data/Restriction.java Outdated Show resolved Hide resolved

njr-11 and others added 2 commits November 20, 2024 09:25

Update api/src/main/java/jakarta/data/Restriction.java

35dc014

Co-authored-by: Gavin King <[email protected]>

review comment to move interfaces to top level instead of inner classes

1afe5f7

otaviojava reviewed Nov 20, 2024

View reviewed changes

api/src/main/java/jakarta/data/TextRestrictionRecord.java Show resolved Hide resolved

otaviojava reviewed Nov 21, 2024

View reviewed changes

api/src/main/java/jakarta/data/metamodel/SortableAttribute.java Outdated Show resolved Hide resolved

otaviojava added 2 commits November 21, 2024 19:14

test: create scenarios to CompositeRestrictionREcord

8a93a8a

Signed-off-by: Otavio Santana <[email protected]>

test: create scenarios to TextRestrictionRecord

c6f835e

Signed-off-by: Otavio Santana <[email protected]>

Merge pull request #1 from soujava/829-dynamic-query-restrictions

711497c

Create test scenarions

njr-11 and others added 9 commits November 21, 2024 14:50

Enforce that TextRestriction.value() is a String, and reject empty co…

a8707ca

…mposite restriction

test: create initial structure to attrivute test

8bef07a

Signed-off-by: Otavio Santana <[email protected]>

test: create test to attribute scenarios

305a552

Signed-off-by: Otavio Santana <[email protected]>

test: create test structure to Sortableattribute

4fb6854

Signed-off-by: Otavio Santana <[email protected]>

style: fix imports at attributetest

a93f6e9

Signed-off-by: Otavio Santana <[email protected]>

test: create SortableAttribute test

7784d3b

Signed-off-by: Otavio Santana <[email protected]>

test: include test to sortable attribute test

8e9720e

Signed-off-by: Otavio Santana <[email protected]>

test: create structure to textattribute

6804be2

Signed-off-by: Otavio Santana <[email protected]>

test: create scenarios to TextAttribute

a87a71f

Signed-off-by: Otavio Santana <[email protected]>

Merge pull request #2 from soujava/829-dynamic-query-restrictions

8d01623

Create more test scenarios to the new interfaces

otaviojava added 5 commits November 27, 2024 19:34

feat: create field validation

a780900

Signed-off-by: Otavio Santana <[email protected]>

test: create test validation

ac428fa

Signed-off-by: Otavio Santana <[email protected]>

test: create validation to field on basic restriction

6bff08d

Signed-off-by: Otavio Santana <[email protected]>

test: create textrestriction record

822de0f

Signed-off-by: Otavio Santana <[email protected]>

fix: update import at TextRestriction

014fd07

Signed-off-by: Otavio Santana <[email protected]>

Merge pull request #3 from soujava/829-dynamic-query-restrictions

1e70a1a

829 dynamic query restrictions

otaviojava approved these changes Dec 4, 2024

View reviewed changes

otaviojava merged commit a4cd44d into jakartaee:main Dec 4, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dynamic query restrictions #895

dynamic query restrictions #895

njr-11 commented Nov 5, 2024 •

edited

Loading

njr-11 commented Nov 8, 2024 •

edited

Loading

gavinking commented Nov 9, 2024

njr-11 left a comment

njr-11 commented Nov 9, 2024

gavinking commented Nov 9, 2024

njr-11 commented Nov 13, 2024

otaviojava Nov 21, 2024

njr-11 Nov 21, 2024

otaviojava Nov 21, 2024 •

edited

Loading

gavinking Nov 21, 2024

njr-11 Nov 21, 2024

otaviojava Nov 21, 2024

gavinking Nov 21, 2024

njr-11 Nov 21, 2024

otaviojava Nov 21, 2024

njr-11 Nov 21, 2024 •

edited

Loading

otaviojava Nov 22, 2024

otaviojava commented Nov 21, 2024

njr-11 commented Nov 21, 2024

otaviojava commented Nov 21, 2024

otaviojava commented Nov 23, 2024

njr-11 commented Nov 25, 2024

otaviojava commented Nov 26, 2024

njr-11 commented Nov 26, 2024

otaviojava commented Nov 27, 2024

otaviojava commented Nov 27, 2024

dynamic query restrictions #895

dynamic query restrictions #895

Conversation

njr-11 commented Nov 5, 2024 • edited Loading

njr-11 commented Nov 8, 2024 • edited Loading

gavinking commented Nov 9, 2024

njr-11 left a comment

Choose a reason for hiding this comment

njr-11 commented Nov 9, 2024

gavinking commented Nov 9, 2024

njr-11 commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otaviojava Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njr-11 Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otaviojava commented Nov 21, 2024

njr-11 commented Nov 21, 2024

otaviojava commented Nov 21, 2024

otaviojava commented Nov 23, 2024

njr-11 commented Nov 25, 2024

otaviojava commented Nov 26, 2024

njr-11 commented Nov 26, 2024

otaviojava commented Nov 27, 2024

otaviojava commented Nov 27, 2024

njr-11 commented Nov 5, 2024 •

edited

Loading

njr-11 commented Nov 8, 2024 •

edited

Loading

otaviojava Nov 21, 2024 •

edited

Loading

njr-11 Nov 21, 2024 •

edited

Loading