-
Notifications
You must be signed in to change notification settings - Fork 38k
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
DataAccessUtils result accessors with Optional return value #27735
DataAccessUtils result accessors with Optional return value #27735
Conversation
Using the * form of import should be avoided - java.util.*. |
if (results == null) { | ||
return null; | ||
} | ||
List<T> resultList = results.toList(); |
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.
Wouldn't it be better to do a
.limit(2).toList()
and then avoid opening the second stream and instead do
resultList.get(0)
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.
Hi @marschall,
thanks for reviewing, sounds good, i refactored this. Does it make more sense now?
throw new IncorrectResultSizeDataAccessException(1, resultList.size());
as we limited the resultList size with .limit, this would not report the original passed Stream results size though but instead a maximum of 2... is that an issue?
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.
Yes, looks better to me.
return null; | ||
} | ||
Iterable<T> iterable = () -> results; | ||
List<T> resultList = StreamSupport.stream(iterable.spliterator(), false).toList(); |
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.
Why a stream instead of just
results.next()
Thanks for the tip, i adjusted that. |
if (results == null) { | ||
return null; | ||
} | ||
List<T> resultList = results.limit(2).toList(); |
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.
There is a trade off here and in #optionalResult
whether we should call #close
on the stream:
On one hand Java convention is the code who creates a "resource" is responsible for disposing it, in this case the caller of this method would be responsible to call #close
on the streams where required.
On the other hand failing to call #close
on some streams, like the ones returned by JdbcOperations#queryForStream
, can lead to resource leaks. Having the caller put streams in a try-with-resources block is both cumbersome and error prone.
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.
Makes sense. The passed stream is now closed in both methods. Also e.g. hibernate's Query.stream() documentation suggests the stream should be closed afterwards, so not so far fetched 👍
spring-tx/src/main/java/org/springframework/dao/support/DataAccessUtils.java
Outdated
Show resolved
Hide resolved
spring-tx/src/main/java/org/springframework/dao/support/DataAccessUtils.java
Outdated
Show resolved
Hide resolved
spring-tx/src/main/java/org/springframework/dao/support/DataAccessUtils.java
Outdated
Show resolved
Hide resolved
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.
Hi,
This is my first review comments in any open source project. Kindly let me know if my comments are invalid.
…cessUtils.java re-use newly added singleResult methods Co-authored-by: Vigneswaran <[email protected]>
…cessUtils.java re-use newly added singleResult methods Co-authored-by: Vigneswaran <[email protected]>
…cessUtils.java re-use newly added singleResult methods Co-authored-by: Vigneswaran <[email protected]>
spring-tx/src/main/java/org/springframework/dao/support/DataAccessUtils.java
Outdated
Show resolved
Hide resolved
spring-tx/src/main/java/org/springframework/dao/support/DataAccessUtils.java
Outdated
Show resolved
Hide resolved
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.
Modifying the exception to make it clear that size of the underlying Stream/Iterator is not known
…cessUtils.java use "public IncorrectResultSizeDataAccessException(int expectedSize)" in stream and iterator method Co-authored-by: Kwangyong Kim <[email protected]>
…cessUtils.java use "public IncorrectResultSizeDataAccessException(int expectedSize)" in stream and iterator method Co-authored-by: Vigneswaran <[email protected]>
Closes #27728
(#27728)
Hi everyone, this is my first PR in any project here on github with the goal of improving my understanding of java. Hope i understood the enhancement request correctly.