blackbeam / rust-mysql-simple

Mysql client library implemented in rust.
Apache License 2.0
663 stars 144 forks source link

QueryResult.next_set does not actually advance to the next set #291

Closed infinity0 closed 2 years ago

infinity0 commented 3 years ago

https://github.com/blackbeam/rust-mysql-simple/blob/master/src/conn/query_result.rs#L156

    pub fn next_set<'d>(&'d mut self) -> Option<Result<ResultSet<'c, 't, 'tc, 'd, T>>> {
        use SetIteratorState::*;

        if let OnBoundary | Done = &self.state {
            debug_assert!(
                !self.conn.more_results_exists(),
                "the next state must be handled by the Iterator::next"
            );

            None
        } else {
            Some(Ok(ResultSet {
                set_index: self.set_index,
                inner: self,
            }))
        }
    }

This does not actually advance to the next set like repeated calls to Iterator::next() and other conventional methods called next_*. Instead, the advancing happens when iterating past the last element of ResultSet in <QueryResult as Iterator>::next when it calls QueryResult::handle_next. The caller must perform this iteration even when the ResultSet is not an actual iterable result set (InSet), but an OkPacket (InEmptySet). This must be clearly documented, so that people don't do something incorrect like:

while let Some(result) = query_result.next_set() {
    something(result.affected_rows())
    /// WRONG! doesn't iterate through `result`, so successive calls to `next_set` return the same `result`.
}

It's also unclear why the type signature should be Option<Result<_>> when the function returns either None or Some(Ok(_)), but it would be a breaking change to change that now.

infinity0 commented 3 years ago

Note that the existing documentation, "Returns an iterator over the current result set" is insufficient; this does not explicitly say nor imply that the function also doesn't advance to the next set, so the reasonable assumption of the reader is that it does. For example, the next function of an Iterator<Iterator<_>> would advance to the next set as well as return an iterator over the now-current result set. The documentation of QueryResult connotates that it is simply an Iterator<Iterator<_>> so it's confusing.