fix(Data): RecordSet issue since 1.10.0 (#4739)

* fix(Data): RecordSet issue since 1.10.0

* fix(RecordSet): copy, move #4525

Data/SQLite/testsuite/src/SQLiteTest.cpp

@@ -238,6 +238,29 @@ class TypeHandler<Person>
 } } // namespace Poco::Data
 
 
+RecordSet getRecordsetMove(Session& session, const std::string& sql)
+{
+	Statement select(session);
+	select << sql;
+	select.execute();
+
+	// return directly
+	return RecordSet(select);
+}
+
+
+RecordSet getRecordsetCopyRVO(Session& session, const std::string& sql)
+{
+	Statement select(session);
+	select << sql;
+	select.execute();
+
+	// return temp copy (RVO)
+	RecordSet recordSet(select);
+	return recordSet;
+}
+
+
 int SQLiteTest::_insertCounter;
 int SQLiteTest::_updateCounter;
 int SQLiteTest::_deleteCounter;
@@ -3636,6 +3659,96 @@ void SQLiteTest::testTransactionTypeProperty()
 }
 
 
+void SQLiteTest::testRecordsetCopyMove()
+{
+	Session session(Poco::Data::SQLite::Connector::KEY, ":memory:");
+
+	{
+		auto recordSet = getRecordsetMove(session, "SELECT sqlite_version()");
+		assertTrue(recordSet.moveFirst());
+	}
+
+	{
+		auto recordSet = getRecordsetCopyRVO(session, "SELECT sqlite_version()");
+		assertTrue(recordSet.moveFirst());
+	}
+
+	session << "CREATE TABLE Vectors (int0 INTEGER, flt0 REAL, str0 VARCHAR)", now;
+
+	std::vector<Tuple<int, double, std::string> > v;
+	v.push_back(Tuple<int, double, std::string>(1, 1.5f, "3"));
+	v.push_back(Tuple<int, double, std::string>(2, 2.5f, "4"));
+	v.push_back(Tuple<int, double, std::string>(3, 3.5f, "5"));
+	v.push_back(Tuple<int, double, std::string>(4, 4.5f, "6"));
+
+	session << "INSERT INTO Vectors VALUES (?,?,?)", use(v), now;
+
+	RecordSet rset(session, "SELECT * FROM Vectors");
+	std::ostringstream osLoop;
+	RecordSet::Iterator it = rset.begin();
+	RecordSet::Iterator end = rset.end();
+	for (int i = 1; it != end; ++it, ++i)
+	{
+		assertTrue(it->get(0) == i);
+		osLoop << *it;
+	}
+	assertTrue(!osLoop.str().empty());
+	std::ostringstream osCopy;
+	std::copy(rset.begin(), rset.end(), std::ostream_iterator<Row>(osCopy));
+	assertTrue(osLoop.str() == osCopy.str());
+
+	// copy
+	RecordSet rsetCopy(rset);
+	osLoop.str("");
+	it = rsetCopy.begin();
+	end = rsetCopy.end();
+	for (int i = 1; it != end; ++it, ++i)
+	{
+		assertTrue(it->get(0) == i);
+		osLoop << *it;
+	}
+	assertTrue(!osLoop.str().empty());
+
+	osCopy.str("");
+	std::copy(rsetCopy.begin(), rsetCopy.end(), std::ostream_iterator<Row>(osCopy));
+	assertTrue(osLoop.str() == osCopy.str());
+
+	// move
+	RecordSet rsetMove(std::move(rsetCopy));
+	osLoop.str("");
+	it = rsetMove.begin();
+	end = rsetMove.end();
+	for (int i = 1; it != end; ++it, ++i)
+	{
+		assertTrue(it->get(0) == i);
+		osLoop << *it;
+	}
+	assertTrue(!osLoop.str().empty());
+
+	osCopy.str("");
+	std::copy(rsetMove.begin(), rsetMove.end(), std::ostream_iterator<Row>(osCopy));
+	assertTrue(osLoop.str() == osCopy.str());
+
+	// moved from object must remain in valid unspecified state
+	// and can be reused
+	assertEqual(0, rsetCopy.rowCount());
+	rsetCopy = (session << "SELECT * FROM Vectors", now);
+	assertEqual(v.size(), rsetCopy.rowCount());
+	osLoop.str("");
+	it = rsetCopy.begin();
+	end = rsetCopy.end();
+	for (int i = 1; it != end; ++it, ++i)
+	{
+		assertTrue(it->get(0) == i);
+		osLoop << *it;
+	}
+	assertTrue(!osLoop.str().empty());
+	osCopy.str("");
+	std::copy(rsetCopy.begin(), rsetCopy.end(), std::ostream_iterator<Row>(osCopy));
+	assertTrue(osLoop.str() == osCopy.str());
+}
+
+
 void SQLiteTest::setUp()
 {
 }
