Skip to content

Conversation

@rscales
Copy link

@rscales rscales commented May 21, 2025

Implementation of SQLGetDiagField.

@alinaliBQ
Copy link

I'm okay with this PR not having test cases as the tests will be dependent on #28. After #28 is merged, we can have a separate PR for SQLGetDiagField test cases

@rscales rscales marked this pull request as ready for review May 22, 2025 17:24
Copy link

@alinaliBQ alinaliBQ left a comment

Choose a reason for hiding this comment

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

LGTM

@rscales rscales merged commit 4108b18 into apache-odbc May 22, 2025
Copy link

@jduo jduo left a comment

Choose a reason for hiding this comment

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

Will SQLGetDiagRec and SQLError be implemented in separate PRs? They are commonly used in applications.

diagnostics = &connection->GetDiagnostics();
break;
}

Copy link

Choose a reason for hiding this comment

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

We can go ahead and add statement and descriptor cases now so we don't forget.

// Retrieve record field data
switch (diagIdentifier) {
case SQL_DIAG_MESSAGE_TEXT: {
const std::string message = diagnostics->GetMessageText(recordIndex);
Copy link

Choose a reason for hiding this comment

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

This can be a const std::string& right? That avoids a copy. Check throughout.

}

case SQL_DIAG_SERVER_NAME: {
const std::string source = diagnostics->GetDataSourceComponent();
Copy link

Choose a reason for hiding this comment

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

This field is supposed to be the DSN if connecting via DSN (same as SQLGetInfo(DATA_SOURCE_NAME).

I do not remember if GetDataSourceComponent() returns the DSN, or if it returns the component name that is used for formatting the error message here: https://learn.microsoft.com/en-us/sql/odbc/reference/develop-app/diagnostic-messages?view=sql-server-ver16 . That's supposed to describe the internal component reporting the error.

return SQL_INVALID_HANDLE;
}

if (!diagInfoPtr) {
Copy link

Choose a reason for hiding this comment

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

It's legal to pass a NULL here if they just want the length. Same as for attributes.

stringLengthPtr, *diagnostics);
}

default:
Copy link

Choose a reason for hiding this comment

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

It might be a problem to throw an error for unrecognized, but standard diag types. (Like it breaks apps). Perhaps we just return dummy values for any valid, but not supported fields. For non-standard fields we can report error.

The spec says we should only return SQL_ERROR for when "The DiagIdentifier argument was not one of the valid values."

@alinaliBQ
Copy link

Will SQLGetDiagRec and SQLError be implemented in separate PRs? They are commonly used in applications.

@jduo SQLError would be in milestone 3

@alinaliBQ alinaliBQ deleted the sql-get-diag-field branch June 17, 2025 22:10
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.

4 participants