-
Notifications
You must be signed in to change notification settings - Fork 425
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 support for UTF-8 feature extension. #722
Changes from 11 commits
df9ef73
fccdcbb
b21fe49
26f914c
a5ca684
4c5acb4
e95e34c
5bcd514
6c1cad3
8e67788
d3de10c
d089d8f
0dac284
72d3b9b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3432,6 +3432,16 @@ int writeFedAuthFeatureRequest(boolean write, | |
return totalLen; | ||
} | ||
|
||
int writeUTF8SupportFeatureRequest(boolean write, | ||
TDSWriter tdsWriter /* if false just calculates the length */) throws SQLServerException { | ||
int len = 5; // 1byte = featureID, 4bytes = featureData length | ||
if (write) { | ||
tdsWriter.writeByte(TDS.TDS_FEATURE_EXT_UTF8SUPPORT); | ||
tdsWriter.writeInt(0); | ||
} | ||
return len; | ||
} | ||
|
||
private final class LogonCommand extends UninterruptableTDSCommand { | ||
LogonCommand() { | ||
super("logon"); | ||
|
@@ -4130,7 +4140,19 @@ private void onFeatureExtAck(int featureId, | |
serverSupportsColumnEncryption = true; | ||
break; | ||
} | ||
case TDS.TDS_FEATURE_EXT_UTF8SUPPORT: { | ||
if (connectionlogger.isLoggable(Level.FINER)) { | ||
connectionlogger.fine(toString() + " Received feature extension acknowledgement for UTF8 support."); | ||
} | ||
|
||
if (1 > data.length) { | ||
if (connectionlogger.isLoggable(Level.SEVERE)) { | ||
connectionlogger.severe(toString() + " Unknown value for UTF8 support."); | ||
} | ||
throw new SQLServerException(SQLServerException.getErrString("R_unknownUTF8SupportValue"), null); | ||
} | ||
break; | ||
} | ||
default: { | ||
// Unknown feature ack | ||
if (connectionlogger.isLoggable(Level.SEVERE)) { | ||
|
@@ -4419,6 +4441,8 @@ else if (serverMajorVersion >= 9) // Yukon (9.0) --> TDS 7.2 // Prelogin disconn | |
|
||
len2 = len2 + 1; // add 1 to length becaue of FeatureEx terminator | ||
|
||
len2 = len2 + writeUTF8SupportFeatureRequest(false, tdsWriter); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a little convoluted, the function basically always returns "5". It also has a Boolean to turn on/off the actual writing. Shouldn't we remove the length calculation from this function and just have a static variable which equals 5? Can also remove the Boolean parameter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is done to keep the same structure for all feature extension requests, in case we calculate the length differently in the future. Take a look at There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes sense with fedAuth because the length is actually variable. For the others, it seems there's a set value(6 for AE, 5 for UTF8, 6 for data classification). I guess it's fine if we're consistent, but I'm just saying that other than the fedAuth function, the others don't really make a lot of sense. |
||
|
||
// Length of entire Login 7 packet | ||
tdsWriter.writeInt(len2); | ||
tdsWriter.writeInt(tdsVersion); | ||
|
@@ -4598,6 +4622,8 @@ else if (serverMajorVersion >= 9) // Yukon (9.0) --> TDS 7.2 // Prelogin disconn | |
writeFedAuthFeatureRequest(true, tdsWriter, fedAuthFeatureExtensionData); | ||
} | ||
|
||
writeUTF8SupportFeatureRequest(true, tdsWriter); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Length is being "calculated" and returned, but we don't use the value. Seems like we're always using half of the functionalities. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes I've never liked this. It's not really "calculated" at all the whole thing is just hardcoded |
||
|
||
tdsWriter.writeByte((byte) TDS.FEATURE_EXT_TERMINATOR); | ||
tdsWriter.setDataLoggable(true); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,174 @@ | ||
/* | ||
* Microsoft JDBC Driver for SQL Server | ||
* | ||
* Copyright(c) Microsoft Corporation All rights reserved. | ||
* | ||
* This program is made available under the terms of the MIT License. See the LICENSE file in the project root for more information. | ||
*/ | ||
|
||
package com.microsoft.sqlserver.jdbc.unit; | ||
|
||
import static org.junit.jupiter.api.Assertions.assertArrayEquals; | ||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
||
import java.nio.charset.StandardCharsets; | ||
import java.sql.Connection; | ||
import java.sql.PreparedStatement; | ||
import java.sql.ResultSet; | ||
import java.sql.SQLException; | ||
import java.sql.Statement; | ||
import java.util.Collections; | ||
|
||
import org.junit.jupiter.api.AfterAll; | ||
import org.junit.jupiter.api.BeforeAll; | ||
import org.junit.jupiter.api.Test; | ||
import org.junit.platform.runner.JUnitPlatform; | ||
import org.junit.runner.RunWith; | ||
|
||
import com.microsoft.sqlserver.testframework.AbstractSQLGenerator; | ||
import com.microsoft.sqlserver.testframework.AbstractTest; | ||
import com.microsoft.sqlserver.testframework.PrepUtil; | ||
import com.microsoft.sqlserver.testframework.Utils; | ||
import com.microsoft.sqlserver.testframework.util.RandomUtil; | ||
|
||
/** | ||
* A class for testing the UTF8 support changes. | ||
*/ | ||
@RunWith(JUnitPlatform.class) | ||
public class UTF8SupportTest extends AbstractTest { | ||
private static Connection connection; | ||
private static String databaseName; | ||
private static String tableName; | ||
|
||
/** | ||
* Test against UTF8 CHAR type. | ||
* | ||
* @throws SQLException | ||
*/ | ||
@Test | ||
public void testChar() throws SQLException { | ||
if (Utils.serverSupportsUTF8(connection)) { | ||
createTable("char(10)"); | ||
validate("teststring"); | ||
// This is 10 UTF-8 bytes. D1 82 D0 B5 D1 81 D1 82 31 32 | ||
validate("тест12"); | ||
// E2 95 A1 E2 95 A4 E2 88 9E 2D | ||
validate("╡╤∞-"); | ||
|
||
createTable("char(4000)"); | ||
validate(String.join("", Collections.nCopies(400, "teststring"))); | ||
validate(String.join("", Collections.nCopies(400, "тест12"))); | ||
validate(String.join("", Collections.nCopies(400, "╡╤∞-"))); | ||
|
||
createTable("char(4001)"); | ||
validate(String.join("", Collections.nCopies(400, "teststring")) + "1"); | ||
validate(String.join("", Collections.nCopies(400, "тест12")) + "1"); | ||
validate(String.join("", Collections.nCopies(400, "╡╤∞-")) + "1"); | ||
|
||
createTable("char(8000)"); | ||
validate(String.join("", Collections.nCopies(800, "teststring"))); | ||
validate(String.join("", Collections.nCopies(800, "тест12"))); | ||
validate(String.join("", Collections.nCopies(800, "╡╤∞-"))); | ||
} | ||
} | ||
|
||
/** | ||
* Test against UTF8 VARCHAR type. | ||
* | ||
* @throws SQLException | ||
*/ | ||
@Test | ||
public void testVarchar() throws SQLException { | ||
if (Utils.serverSupportsUTF8(connection)) { | ||
createTable("varchar(10)"); | ||
validate("teststring"); | ||
validate("тест12"); | ||
validate("╡╤∞-"); | ||
|
||
createTable("varchar(4000)"); | ||
validate(String.join("", Collections.nCopies(400, "teststring"))); | ||
validate(String.join("", Collections.nCopies(400, "тест12"))); | ||
validate(String.join("", Collections.nCopies(400, "╡╤∞-"))); | ||
|
||
createTable("varchar(4001)"); | ||
validate(String.join("", Collections.nCopies(400, "teststring")) + "1"); | ||
validate(String.join("", Collections.nCopies(400, "тест12")) + "1"); | ||
validate(String.join("", Collections.nCopies(400, "╡╤∞-")) + "1"); | ||
|
||
createTable("varchar(8000)"); | ||
validate(String.join("", Collections.nCopies(800, "teststring"))); | ||
validate(String.join("", Collections.nCopies(800, "тест12"))); | ||
validate(String.join("", Collections.nCopies(800, "╡╤∞-"))); | ||
|
||
createTable("varchar(MAX)"); | ||
validate(String.join("", Collections.nCopies(800, "teststring"))); | ||
validate(String.join("", Collections.nCopies(800, "тест12"))); | ||
validate(String.join("", Collections.nCopies(800, "╡╤∞-"))); | ||
} | ||
} | ||
|
||
@BeforeAll | ||
public static void setUp() throws ClassNotFoundException, SQLException { | ||
connection = PrepUtil.getConnection(getConfiguredProperty("mssql_jdbc_test_connection_properties")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. who closes this connection? |
||
if (Utils.serverSupportsUTF8(connection)) { | ||
databaseName = RandomUtil.getIdentifier("UTF8Database"); | ||
tableName = AbstractSQLGenerator.escapeIdentifier(RandomUtil.getIdentifier("RequestBoundaryTable")); | ||
createDatabaseWithUTF8Collation(); | ||
connection.setCatalog(databaseName); | ||
} | ||
} | ||
|
||
@AfterAll | ||
public static void cleanUp() throws SQLException { | ||
if (Utils.serverSupportsUTF8(connection)) { | ||
Utils.dropDatabaseIfExists(databaseName, connection.createStatement()); | ||
} | ||
connection.close(); | ||
} | ||
|
||
private static void createDatabaseWithUTF8Collation() throws SQLException { | ||
try (Statement stmt = connection.createStatement();) { | ||
stmt.executeUpdate("CREATE DATABASE " + AbstractSQLGenerator.escapeIdentifier(databaseName) + " COLLATE Cyrillic_General_100_CS_AS_UTF8"); | ||
} | ||
} | ||
|
||
private static void createTable(String columnType) throws SQLException { | ||
try (Statement stmt = connection.createStatement();) { | ||
Utils.dropTableIfExists(tableName, stmt); | ||
stmt.executeUpdate("CREATE TABLE " + tableName + " (c " + columnType + ")"); | ||
} | ||
} | ||
|
||
public void clearTable() throws SQLException { | ||
try (Statement stmt = connection.createStatement();) { | ||
stmt.executeUpdate("DELETE FROM " + tableName); | ||
} | ||
} | ||
|
||
public void validate(String value) throws SQLException { | ||
try (PreparedStatement psInsert = connection.prepareStatement("INSERT INTO " + tableName + " VALUES(?)"); | ||
PreparedStatement psFetch = connection.prepareStatement("SELECT * FROM " + tableName); | ||
Statement stmt = connection.createStatement();) { | ||
clearTable(); | ||
// Used for exact byte comparison. | ||
byte[] valueBytes = value.getBytes(StandardCharsets.UTF_8); | ||
|
||
psInsert.setString(1, value); | ||
psInsert.executeUpdate(); | ||
|
||
// Fetch using Statement. | ||
ResultSet rsStatement = stmt.executeQuery("SELECT * FROM " + tableName); | ||
rsStatement.next(); | ||
// Compare Strings. | ||
assertEquals(value, rsStatement.getString(1)); | ||
// Test UTF8 sequence returned from getBytes(). | ||
assertArrayEquals(valueBytes, rsStatement.getBytes(1)); | ||
|
||
// Fetch using PreparedStatement. | ||
ResultSet rsPreparedStatement = psFetch.executeQuery(); | ||
rsPreparedStatement.next(); | ||
assertEquals(value, rsPreparedStatement.getString(1)); | ||
assertArrayEquals(valueBytes, rsPreparedStatement.getBytes(1)); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use R_unknownUTF8SupportValue here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, we don't localize the logger messages.