Add support for TDSType.GUID (#1582) by vxel · Pull Request #2324 · microsoft/mssql-jdbc

vxel · 2024-02-08T20:57:03Z

This PR intends to provide support for sending UUID parameters as TDSType.GUID (0x24) on the wire and (hopefully) implements feature request #1582.
It currently requires the Java parameter be given to the driver as String with JDBCType.GUID, i.e. :

st.setObject(index, myUUID.toString(), microsoft.sql.Types.GUID);

vxel · 2024-02-09T07:08:55Z

@microsoft-github-policy-service agree company="Oniryx"

lilgreenbird · 2024-02-09T07:56:39Z

@vxel thank you for submitting this, the team will review this when time permits

lilgreenbird · 2024-02-09T08:00:18Z

took a quick glance at this, please add tests covering the added feature, thanks!

vxel · 2024-02-09T08:18:38Z

took a quick glance at this, please add tests covering the added feature, thanks!

The code is already covered by the BatchExecutionWithBulkCopyTest.testNullGuid() and StatementTest.testJdbc41CallableStatementMethods() methods, so I wonder which kind of specific test you request.
I guess I can make tests similar to those in BigIntegerTest ?

vxel · 2024-02-09T11:06:00Z

I just pushed some specific tests

tkyc · 2024-02-12T18:16:06Z

@vxel Thanks, I'll need to some testing on our private pipelines.

tkyc · 2024-02-14T00:21:27Z

Testing looks good from internal runs. A question more for the team, is there anywhere else that test can be placed rather than in a new test file?

vxel · 2024-02-14T09:38:11Z

Considering the test classes from the com.microsoft.sqlserver.jdbc.datatypes package, it seems the practice is to use individual test files for each "special" datatype, e.g. BigIntegerTest (for java.math.BigInteger), DateAndTimeTypeTest (for temporal datatypes), SQLServerSpatialDatatypeTest (for Geometry / Geography classes). Hence I guess adding a new test file for the java.util.UUID datatype is also suitable.
Another test file I considered was com.microsoft.sqlserver.jdbc.preparedStatement.SetObjectTest but the class is annotated with @Tag(Constants.xAzureSQLDW) which I guess would unnecessarily retrict the scope for my GUID tests.
I do not see alternatives but you certainly know the driver better than me.
Please tell me if you find a better place and I will move the test.

lilgreenbird · 2024-02-28T00:22:02Z

+                    assertEquals(uuid, UUID.fromString(rs.getUniqueIdentifier(1)));
+                }
+
+                // Test NULL GUFID


typo?? same in line 81?

Yes this is indeed a typo, copy-pasted to line 81... Thanks. This is fixed in my latest commit.

lilgreenbird · 2024-02-28T00:26:43Z

