Make responsibilities more focused and clear #89
Conversation
| class SqlResult(val resultSet: java.sql.ResultSet) extends ResultSetWrapper with CollectionsSqlResult { | ||
|
|
||
| def as[A: RowParser](): A = implicitly[RowParser[A]].parse(asRow) | ||
| class SqlResult(val resultSet: java.sql.ResultSet) extends CollectionsSqlResult { |
There was a problem hiding this comment.
should we make resultSet private?
| @@ -42,20 +42,16 @@ class SqlResult(val resultSet: java.sql.ResultSet) extends ResultSetWrapper with | |||
| val mm: mutable.MultiMap[U, V] = new mutable.HashMap[U, mutable.Set[V]] with mutable.MultiMap[U, V] | |||
| withResultSet { resultSet => | |||
| while (resultSet.next()) { | |||
| val parsed = parser(asRow) | |||
| val parsed = parser(SqlRow(resultSet)) | |||
There was a problem hiding this comment.
would it be worth replacing asRow with currentRow?
There was a problem hiding this comment.
If I did, I'd want it to be private to relate, as it doesn't really align with the rest of the public-facing interface, which all involves processing the whole ResultSet in one go. But really SqlRow(resultSet) isn't that hard to type so I think I'll just leave it
| if (_hasNext) { | ||
| _hasNext = result.next() | ||
| _hasNext = result.resultSet.next() | ||
| } | ||
|
|
||
| // if we've iterated through the whole thing, close resources |
There was a problem hiding this comment.
Something I noticed, when looking over this, is that if we stop iterating over the iterator, then we won't close the resultSet.
I wonder if we should also replace asIterator with withIterator that calls a function with the Iterator, and ensure that it is closed at the end.
There was a problem hiding this comment.
I think asIterator intentionally skips the auto-closing stuff to give the user more control. I looked into this but wasn't able to quickly decide a good way to structure things as there are a lot of different pieces involved (e.g. this is the only place that uses StreamedStatementPreparer). I think it could be good to take a more comprehensive look at cleaning up/improving relate, but my main goal here is just to try to track down some data service issues, so I'm going to say this is out of scope
| if (_hasNext) { | ||
| _hasNext = result.next() | ||
| _hasNext = result.resultSet.next() | ||
| } | ||
|
|
||
| // if we've iterated through the whole thing, close resources |
| @@ -13,7 +13,7 @@ object SqlRow { | |||
| def apply(rs: java.sql.ResultSet): SqlRow = new SqlRow(rs) | |||
| } | |||
|
|
|||
| class SqlRow(val resultSet: java.sql.ResultSet) extends ResultSetWrapper { | |||
| class SqlRow(val resultSet: java.sql.ResultSet) { | |||
There was a problem hiding this comment.
Given this:
That's not their purpose though—SqlRow represents a single row and shouldn't expose implementation details allowing it to access the whole result set
Should we make resultSet private?
| } else { | ||
| None | ||
| } | ||
| def asScalarOption[A](): Option[A] = withResultSet { resultSet => |
There was a problem hiding this comment.
Can we make this asCollection[A, Option](_.asInstanceOf[A])?
There was a problem hiding this comment.
Honestly, I'd rather go the opposite direction, and make asSingle and asSingleOption stop using asCollection
There was some ambiguity in how some things should work (specifically, `asScalarOption` and `as` in SqlResult) due to the very general nature of things extending SqlResultWrapper. SqlResultWrapper made all the methods on ResultSet available, which resulted in SqlRow and SqlResult both being usable like a ResultSet. That's not their purpose though— SqlRow represents _a single row_ and shouldn't expose implementation details allowing it to access the whole result set (similarly, RowParser should parse _a single row_ and never parse or be able to parse or access anything beyond that row) and SqlResult represents the result set as a whole, abstracting away the underlying implementation details. These two classes _should not_ be interchangeable. This commit streamlines the interface of the two mentioned classes so that their purpose is more clear, and so that their behavior can be more clearly indicated by their purpose. Methods that did not align with the ostensible purpose of each class were removed.
Before the previous commit, it perhaps made sense for these methods to not close the ResultSet—the SqlResult exposed methods to iterate through the results one-by-one, and these methods only deal with the current row.
coreywoodfield
force-pushed
the
cwoodfield-SqlResult-asScalarOption
branch
from
d75acd5 to
c7ed6ba
Compare
No description provided.