-
Notifications
You must be signed in to change notification settings - Fork 34
Update spec to reflect addition of Constraint and @Is #1170
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
base: main
Are you sure you want to change the base?
Update spec to reflect addition of Constraint and @Is #1170
Conversation
Co-authored-by: Nathan Rauh <[email protected]>
| - a `PageRequest` splits results into pages. A parameter of this type must be declared when the repository method returns a `Page` of results, as specified below in <<Offset-based pagination>>, or a `CursoredPage`, as specified in <<Cursor-based pagination>>. | ||
|
|
||
| The types `Restriction`, `Sort`, and `Order` are parameterized by an entity type. | ||
| Any special parameter declared with one of these types must be declared with type argument `E` or `? super E` where `E` is the queried entity type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Data 1.0, we allowed ? for Sort and Order. I think we need an allowance here for that usage to remain valid,
| Any special parameter declared with one of these types must be declared with type argument `E` or `? super E` where `E` is the queried entity type. | |
| Any special parameter declared with one of these types must be declared with type argument `E` or `? super E` where `E` is the queried entity type. Usage of the generic type argument `?` in a special parameter of type `Sort<?>...` is deprecated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Data 1.0, we allowed
?forSortandOrder.
Where precisely did we say that?
I found it in two code examples, but not in any text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we didn't say it anywhere for Order, only Sort, so I'll update the suggestion to only say it about Sort. In addition to the code examples (I see the 2 in the spec doc and maybe 5 move in the javadoc), we also have this statement:
Generic, untyped Sort criteria can be supplied directly to a repository method with a variable arguments Sort<?>... parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the exception if someone passes a Sort<Author> to a repository method that queries Book :-/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the exception if someone passes a
Sort<Author>to a repository method that queriesBook:-/
I don't think the spec says anything on this, but I would say that IllegalArgumentException seems appropriate if an implementation were to detect it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tsk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gavinking how are going to solve this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I agree with @njr-11 that we should have removed those references to Sort<?>.... It was an oversight to leave them there.
Perhaps we could say that this usage is deprecated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could say that this usage is deprecated?
Good idea. That will put us on the path toward removing it completely when we move up major versions at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created #1270 to deprecate it.
Rebase of #1159.