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.

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

Added empty result set check to nextRowset() #520

Merged
merged 30 commits into from
Oct 3, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
83631cd
Added empty result set check to nextRowset()
david-puglielli Sep 7, 2017
f4117a8
Added PHPT test
david-puglielli Sep 8, 2017
8666ea6
Fixed bug for sqlsrv too, sqlsrv test added
david-puglielli Sep 22, 2017
7046086
Fixed test failures by moving logic back to stmt.cpp and pdo_stmt.cpp
david-puglielli Sep 22, 2017
0d06dea
Fixed compile bug
david-puglielli Sep 22, 2017
2c28d0b
Update stmt.cpp
david-puglielli Sep 22, 2017
f11c208
Update sqlsrv_empty_result_error.phpt
david-puglielli Sep 22, 2017
be2be4b
Merge remote-tracking branch 'upstream/dev' into nextrowset-fix
david-puglielli Sep 26, 2017
270f2b4
Use core_sqlsrv_has_any_result to check for null results
david-puglielli Sep 26, 2017
1045ea4
fixed conflicts
david-puglielli Sep 26, 2017
68bb42b
Moved check to core_sqlsrv_next_result to avoid suppressing a differe…
david-puglielli Sep 27, 2017
66a12d3
Fixed errant variable name
david-puglielli Sep 27, 2017
a749010
Moved errors back to pdo_stmt and stmt.cpp
david-puglielli Sep 28, 2017
d039cf6
Fixed typos
david-puglielli Sep 28, 2017
7713b70
Fixed typos again
david-puglielli Sep 28, 2017
d56446c
More tiny fixes
david-puglielli Sep 28, 2017
3acab1a
Fixed tests and added code comments
david-puglielli Sep 28, 2017
257b1cd
Fixed test outputs
david-puglielli Sep 28, 2017
7315d44
Fixed test output
david-puglielli Sep 28, 2017
494527d
Tweaked tests, added comments per code review comments
david-puglielli Sep 29, 2017
0728a1d
Update pdo_empty_result_error.phpt
david-puglielli Sep 29, 2017
dc6e2a3
Update sqlsrv_empty_result_error.phpt
david-puglielli Sep 29, 2017
9e92867
Update pdo_stmt.cpp
david-puglielli Sep 29, 2017
4674f58
Update stmt.cpp
david-puglielli Sep 29, 2017
4151f8f
Minor fixes
david-puglielli Sep 30, 2017
95f7675
Merge branch 'nextrowset-fix' of https://github.com/david-puglielli/m…
david-puglielli Oct 2, 2017
cc0d71d
Fixed EXPECTF test output
david-puglielli Oct 2, 2017
6aa062f
Changed mssql-server-linux tag
david-puglielli Oct 2, 2017
4349a3c
Changed mssql-server-linux tag
david-puglielli Oct 2, 2017
70b20a2
Fixed regex output
david-puglielli Oct 2, 2017
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
8 changes: 8 additions & 0 deletions source/pdo_sqlsrv/pdo_stmt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1067,6 +1067,14 @@ int pdo_sqlsrv_stmt_next_rowset( _Inout_ pdo_stmt_t *stmt TSRMLS_DC )

SQLSRV_ASSERT( driver_stmt != NULL, "pdo_sqlsrv_stmt_next_rowset: driver_data object was null" );