@@ -3746,6 +3859,7 @@ CppUnit::Test* SQLiteTest::suite()
 	CppUnit_addTest(pSuite, SQLiteTest, testFTS3);
 	CppUnit_addTest(pSuite, SQLiteTest, testIllegalFilePath);
 	CppUnit_addTest(pSuite, SQLiteTest, testTransactionTypeProperty);
+	CppUnit_addTest(pSuite, SQLiteTest, testRecordsetCopyMove);
 
 	return pSuite;
 }

Data/SQLite/testsuite/src/SQLiteTest.h

@@ -144,6 +144,8 @@ class SQLiteTest: public CppUnit::TestCase
 	void testIllegalFilePath();
 	void testTransactionTypeProperty();
 
+	void testRecordsetCopyMove();
+
 	void setUp();
 	void tearDown();
 

Data/include/Poco/Data/RecordSet.h

@@ -68,7 +68,7 @@ class Data_API RecordSet: private Statement
 	/// a limit for the Statement.
 {
 public:
-	using RowMap = std::map<std::size_t, Row*>;
+	using RowMap = std::map<std::size_t, std::shared_ptr<Row>>;
 	using ConstIterator = const RowIterator;
 	using Iterator = RowIterator;
 

Data/include/Poco/Data/Statement.h

@@ -524,6 +524,9 @@ class Data_API Statement
 	Session session();
 		/// Returns the underlying session.
 
+	void clear() noexcept;
+		/// Clears the statement.
+
 private:
 	const Result& doAsyncExec(bool reset = true);
 		/// Asynchronously executes the statement.

Data/src/RecordSet.cpp

@@ -60,10 +60,11 @@ RecordSet::RecordSet(Session& rSession,
 
 
 RecordSet::RecordSet(const RecordSet& other):
-	Statement(other.impl()),
+	Statement(other),
 	_currentRow(other._currentRow),
 	_pBegin(new RowIterator(this, 0 == rowsExtracted())),
 	_pEnd(new RowIterator(this, true)),
+	_rowMap(other._rowMap),
 	_pFilter(other._pFilter),
 	_totalRowCount(other._totalRowCount)
 {
@@ -72,12 +73,21 @@ RecordSet::RecordSet(const RecordSet& other):
 
 RecordSet::RecordSet(RecordSet&& other) noexcept:
 	Statement(std::move(other)),
-	_currentRow(std::move(other._currentRow)),
-	_pBegin(std::move(other._pBegin)),
-	_pEnd(std::move(other._pEnd)),
-	_pFilter(std::move(other._pFilter)),
-	_totalRowCount(std::move(other._totalRowCount))
+	_currentRow(other._currentRow),
+	_pBegin(new RowIterator(this, 0 == rowsExtracted())),
+	_pEnd(new RowIterator(this, true)),
+	_rowMap(std::move(other._rowMap)),
+	_pFilter(other._pFilter),
+	_totalRowCount(other._totalRowCount)
 {
+	other._currentRow = 0;
+	delete other._pBegin;
+	other._pBegin = nullptr;
+	delete other._pEnd;
+	other._pEnd = nullptr;
+	other._rowMap.clear();
+	other._pFilter.reset();
+	other._totalRowCount = UNKNOWN_TOTAL_ROW_COUNT;
 }
 
 
@@ -87,10 +97,6 @@ RecordSet::~RecordSet()
 	{
 		delete _pBegin;
 		delete _pEnd;
-
-		RowMap::iterator it = _rowMap.begin();
-		RowMap::iterator end = _rowMap.end();
-		for (; it != end; ++it) delete it->second;
 	}
 	catch (...)
 	{
@@ -103,10 +109,19 @@ RecordSet& RecordSet::operator = (RecordSet&& other) noexcept
 {
 	Statement::operator = (std::move(other));
 	_currentRow = std::move(other._currentRow);
-	_pBegin = std::move(other._pBegin);
-	_pEnd = std::move(other._pEnd);
+	other._currentRow = 0;
+	_pBegin = new RowIterator(this, 0 == rowsExtracted());
+	delete other._pBegin;
+	other._pBegin = nullptr;
+	_pEnd = new RowIterator(this, true);
+	delete other._pEnd;
+	other._pEnd = nullptr;
+	_rowMap = std::move(other._rowMap);
+	other._rowMap.clear();
 	_pFilter = std::move(other._pFilter);
+	other._pFilter.reset();
 	_totalRowCount = std::move(other._totalRowCount);
+	other._totalRowCount = UNKNOWN_TOTAL_ROW_COUNT;
 
 	return *this;
 }
@@ -121,9 +136,6 @@ void RecordSet::reset(const Statement& stmt)
 	_currentRow = 0;
 	_totalRowCount = UNKNOWN_TOTAL_ROW_COUNT;
 
-	RowMap::iterator it = _rowMap.begin();
-	RowMap::iterator end = _rowMap.end();
-	for (; it != end; ++it) delete it->second;
 	_rowMap.clear();
 
 	Statement::operator = (stmt);
@@ -487,7 +499,7 @@ Row& RecordSet::row(std::size_t pos)
 	}
 	else
 	{
-		pRow = it->second;
+		pRow = it->second.get();
 		poco_check_ptr (pRow);
 	}
 
@@ -497,7 +509,7 @@ Row& RecordSet::row(std::size_t pos)
 
 std::size_t RecordSet::rowCount() const
 {
-	if (extractions().size() == 0) return 0;
+	if (!impl() || extractions().size() == 0) return 0;
 
 	std::size_t rc = subTotalRowCount();
 	if (!isFiltered()) return rc;

Data/src/RowIterator.cpp

@@ -41,9 +41,11 @@ RowIterator::RowIterator(const RowIterator& other):
 
 
 RowIterator::RowIterator(RowIterator&& other) noexcept:
-	_pRecordSet(std::move(other._pRecordSet)),
-	_position(std::move(other._position))
+	_pRecordSet(other._pRecordSet),
+	_position(other._position)
 {
+	other._pRecordSet = nullptr;
+	other._position = POSITION_END;
 }
 
 RowIterator::~RowIterator()

Data/src/Statement.cpp

@@ -70,23 +70,14 @@ Statement::Statement(Statement&& stmt) noexcept:
 	_parseError(std::move(stmt._parseError)),
 #endif
 	_pImpl(std::move(stmt._pImpl)),
-	_async(std::move(stmt._async)),
+	_async(stmt._async),
 	_pResult(std::move(stmt._pResult)),
 	_pAsyncExec(std::move(stmt._pAsyncExec)),
 	_arguments(std::move(stmt._arguments)),
 	_pRowFormatter(std::move(stmt._pRowFormatter)),
 	_stmtString(std::move(stmt._stmtString))
 {
-	stmt._pImpl = nullptr;
-	stmt._async = false;
-	stmt._pResult = nullptr;
-	stmt._pAsyncExec = nullptr;
-	stmt._arguments.clear();
-	stmt._pRowFormatter = nullptr;
-	_stmtString.clear();
-#ifndef POCO_DATA_NO_SQL_PARSER
-	_parseError.clear();
-#endif
+	stmt.clear();
 }
 
 
@@ -95,6 +86,22 @@ Statement::~Statement()
 }
 
 
+void Statement::clear() noexcept
+{
+	_pImpl.reset();
+	_async = false;
+	_pResult = nullptr;
+	_pAsyncExec = nullptr;
+	_arguments.clear();
+	_pRowFormatter = nullptr;
+	_stmtString.clear();
+#ifndef POCO_DATA_NO_SQL_PARSER
+	_pParseResult = nullptr;
+	_parseError.clear();
+#endif
+}
+
+
 Statement& Statement::operator = (const Statement& stmt)
 {
 	Statement tmp(stmt);
@@ -123,7 +130,7 @@ Statement& Statement::operator = (Statement&& stmt) noexcept
 	_pRowFormatter = std::move(stmt._pRowFormatter);
 	stmt._pRowFormatter = nullptr;
 	_stmtString = std::move(stmt._stmtString);
-	_stmtString.clear();
+	stmt._stmtString.clear();
 
 	return *this;
 }