Skip to content

Read GUIDs from out parameters #1546

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 9, 2025
Merged

Read GUIDs from out parameters #1546

merged 3 commits into from
Mar 9, 2025

Conversation

@bgrainger
Copy link
Member

Fixes #1528

@bgrainger
Read GUIDs from out parameters. Fixes #1528
944ced6 
Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
@bgrainger bgrainger requested a review from Copilot March 8, 2025 20:45
Copilot AI reviewed Mar 8, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR aims to fix issue #1528 by ensuring that GUID values are correctly read from OUT parameters. Key changes include adding a guidFormat parameter to several methods and constructors, updating the SQL payload generation for GUID columns, and revising tests to validate these changes.

Reviewed Changes

File Description
tests/IntegrationTests/StoredProcedureTests.cs Added StoredProcedureReturnsGuid tests with guidFormat parameter
src/MySqlConnector/Core/SingleCommandPayloadCreator.cs Adjusted out parameter handling for GUIDs with explicit casts
src/MySqlConnector/Core/ColumnTypeMetadata.cs Extended lookup keys to include the guidFormat parameter
src/MySqlConnector/Core/TypeMapper.cs Registered new GUID types with explicit guidFormat values
src/MySqlConnector/Core/CachedProcedure.cs Propagated guidFormat through parameter parsing and cached parameter creation
src/MySqlConnector/ColumnReaders/ColumnReader.cs Updated case handling for VarString/VarChar redirection
tests/MySqlConnector.Tests/CachedProcedureTests.cs Updated tests to include guidFormat for parameter parsing
src/MySqlConnector/Core/CachedParameter.cs Modified the constructor to accept and propagate guidFormat

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

tests/IntegrationTests/StoredProcedureTests.cs Outdated
{
command.CommandType = CommandType.StoredProcedure;
var param = new MySqlParameter("out_name", null) { Direction = ParameterDirection.Output };
if (mySqlDbType.HasValue && DateTime.UtcNow.Year == 2024)
Copy link

Copilot AI Mar 8, 2025

Choose a reason for hiding this comment

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

Using a hard-coded year (2024) in the condition may cause tests to behave unexpectedly in future years. Consider replacing the DateTime.UtcNow.Year check with a configurable condition or removing it entirely.

Suggested change
if (mySqlDbType.HasValue && DateTime.UtcNow.Year == 2024)
var targetYear = int.Parse(Environment.GetEnvironmentVariable("TARGET_YEAR") ?? "2024");
if (mySqlDbType.HasValue && DateTime.UtcNow.Year == targetYear)

Copilot uses AI. Check for mistakes.
bgrainger added 2 commits March 8, 2025 12:58
@bgrainger
Fix tests to not have hardcoded check.
68170e3 
Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
@bgrainger
Merge master into guid-out-param.
50ab18a 
Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
@bgrainger bgrainger requested a review from Copilot March 9, 2025 20:40
Copilot AI reviewed Mar 9, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR addresses issue #1528 by adding support for reading GUIDs from OUT parameters. Key changes include new test cases for stored procedures using various GUID formats, updates to type mapping and lookup keys incorporating a new MySqlGuidFormat parameter, and corresponding modifications in parameter parsing and column reading logic.

Reviewed Changes

File Description
tests/IntegrationTests/StoredProcedureTests.cs New test cases for stored procedure GUID output handling
src/MySqlConnector/Core/SingleCommandPayloadCreator.cs Added special handling to cast GUID out parameters based on size and format
src/MySqlConnector/Core/ColumnTypeMetadata.cs Updated lookup key generation to include a GUID format component
src/MySqlConnector/Core/TypeMapper.cs Adjusted metadata addition and lookup to incorporate the new GUID format
src/MySqlConnector/Core/CachedProcedure.cs Modified parameter parsing and object creation to pass along GUID format
src/MySqlConnector/ColumnReaders/ColumnReader.cs Updated reader dispatch to use the correct case label for GUID types
tests/MySqlConnector.Tests/CachedProcedureTests.cs Updated test cases to include the GUID format parameter in inline data
src/MySqlConnector/Core/CachedParameter.cs Modified constructor to accept and pass the GUID format

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/MySqlConnector/Core/TypeMapper.cs:200

  • Ensure that the fallback TryGetValue call properly assigns columnTypeMetadata when a match is found. Consider checking the result of TryGetValue or using an 'if' block to validate the lookup for clarity.
if (length != 0) m_columnTypeMetadataLookup.TryGetValue(ColumnTypeMetadata.CreateLookupKey(columnTypeName, unsigned, 0, MySqlGuidFormat.Default), out columnTypeMetadata);

@bgrainger bgrainger merged commit 97b776b into master Mar 9, 2025
24 checks passed
@bgrainger bgrainger deleted the guid-out-param branch March 9, 2025 20:42
@bgrainger bgrainger added this to the 2.5.0 milestone Mar 9, 2025
@gnjack
Copy link

gnjack commented Apr 15, 2025

@bgrainger I think this has caused a breaking change for us.

In version 2.4.0, a VARCHAR(36) column is read as a string when using the default GuidFormat - Char36.
In a branch build of master, the same VARCHAR(36) column is read as a GUID.

@bgrainger
Copy link
Member Author

bgrainger commented Apr 15, 2025 •
edited
Loading

Yes; that is an intentional change. With the Char36 GuidFormat, all CHAR(36) columns will be read as Guid, whether it's via MySqlDataReader.GetValue (long-time behavior) or a OUT parameter from a stored procedure (new behavior).

To avoid that, make the parameter/column a different width, or use GuidFormat=None in the connection string.

Edit: I misread your post. This should not be happening for VARCHAR(36).

@gnjack
Copy link

gnjack commented Apr 15, 2025

Correct, a CHAR(36) column has always been read as Guid by default - it's the behaviour for VARCHAR(36) that seems to have unintentionally changed while fixing the behaviour for stored procedures.

@bgrainger bgrainger mentioned this pull request Apr 17, 2025
Closed
bgrainger added a commit that referenced this pull request Apr 17, 2025
@bgrainger
Only treat CHAR(n) columns as Guids. Fixes #1565
3dca6b1 
This reverts a change in #1546 that was introduced to fix #1528. However, none of the integration tests fail locally when reverting this change, so it's not clear why it was necessary.

Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
bgrainger added a commit that referenced this pull request Apr 17, 2025
@bgrainger
Don't treat VARCHAR columns as GUIDs. Fixes #1565 (#1566)
b81c038 
* Only treat CHAR(n) columns as Guids.
* Add test to reproduce bug.

This reverts a change in #1546 that was introduced to fix #1528. However, none of the integration tests fail locally when reverting this change, so it's not clear why it was necessary.
@bgrainger
Copy link
Member Author

@gnjack Fixed: b81c038; thanks for bringing this to my attention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Milestone

2.5.0

Development

Successfully merging this pull request may close these issues.

Mysql 5.6, output MysqlParameter, binary(16) -> Guid conversion issue

3 participants

@bgrainger @gnjack