I’m having an bit of a skill issue implementing a “correct” interator.
My first version was working:
internal class ResultSetIterator(private val resultSet: ResultSet) : Iterator<Row> {
private var hasNext: Boolean = false
override fun hasNext(): Boolean {
hasNext = resultSet.next()
return hasNext
}
override fun next(): Row {
if (!hasNext) throw NoSuchElementException("No more rows in ResultSet.")
return Row(resultSet)
}
}
fun <R> getList(conn: Connection, mapper: (Row) -> R): Result<List<R>> {
return runCatching {
val (sql, args) = sqlArgs()
val stmt = conn.prepareStatement(sql)
setParameters(stmt, args)
val rs = stmt.executeQuery()
val rows = Rows(rs).iterator()
val resultList = mutableListOf<R>()
while (rows.hasNext()) {
val row = rows.next()
resultList.add(mapper(row))
}
resultList
}
}
However, im using detekt and it says a iterator hasNext method should be side effect free, ok makes sense.
But now my logic is broken and i’m getting an off by one and i just can’t find a appropriate way to make it work.
Let’s go step by step:
internal class ResultSetIterator(private val resultSet: ResultSet) : Iterator<Row> {
private var hasNext: Boolean = resultSet.next()
override fun hasNext(): Boolean {
return hasNext
}
override fun next(): Row {
if (!hasNext) throw NoSuchElementException("No more rows in ResultSet.")
// we first instantiate an row mapper to be returned, because it is already verified that hasNext is true.
val row = Row(resultSet)
// moves one position to check if there is more rows and store the result to be called by hasNext
hasNext = resultSet.next()
return row
}
}
fun <R> getList(conn: Connection, mapper: (Row) -> R): Result<List<R>> {
return runCatching {
val (sql, args) = sqlArgs()
val stmt = conn.prepareStatement(sql)
setParameters(stmt, args)
val rs = stmt.executeQuery()
val rows = Rows(rs).iterator()
val resultList = mutableListOf<R>()
// here we call hasNext, hasNext is initially set when rows is called for the first time, thus resultSet moves one row, from neutral to our very first result row.
while (rows.hasNext()) {
// rows.Next() returns the current row, and moves one position, so that hasNext will know if there is another result to be processed
val row = rows.next()
resultList.add(mapper(row))
}
resultList
}
}
As i was writing this, it ocurred to me that since i have to call result.Next() before returning the row, it shares the memory of the resultSet im passing to Row, thus mutating it internally and giving me off by one result.
Anyway, any ideas of how to implement this with a side effect free hasNext() ?
I tried mutating back the resultSet after verifying if it has a next, with resultSet.previous(), did not work: Failure(java.sql.SQLException: SQLite only supports TYPE_FORWARD_ONLY cursors)
Honestly I think your first solution is fine… I haven’t played around with writing iterators much myself, but I think the “side-effect free” thing just means “don’t advance the iterator when checking if there’s a next item in the iterator”, and your original code does that just fine. I think you should just ignore Detekt, and tell it that you’re smarter than it and you know what you’re doing.
Also this is probably unrelated, but why are you writing an iterator that wraps a ResultSet? Depending on what libraries or whatever you’re using, I’d imagine there’s already functions and stuff for iterating through a ResultSet.
operator fun ResultSet.iterator() = object : Iterator<Row> {
override fun hasNext() = !isLast
override fun next(): Row {
if (!this@iterator.next()) throw NoSuchElementException("No more rows in ResultSet.")
return Row(this@iterator)
}
}
Yep, i thought about it, and even tried, but the problem now became SQLite, it doesn’t support much from resultSet interface: SQLite only supports TYPE_FORWARD_ONLY cursors.
SQLite is a must for my case, as we use it for all our clients in my work. B2B products, so spinning up a PG server for it is just a waste of resources.
Look at existing libraries for inspiration as well. For example, this is how Jdbi does it:
@Override
public boolean hasNext() {
if (closed) {
return false;
}
if (alreadyAdvanced) {
return hasNext;
}
hasNext = safeNext();
if (hasNext) {
alreadyAdvanced = true;
} else {
close();
}
return hasNext;
}
You can see that both hasNext and alreadyAdvanced are mutated. I think iterators just have to mutate state to keep track of this kind of stuff, unfortunately.
Interesting implementation. Even though it isn’t side effect-free, it is somewhat safe to be called more than once if the row hasn’t moved. I’m definitely going to use it as inspiration, thanks!
This sounds like a case where ‘no side-effects’ can be weakened slightly to ‘no visible side-effects’.
In that implementation, the only mutation is presumably to private implementation details — so it behaves as though nothing has changed, which is what matters.
(In practice, I think it’d also need thread-safety, to make sure that no side-effects are visible even if called on multiple threads simultaneously.)
I believe your first implementation is incorrect. For two reasons:
As the warning says, hasNext has side-effects - it progresses the iterator. Iterator contract is that we should be able to call hasNext() multiple times and it shouldn’t change anything, In your implementation if called hasNext multiple times, we would skip items.
There is no requirement that we have to call hasNext() before next(). In your implementation we will get NoSuchElementException even if there are items in the result set.
I believe it is not possible to implement this fully correctly, because the Iterator interface provides a feature of checking if we have a next item without progressing and ResultSet can check only by progressing. However, if we assume we never use the current row after hasNext() and we always call next() after the hasNext(), then I think this should do:
class ResultSetIterator(private val resultSet: ResultSet) : Iterator<Row> {
var hasNext: Boolean? = null
override fun hasNext() = hasNext ?: resultSet.next().also { hasNext = it }
override fun next(): Row {
val hasNext = hasNext?.also { hasNext = null } ?: resultSet.next()
if (!hasNext) throw NoSuchElementException("No more rows in ResultSet.")
return Row(resultSet)
}
}
Disclaimer: I didn’t test the code.
The idea is that both hasNext() and next() progress the result set, but hasNext() does this only once since the last next(), while next() does it every time.
Another thing is that I don’t know what is this Row, but we can’t really use items from an iterator after moving to the next one. If Row doesn’t store the data from the ResultSet and it only provides mappings, then I think we can return exactly the same Row instance from each next call.
I’ve reached the same conclusion, keep mutations internal with no direct side effects for the caller. Even though Detekt will flag it as an error, it should be correct. it’s just a case of suppressing the lint warning.
Yes, you’re right. I was just about to refactor with an idea I got from reading the JDBI implementation, as pointed out by Skater901.
You’ve written truly beautiful code; it seems very Kotlin-idiomatic. It’s exactly what I was going to try to achieve, and so far, the tests I’ve made with it works beautifully!
For example, im calling hasNext on every iteration, it just prints out if it has, but don’t advance a row, perfect behaviour.
while (rows.hasNext()) {
println(rows.hasNext())
val row = rows.next()
resultList.add(mapper(row))
}
Btw, row is just a wrapper over the get operator to map the ResultSet to its correct type.
class Row(private val resultSet: ResultSet) {
@Suppress("UNCHECKED_CAST")
operator fun <T : Any> get(column: Column<T>): T {
return when (column.type()) {
ColumnType.INT -> resultSet.getInt(column.key()) as T
ColumnType.STRING -> {
val value = resultSet.getString(column.key())
(value ?: "") as T
}
ColumnType.LONG -> resultSet.getLong(column.key()) as T
ColumnType.FLOAT -> resultSet.getFloat(column.key()) as T
ColumnType.DOUBLE -> resultSet.getDouble(column.key()) as T
ColumnType.DECIMAL -> {
val value = resultSet.getBigDecimal(column.key())
(value ?: BigDecimal.ZERO) as T
}
ColumnType.BOOLEAN -> resultSet.getBoolean(column.key()) as T
}
}
}
I’m not sure iterators are ever thread-safe… I think by their very nature they require mutating internal state to function. I believe that iterators are only ever meant to be used by one thing; you wouldn’t create one iterator, then use it in multiple places in your code. So thread safety is not a concern, imo.
I’m not familiar with SQL but it seems to me from the doc of ResultSet that this should be relatively easy to implement without side effects ? Something like :
internal class ResultSetIterator(private val resultSet: ResultSet) : Iterator<Row> {
override fun hasNext() = not resultSet.isAfterLast()
override fun next() = try {
resultSet.next()
Row(resultSet)
} catch (e : SQLException) {
// Check this is the exception you're looking for
throw NoSuchElementException("No more rows in ResultSet.")
}
}
(not tested)
This is even simpler than the original code, it will work as expected if hasNext() is called multiple times as well as if it is never called. Am I missing something obvious ?
This still comports of a large caveat : if the hint at the end of OP is correct, then even with this version the Row is only usable until the next call to next(), since the internal members are mutated. I’d say this is pretty dangerous but all versions in this thread share this drawback, and maybe users of the Row interface are aware of this, since this seems to be a drawback of ResultSet itself. In any case if this is a problem it should be addressed independently of this question
