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.

Sign up for GitHub

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

Make responsibilities more focused and clear #89

Merged
merged 2 commits into from
Oct 31, 2024
Merged

Make responsibilities more focused and clear #89

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d881a6
Make classes more focused
coreywoodfield Oct 24, 2024
c7ed6ba
Make all SqlResult methods close the ResultSet when finished
coreywoodfield Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 0 additions & 42 deletions relate/src/main/scala/com/lucidchart/relate/CollectionsParser.scala
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions relate/src/main/scala/com/lucidchart/relate/CollectionsSqlResult.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ trait CollectionsSqlResult { self: SqlResult =>

withResultSet { resultSet =>
while (resultSet.getRow < maxRows && resultSet.next()) {
builder += parser(asRow)
builder += parser(SqlRow(resultSet))
}
}

Expand All @@ -41,7 +41,7 @@ trait CollectionsSqlResult { self: SqlResult =>

withResultSet { resultSet =>
while (resultSet.getRow < maxRows && resultSet.next()) {
builder += parser(asRow)
builder += parser(SqlRow(resultSet))
}
}

Expand Down
43 changes: 0 additions & 43 deletions relate/src/main/scala/com/lucidchart/relate/ResultSetWrapper.scala
View file
Open in desktop

This file was deleted.

6 changes: 3 additions & 3 deletions relate/src/main/scala/com/lucidchart/relate/RowIterator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ private[relate] object RowIterator {
private[relate] class RowIterator[A](parser: SqlRow => A, stmt: PreparedStatement, result: SqlResult)
extends Iterator[A] {

private var _hasNext = result.next()
private var _hasNext = result.resultSet.next()

/**
* Make certain that all resources are closed
Expand All @@ -32,9 +32,9 @@ private[relate] class RowIterator[A](parser: SqlRow => A, stmt: PreparedStatemen
* the parsed record
*/
override def next(): A = {
val ret = parser(result.asRow)
val ret = parser(SqlRow(result.resultSet))
if (_hasNext) {
_hasNext = result.next()
_hasNext = result.resultSet.next()
}

// if we've iterated through the whole thing, close resources
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:upvote:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand Down
22 changes: 1 addition & 21 deletions relate/src/main/scala/com/lucidchart/relate/RowParser.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ trait RowParser[A] extends (SqlRow => A) {
def apply(row: SqlRow) = parse(row)
}

object RowParser extends CollectionsParser {
object RowParser {
def apply[A](f: (SqlRow) => A) = new RowParser[A] {
def parse(row: SqlRow) = f(row)
}
Expand All @@ -25,24 +25,4 @@ object RowParser extends CollectionsParser {

private[relate] val insertInt = (row: SqlRow) => row.strictInt(1)
private[relate] val insertLong = (row: SqlRow) => row.strictLong(1)

implicit def multiMap[Key: RowParser, Value: RowParser] = RowParser[Map[Key, Set[Value]]] { result =>
val mm: mutable.Map[Key, Set[Value]] = new mutable.HashMap[Key, Set[Value]]

result.withResultSet { resultSet =>
while (resultSet.next()) {
val key = implicitly[RowParser[Key]].parse(result)
val value = implicitly[RowParser[Value]].parse(result)

mm.get(key)
.map { foundValue =>
mm += (key -> (foundValue + value))
}
.getOrElse {
mm += (key -> Set(value))
}
}
}
mm.toMap
}
}
2 changes: 0 additions & 2 deletions relate/src/main/scala/com/lucidchart/relate/SqlQuery.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,6 @@ trait Sql extends CollectionsSql {
def executeInsertSingle[U](parser: SqlRow => U)(implicit connection: Connection): U =
insertionStatement.execute(_.asSingle(parser))

def as[A: RowParser]()(implicit connection: Connection): A = normalStatement.execute(_.as[A])

/**
* Execute this query and get back the result as a single record
* @param parser
Expand Down
16 changes: 6 additions & 10 deletions relate/src/main/scala/com/lucidchart/relate/SqlResult.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package com.lucidchart.relate
import java.sql.ResultSetMetaData
import scala.collection.mutable
import scala.language.higherKinds
import scala.util.Using

object SqlResult {
def apply(resultSet: java.sql.ResultSet) = new SqlResult(resultSet)
Expand All @@ -19,9 +20,8 @@ object SqlResult {
* The extraction methods (int, string, long, etc.) also have "strict" counterparts. The "strict" methods are slightly
* faster, but do not do type checking or handle null values.
*/
class SqlResult(val resultSet: java.sql.ResultSet) extends ResultSetWrapper with CollectionsSqlResult {

def as[A: RowParser](): A = implicitly[RowParser[A]].parse(asRow)
class SqlResult(private[relate] val resultSet: java.sql.ResultSet) extends CollectionsSqlResult {
private[relate] def withResultSet[A](f: (java.sql.ResultSet) => A) = Using.resource(resultSet)(f)

def asSingle[A: RowParser](): A = asCollection[A, Seq](1).head
def asSingle[A](parser: SqlRow => A): A = asCollection[A, Seq](parser, 1).head
Expand All @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be worth replacing asRow with currentRow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

mm.addBinding(parsed._1, parsed._2)
}
}
mm.toMap.map(x => x._1 -> x._2.toSet)
}

def asScalar[A](): A = asScalarOption.get
def asScalarOption[A](): Option[A] = {
if (resultSet.next()) {
Some(resultSet.getObject(1).asInstanceOf[A])
} else {
None
}
def asScalarOption[A](): Option[A] = withResultSet { resultSet =>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this asCollection[A, Option](_.asInstanceOf[A])?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I'd rather go the opposite direction, and make asSingle and asSingleOption stop using asCollection

Option.when(resultSet.next())(resultSet.getObject(1).asInstanceOf[A])
}

/**
Expand Down
2 changes: 1 addition & 1 deletion relate/src/main/scala/com/lucidchart/relate/SqlRow.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(resultSet: java.sql.ResultSet) {

/**
* Get the number of the row the SqlResult is currently on
Expand Down
142 changes: 0 additions & 142 deletions relate/src/test/scala/ImplicitParsingTest.scala
View file
Open in desktop

This file was deleted.

Loading
Loading