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

add error handling for fetching stream with always encrypted #621

Merged
merged 3 commits into from
Dec 9, 2017

Conversation

yukiwongky
Copy link
Contributor

@yukiwongky yukiwongky commented Dec 8, 2017

#615


This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage increased (+0.001%) to 72.542% when pulling b9579a2 on v-kaywon:AEStreamError into 05f9a54 on Microsoft:dev.

@codecov-io
Copy link

codecov-io commented Dec 8, 2017

Codecov Report

Merging #621 into dev will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #621      +/-   ##
==========================================
- Coverage   77.17%   77.15%   -0.02%     
==========================================
  Files          25       25              
  Lines        7308     7315       +7     
==========================================
+ Hits         5640     5644       +4     
- Misses       1668     1671       +3
Impacted Files Coverage Δ
...x86/php-7.1.12-src/ext/sqlsrv/shared/core_conn.cpp 72.67% <0%> (-0.23%) ⬇️
...php-7.1.12-src/ext/pdo_sqlsrv/shared/core_stmt.cpp 62.91% <0%> (-0.2%) ⬇️
...x86/php-7.1.12-src/ext/sqlsrv/shared/core_stmt.cpp 82.69% <0%> (-0.05%) ⬇️
...php-7.1.12-src/ext/pdo_sqlsrv/shared/core_sqlsrv.h 75.09% <0%> (ø) ⬆️
...php/x86/php-7.1.12-src/ext/pdo_sqlsrv/pdo_util.cpp 90.21% <0%> (ø) ⬆️
...x86/php-7.1.12-src/ext/sqlsrv/shared/core_sqlsrv.h 78.37% <0%> (ø) ⬆️
...rojects/php/x86/php-7.1.12-src/ext/sqlsrv/util.cpp 84.69% <0%> (ø) ⬆️
...6/php-7.1.12-src/ext/sqlsrv/shared/core_stream.cpp 84.5% <0%> (+2.81%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05f9a54...2bf915c. Read the comment docs.

Copy link
Contributor

@yitam yitam left a comment

Choose a reason for hiding this comment

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

Done my first round

@@ -1711,6 +1711,9 @@ void core_get_field_common( _Inout_ sqlsrv_stmt* stmt, _In_ SQLUSMALLINT field_i
// for how these fields are used.
case SQLSRV_PHPTYPE_STREAM:
{
CHECK_CUSTOM_ERROR(stmt->conn->ce_option.enabled, stmt, SQLSRV_ERROR_ENCRYPTED_STREAM_FETCH) {
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.

please fix the spacing

{
SQLSRV_ERROR_ENCRYPTED_STREAM_FETCH,
{ IMSSP, (SQLCHAR*) "Connection with Column Encryption enabled does not support fetching stream. Please fetch the data as a string.", -84, false }
},
{ UINT_MAX, {} }
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to add this error in PDO when it doesn't support returning streams?

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 the error is thrown in the core code, it needs to be here to compile.


$tableName = "test_max_fields";
dropTable($conn, $tableName);
AE\createTable($conn, $tableName, array(new AE\ColumnMeta("varchar(max)", "varchar_max_col")));
Copy link
Contributor

Choose a reason for hiding this comment

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

there's no need to drop table here because createTable will do it :)

} else {
fatalError("Error in getting stream!");
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps simplify this checking because $stream is expected to fail with AE?

Copy link
Contributor

Choose a reason for hiding this comment

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

use PHP_STREAM_BUFFER_SIZE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yitam But the test should work without AE too right?
@lilgreenbird where did you get that constant from? somehow when I put it in the test it returns undefined constant

Copy link
Contributor

Choose a reason for hiding this comment

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

I just thought you could check if AE is enabled first and expected it to fail with the correct message?
Otherwise, it should pass with the results matched?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.001%) to 72.542% when pulling 2bf915c on v-kaywon:AEStreamError into 05f9a54 on Microsoft:dev.

@yukiwongky yukiwongky merged commit 54ca7ff into microsoft:dev Dec 9, 2017
@yukiwongky yukiwongky deleted the AEStreamError branch January 15, 2018 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants