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

Need a way to indicate no additional restrictions #947

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 Jan 23, 2025

This came up during discussion of one of the other issues.
Given a repository method that allows Restrictions to be supplied at run time, such as,

@Find
List<Person> bornIn(int year, Restriction<Person> restrictions, Order<Person> order);

There is currently no way for a repository user to use the method when a case arises where they don't need to make any additional restrictions.

Currently, allowing for that possibility would require the repository to duplicate the method and omit the Restriction parameter,

@Find
List<Person> bornIn(int year, Order<Person> order);

One idea to avoid the need to duplicate repository methods is to allow an empty Restriction instance that indicates the decision to not apply any additional restrictions,

everyoneBornIn2000 = people.bornIn(2000, Restrict.none(), Order.by(_Person.ssn.asc()));

I'm not sure what a good name for it is. This PR calls it Restrict.none(), but open to better ideas for names.

@njr-11 njr-11 added this to the 1.1 milestone Jan 23, 2025
@otaviojava
Copy link
Contributor

@njr-11 shall we combine those?

One is for static query, and the other one is for dynamic.

Using your sample:

@Find
List<Person> bornIn(int year, Restriction<Person> restrictions, Order<Person> order);

I could you do the same using:

@Find
List<Person> bornIn(Restriction<Person> restrictions, Order<Person> order);

And implement this way:

Restriction<Person> restriction = _Person.age().in(List.of(1,2,3);

I am inclined to run an exception when the user tries this approach.

@gavinking
Copy link
Contributor

I think Restrict.none() is pretty ambiguous, and could be read as "restrict to none". That's why I called it unrestricted() on the Hibernate API.

@njr-11
Copy link
Contributor Author

njr-11 commented Jan 24, 2025

@njr-11 shall we combine those?

One is for static query, and the other one is for dynamic.

Using your sample:

@Find
List<Person> bornIn(int year, Restriction<Person> restrictions, Order<Person> order);

I could you do the same using:

@Find
List<Person> bornIn(Restriction<Person> restrictions, Order<Person> order);

And implement this way:

Restriction<Person> restriction = _Person.age().in(List.of(1,2,3);

I am inclined to run an exception when the user tries this approach.

@otaviojava I think you misunderstood the point. The presence or absence of a statically defined restriction is not important at all here. We could instead give an example that doesn't use int year, and the exact same problem arises regardless.

A repository provides the following,

@Find
List<Person> findAll(Restriction<Person> restrictions, Order<Person> order);

The repository user computes restrictions at run time, and this works great as long as the set of restrictions is non-empty, but as soon as the the scenario arises that nothing should be restricted, the method cannot be used anymore, and the repository author ends up needing to a duplicate method, but without the Restriction<Person> restrictions,

@Find
List<Person> findAll(Order<Person> order);

This is where a way to represent "no restrictions" is useful, so that duplicated methods can be avoided, and the computation at run time doesn't need to be annoyed with whether to invoke one method or another.

@njr-11
Copy link
Contributor Author

njr-11 commented Jan 24, 2025

I think Restrict.none() is pretty ambiguous, and could be read as "restrict to none". That's why I called it unrestricted() on the Hibernate API.

Good idea. I renamed it to unrestricted() in 48e2c3a

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