+                // Test Illegal GUFID
+                try {
+                    pstmt.setObject(1, "garbage", Types.GUID);
+                    fail("Must throw an IllegalArgumentException for this illegal UUID");


Suggested change

fail("Must throw an IllegalArgumentException for this illegal UUID");

fail(TestResource.getResource("R_expectedFailPassed"));

Thanks. I applied your suggestion in the latest commit.

codecov · 2024-03-01T15:11:55Z

Codecov Report

Attention: Patch coverage is 71.64179% with 19 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (main@9b5036d). Click here to learn what that means.

❗ Current head 10bbb16 differs from pull request most recent head 40b9ed9. Consider uploading reports for the commit 40b9ed9 to get more accurate results

Files	Patch %	Lines
.../microsoft/sqlserver/jdbc/SQLServerConnection.java	59.37%	5 Missing and 8 partials ⚠️
...m/microsoft/sqlserver/jdbc/SQLServerException.java	50.00%	0 Missing and 3 partials ⚠️
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java	77.77%	1 Missing and 1 partial ⚠️
...oft/sqlserver/jdbc/SQLServerPreparedStatement.java	87.50%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2324   +/-   ##
=======================================
  Coverage        ?   50.12%           
  Complexity      ?     3788           
=======================================
  Files           ?      143           
  Lines           ?    33146           
  Branches        ?     5630           
=======================================
  Hits            ?    16614           
  Misses          ?    14147           
  Partials        ?     2385

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This reverts commit 9a8849b.

Jeffery-Wasty · 2024-03-25T19:38:06Z

Hi @vxel,

This PR requires additional work to integrate fully with existing code. Unfortunately, this fact wasn't caught prior to merging. We've therefore reverted the PR, and will look at merging it again once we make the necessary changes.

vxel · 2024-03-26T13:44:19Z

Hi @Jeffery-Wasty
No problem, I'm sorry I missed this.
I wonder however why the CI did not catch this when I submitted the PR.
I will take a look at the issue.

vxel · 2024-03-26T16:18:36Z

Ok I see now that those tests require external setup and are disabled by default.
Unfortunately I'm unable to setup a suitable test environment to reproduce the problem. Hence it is hard for me to see how to fix it.

vxel · 2024-03-27T16:14:40Z

Ok I finally got a suitable environment and managed to reproduce the problem.
I think I can fix it. I'm preparing a new PR.

Cédric de Launois added 2 commits February 8, 2024 21:46

Added support for TDSType.GUID (microsoft#1582)

1e89477

Fix UUID serialisation

41ec399

lilgreenbird requested review from Jeffery-Wasty, barryw-mssql and tkyc February 9, 2024 07:55

vxel changed the title ~~Added support for TDSType.GUID (#1582)~~ Add support for TDSType.GUID (#1582) Feb 9, 2024

Add unit tests

e629bd5

lilgreenbird linked an issue Feb 14, 2024 that may be closed by this pull request

[FEATURE REQUEST] Allow sending UUID in binary to database #1582

Closed

lilgreenbird reviewed Feb 28, 2024

View reviewed changes

lilgreenbird added this to the 12.7.0 milestone Feb 28, 2024

lilgreenbird added the Enhancement An enhancement to the driver. Lower priority than bugs. label Feb 28, 2024

Fix typo, handle PR feedback

40b9ed9

tkyc approved these changes Mar 19, 2024

View reviewed changes

Jeffery-Wasty approved these changes Mar 19, 2024

View reviewed changes

lilgreenbird approved these changes Mar 20, 2024

View reviewed changes

lilgreenbird merged commit 9a8849b into microsoft:main Mar 20, 2024

vxel deleted the feature/1582 branch March 21, 2024 17:10

tkyc added a commit that referenced this pull request Mar 25, 2024

Revert "Add support for TDSType.GUID (#1582) (#2324)"

fe7f3ff

This reverts commit 9a8849b.

tkyc mentioned this pull request Mar 25, 2024

Revert "Add support for TDSType.GUID (#1582)" #2365

Merged

lilgreenbird pushed a commit that referenced this pull request Mar 25, 2024

Revert "Add support for TDSType.GUID (#1582) (#2324)" (#2365)

8e92248

This reverts commit 9a8849b.

vxel mentioned this pull request Mar 27, 2024

Add support for TDSType.GUID (fix) (#1582) #2370

Merged

lilgreenbird mentioned this pull request Mar 27, 2024

[FEATURE REQUEST] Allow sending UUID in binary to database #1582

Closed

This was referenced Jun 18, 2025

Add support for JavaType UUID #2689

Closed

Add support for JavaType.UUID #2692

Merged

	fail("Must throw an IllegalArgumentException for this illegal UUID");
	fail(TestResource.getResource("R_expectedFailPassed"));

Conversation

vxel commented Feb 8, 2024

Uh oh!

vxel commented Feb 9, 2024

Uh oh!

lilgreenbird commented Feb 9, 2024

Uh oh!

lilgreenbird commented Feb 9, 2024

Uh oh!

vxel commented Feb 9, 2024

Uh oh!

vxel commented Feb 9, 2024

Uh oh!

tkyc commented Feb 12, 2024

Uh oh!

tkyc commented Feb 14, 2024

Uh oh!

vxel commented Feb 14, 2024

Uh oh!

lilgreenbird Feb 28, 2024

Choose a reason for hiding this comment

Uh oh!

vxel Mar 1, 2024

Choose a reason for hiding this comment

Uh oh!

lilgreenbird Feb 28, 2024

Choose a reason for hiding this comment

Uh oh!

vxel Mar 1, 2024

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Jeffery-Wasty commented Mar 25, 2024

Uh oh!

vxel commented Mar 26, 2024

Uh oh!

vxel commented Mar 26, 2024

Uh oh!

vxel commented Mar 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Mar 1, 2024 •

edited

Loading