// If SQLNumResultCols returns 0, the result set is empty. Normally this error is
// handled in core_sqlsrv_fetch, but if the user calls nextRowset() before fetch()
// the error message won't show up unless we handle it here.
SQLSMALLINT has_fields = core::SQLNumResultCols( driver_stmt TSRMLS_CC );
CHECK_CUSTOM_ERROR( has_fields == 0, driver_stmt, SQLSRV_ERROR_NO_FIELDS ) {
throw core::CoreException();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

From this page it seems valid and accurate to call this without making another trip to the server. Please confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since a statement has to be executed for the user to call nextRowset, there is no trip to the server made here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an equivalent scenario in the sqlsrv side?
Based on your comment it seems that fetch() also invokes SQLNumResultCols(), right? Would that be a duplicate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I've verified the same problem exists on the sqlsrv side. But I think core_sqlsrv_fetch is never called if nextRowset is called before fetch, which is why we're catching the error here. I'll add a fix and test for sqlsrv.

core_sqlsrv_next_result( static_cast<sqlsrv_stmt*>( stmt->driver_data ) TSRMLS_CC );

// clear the current meta data since the new result will generate new meta data
Expand Down
96 changes: 96 additions & 0 deletions test/functional/pdo_sqlsrv/pdo_empty_result_error.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
--TEST--
Test that calling nextRowset() on an empty result set produces the correct error message. Fix for Github 507.
--SKIPIF--
<?php require('skipif.inc'); ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please try to limit the length of test title. If you have more please put it in --DESCRIPTION-- instead.
For tests, it's a good practice to drop test tables / procedures at the end to leave no trace.

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 do drop them at the end, but I kept the drop functions at the beginning too just in case...

Copy link
Contributor

Choose a reason for hiding this comment

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

ahhh... I see! In the beginning you use DropTable and DropProc but later you simply use queries.

--FILE--
<?php
require_once("MsSetup.inc");

$conn = new PDO( "sqlsrv:Server = $server; Database = $databaseName; ", $uid, $pwd );
$conn->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );

$stmt = $conn->query("IF OBJECT_ID('TestEmptySetTable', 'U') IS NOT NULL DROP TABLE TestEmptySetTable");
Copy link
Contributor

Choose a reason for hiding this comment

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

There a DropTable and DropProc function you can use in MsCommon

$stmt = $conn->query("CREATE TABLE TestEmptySetTable ([c1] nvarchar(10),[c2] nvarchar(10))");

$stmt = $conn->query("INSERT INTO TestEmptySetTable (c1, c2) VALUES ('a', 'b')");
$stmt = $conn->query("IF OBJECT_ID('TestEmptySetProc', 'P') IS NOT NULL DROP PROCEDURE TestEmptySetProc");
$stmt = $conn->query("CREATE PROCEDURE TestEmptySetProc @a nvarchar(10), @b nvarchar(10)
AS SET NOCOUNT ON
BEGIN
IF @b='b'
BEGIN
SELECT 'a' as testValue
END
ELSE
BEGIN
UPDATE TestEmptySetTable SET c2 = 'c' WHERE c1 = @a
END
END");

// errors out when reaching the second nextRowset() call
// returned error indicates there are no more results
echo "Return a nonempty result set:\n";
try
{
$stmt = $conn->query("TestEmptySetProc @a='a', @b='b'");
$result = $stmt->fetchAll();
print_r($result);
$stmt->nextRowset();
$result = $stmt->fetchAll();
print_r($result);
$stmt->nextRowset();
}
catch(Exception $e)
{
echo $e->getMessage()."\n";
}

// errors out indicating the result set contains no fields
echo "Return an empty result set, call nextRowset on it before fetching anything:\n";
try
{
$stmt = $conn->query("TestEmptySetProc @a='a', @b='c'");
$stmt->nextRowset();
}
catch(Exception $e)
{
echo $e->getMessage()."\n";
}

// errors out indicating the result set contains no fields
echo "Return an empty result set, call fetch on it:\n";
try
{
$stmt = $conn->query("TestEmptySetProc @a='a', @b='c'");
$result = $stmt->fetchAll();
print_r($result);
}
catch(Exception $e)
{
echo $e->getMessage()."\n";
}

$stmt = $conn->query("DROP TABLE TestEmptySetTable");
$stmt = $conn->query("DROP PROCEDURE TestEmptySetProc");

$conn = null;
?>
--EXPECT--
Return a nonempty result set:
Array
(
[0] => Array
(
[testValue] => a
[0] => a
)

)
Array
(
)
SQLSTATE[IMSSP]: There are no more results returned by the query.
Return an empty result set, call nextRowset on it before fetching anything:
SQLSTATE[IMSSP]: The active result for the query contains no fields.
Return an empty result set, call fetch on it:
SQLSTATE[IMSSP]: The active result for the query contains no fields.