-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Use StrategyBasedComWrappers for all IError* interfaces in OleDb #118513
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
Conversation
Tagging subscribers to this area: @roji, @SamMonoRT |
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.
Pull Request Overview
This PR refactors OleDb COM interop to use the new StrategyBasedComWrappers
approach instead of manual COM wrapper implementation. The main purpose is to fix issue #108035 by ensuring that casts to IError* interfaces succeed when expected.
Key changes include:
- Converts COM interfaces from
ComImport
toGeneratedComInterface
pattern - Replaces custom
OleDbComWrappers
implementation with strategy-based approach - Simplifies error info object handling and release mechanisms
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
UnsafeNativeMethods.cs | Converts IErrorInfo and IErrorRecords interfaces to use GeneratedComInterface, adds ISQLErrorInfo interface |
UnsafeNativeMethods.COMWrappers.cs | Simplifies GetErrorInfo method to use direct interface marshalling, updates ReleaseErrorInfoObject |
System.Data.OleDb.csproj | Removes OleDbComWrappers.cs from compilation and reformats file includes |
OleDbComWrappers.cs | Completely removes the custom ComWrappers implementation file |
src/libraries/System.Data.OleDb/src/UnsafeNativeMethods.COMWrappers.cs
Outdated
Show resolved
Hide resolved
Looks like the CLS compliance fixes I tried to do didn't work. I'll do a different fix (and link to the Roslyn issue). |
This reverts commit 6aaee7e.
Is there a reasonable unit test we could add that would have caught this? |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Adding a test for this one is exceedingly difficult as it would require us to string-compare against an error string provided by Windows. Also, we would only be interested in running the test on a Windows machine with non-English locale, and as far as I can tell, we dropped that helix queue a while back (it's not in |
If this failure manifests in another way, we could write a unit test that hits some database to generate the right errors, but based on just the issue that led to the fix, writing a test is more effort than it's worth (and would be a manual test anyway). |
Ah, missed that the machine itself needed to be non-English and we didn't know any part of the string (thought it was only from the language setting in the connection string and thought the DBNETLIB prefix was ours). Agreed that it is not worth it. |
Fixes #108035 by making the cast at
runtime/src/libraries/System.Data.OleDb/src/OleDbErrorCollection.cs
Line 18 in d61ca90