Add support for TDSType.GUID (#1582)

[vxel]

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]

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

[lilgreenbird]

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

[lilgreenbird]

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

[vxel]

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]

I just pushed some specific tests

[tkyc]

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

[tkyc]

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]

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]

typo?? same in line 81?

[vxel]

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

[lilgreenbird]

Suggested change

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

[vxel]

Thanks. I applied your suggestion in the latest commit.

[codecov]

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.

[Jeffery-Wasty]

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]

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]

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]

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.