Skip to content
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

Only check inexact match when no exact match is found. #668

Merged
merged 1 commit into from Aug 7, 2020

Conversation

yyjdelete
Copy link
Contributor

Fix dotnet/runtime#39620

The same as dotnet/runtime#39961

Avoid the exact match result be overwrite by later inexact ones.
This may not happen due to limited callsites, just make the code more clear.

@cheenamalhotra
Copy link
Member

Is there a test case to verify the fix?

@yyjdelete
Copy link
Contributor Author

Infact, I don't know how to create an test for this internal api. And maybe it's not a problem due to it's internal and there is no dupe tables with different case actually used by public apis.

https://github.com/dotnet/sqlclient/search?q=filename%3AMetaData.xml&unscoped_q=filename%3AMetaData.xml

@yyjdelete yyjdelete closed this Jul 28, 2020
@roji
Copy link
Member

roji commented Jul 28, 2020

@yyjdelete it may be worth looking into this a little further and finding an actual user impact - since the current implementation does seem buggy.

@Wraith2
Copy link
Contributor

Wraith2 commented Jul 28, 2020

If it isn't testable but is clearly a fix I'm fine with merging it but testing does always need to be considered een though writing tests sucks.

@David-Engel
Copy link
Contributor

This bug would only surface if the embedded resource, Microsoft.Data.SqlClient.SqlMetaData.xml, contained multiple MetaDataCollections with a CollectionName differing only by case and the desired entry (case sensitive match) appeared before the undesired entry (case insensitive match). Testing the bug would be awkward. I'm fine with simply fixing the bug. Bonus points if a couple lines are added to TestGetSchema to test conn.GetSchema("Databases") in addition to conn.GetSchema("DATABASES") to cover both blocks of the if statement in question.

@yyjdelete
Copy link
Contributor Author

This bug would only surface if the embedded resource, Microsoft.Data.SqlClient.SqlMetaData.xml, contained multiple MetaDataCollections with a CollectionName differing only by case and the desired entry (case sensitive match) appeared before the undesired entry (case insensitive match). Testing the bug would be awkward. I'm fine with simply fixing the bug. Bonus points if a couple lines are added to TestGetSchema to test conn.GetSchema("Databases") in addition to conn.GetSchema("DATABASES") to cover both blocks of the if statement in question.

@David-Engel
There is already another test with conn.GetSchema("Databases")(case sensitive match) in ConnectionSchemaTest.

[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))]
public static void GetDatabasesFromSchema()
{
VerifySchemaTable(SqlClientMetaDataCollectionNames.Databases, new string[] { "database_name", "dbid", "create_date" });
}

@cheenamalhotra cheenamalhotra added this to In progress in SqlClient v2.1 via automation Aug 7, 2020
@cheenamalhotra cheenamalhotra added this to the 2.1.0-preview1 milestone Aug 7, 2020
@cheenamalhotra cheenamalhotra merged commit dbbcde8 into dotnet:master Aug 7, 2020
SqlClient v2.1 automation moved this from In progress to Done Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Strange logic in System.Data.ProviderBase.DbMetaDataFactory.FindMetaDataCollectionRow()
5 participants