Prevent mutation of various records#1353
Conversation
| this.pageRequest = copy(pageRequest); | ||
| this.nextPageRequest = copy(nextPageRequest); | ||
| this.previousPageRequest = copy(previousPageRequest); |
There was a problem hiding this comment.
Why is it necessary to copy PageRequests?
There was a problem hiding this comment.
Paginationis immutable, no?
Good point, if we assume the PageRequest implementation is always the immutable built-in one, then we don't need to copy it.
| * Key values. | ||
| */ | ||
| private final Object[] key; | ||
| private final List<Object> key; |
There was a problem hiding this comment.
Actually @njr-11 I don't think we need to make such a radical change here either. This is a private field, belonging to a package private class. As long as you just clone() the array in the constructor, you are fine. No need to change it to List. That's just an extra object instantiation.
There was a problem hiding this comment.
The reason I switched to a List was that PageRequestCursor's implementation of the List<?> elements() method was already converting from array to List (and doing so each time invoked), so I thought the List conversion would need to be done at least once regardless, making the overall usage less costly than including an extra clone. However, I went back and checked, and there is an alternative to the elements() method, which is to invoke cursor.get(index) to retrieve each element individually. If an implementation is doing that instead of using cursor.elements(), then cloning the array would be more efficient as you point out. I'm not sure which approach implementations are actually going with.
There was a problem hiding this comment.
I just noticed that the only API method that invokes the PageRequestCursor constructor is Cursor.forKey(Object ...)
static Cursor forKey(Object... key) {
return new PageRequestCursor(key);
}Given that it's a varargs, it will almost always be an array instance created by Java that the application doesn't even have a reference to in order to mutate. Technically, someone could pass in an Object[] to it, in which there is risk of the array being modified. But I'm tempted to just put this code back how it was originally and do nothing about this.
There was a problem hiding this comment.
With my prior comment, I convinced myself that the original approach of no copying for Cursor.forKey is best. Technically, it is just as vulnerable to someone mutating the individual values within the array as it is to someone altering the array itself. A user doing that would be trying to make themselves fail. I'd rather just have clarifying text indicating don't do that.
There was a problem hiding this comment.
I also improved the documentation of this method while I was there.
| implements Page<T> { | ||
|
|
||
| // Disallow mutation of PageRequest and List fields after creation | ||
| public PageRecord(PageRequest pageRequest, |
There was a problem hiding this comment.
Shall we have a test to prove this behavior?
There was a problem hiding this comment.
Shall we have a test to prove this behavior?
sure, I added a test for you,
c41b0e2
4 participants
@gavinking pointed out in jakartaee/persistence#944 that records provided by the Jakarta Data API might be vulnerable to mutation. I looked into this and found 3 places (fixed in this PR) plus one other place that I did not cover in this PR.
The one that is not covered is,
A user could supply many different types of mutable objects to that method and later change them, or could invoke
value()to retrieve its value and then mutate the value, impacting subsequent invocations of.value(). However, it does not seem reasonable to try to identify all possible mutable types and provide code to copy them.