Skip to content
This repository has been archived by the owner on Nov 1, 2018. It is now read-only.

Initial implementation for DBConnection.GetSchema(String) #443 #449

Closed
wants to merge 9 commits into from

Conversation

Thorium
Copy link

@Thorium Thorium commented Oct 18, 2017

No description provided.

@dnfclas
Copy link

dnfclas commented Oct 18, 2017

@Thorium,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@Thorium
Copy link
Author

Thorium commented Oct 18, 2017

The Appveyor test fails as I was naive and used the test database file readonly.db that was used by the other tests. However I tested locally with larger database and it works. How should the databases be used in the unit tests?

@bricelam bricelam self-assigned this Oct 18, 2017
@bricelam
Copy link
Contributor

How should the databases be used in the unit tests?

Use :memory: unless the test requires otherwise.

[Fact]
public void GetSchema_has_collections()
{
var connectionString = "Data Source=readonly.db";
Copy link
Contributor

Choose a reason for hiding this comment

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

try datasource as :memory: ?

Copy link
Contributor

@bricelam bricelam left a comment

Choose a reason for hiding this comment

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

Let's hammer out the design of these collections in #443 before iterating on this PR.

var dt = new DataTable(collectionName);
switch (collectionName)
{
case "DataTypes":
Copy link
Contributor

Choose a reason for hiding this comment

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

Use DbMetaDataCollectionNames.DataTypes

dt.Columns.AddRange(new[] {
new DataColumn("DataType", typeof(string)),
new DataColumn("TypeName", typeof(string)),
new DataColumn("ProviderDbType", typeof(int))
Copy link
Contributor

Choose a reason for hiding this comment

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

Use DbMetaDataColumnNames.DataType, etc.

new DataColumn("CONSTRAINT_NAME")
});
foreach(var tablename in tables){
var relationQuery = "pragma foreign_key_list(" + tablename + ")";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can combine this and the previous query using this syntax:

SELECT t.name,
       fk.table,
       fk.from,
       fk.to
FROM sqlite_master t,
     pragma_foreign_key_list(t.name) fk
WHERE t.type = 'table'

Copy link
Author

Choose a reason for hiding this comment

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

Nice idea, however DB Browser for SQLite doesn't support that, so I don't know how widely that is supported.

@bricelam
Copy link
Contributor

We should probably implement the MetaDataCollections collection to show which collections we currently support.

For the collections we do support, we should probably include all the "usual" columns (even if we don't populate them with data).

@bricelam
Copy link
Contributor

Let's add a few more tests to make sure the data returned is correct.

@Thorium
Copy link
Author

Thorium commented Oct 22, 2017

@bricelam MetaDataCollections and more testing have been added.

Copy link
Contributor

@bricelam bricelam left a comment

Choose a reason for hiding this comment

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

Looking good

/// <returns>A DataTable that contains schema information.</returns>
public override System.Data.DataTable GetSchema(string collectionName)
{
return GetSchema(collectionName, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we should also add:

public override DataTable GetSchema()
    => GetSchema(DbMetaDataCollectionNames.MetaDataCollections, null);

Copy link
Author

Choose a reason for hiding this comment

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

done

dt.Rows.Add(new object[] { "System.DateTime","date",6 });
dt.Rows.Add(new object[] { "System.DateTime","time",6 });
dt.Rows.Add(new object[] { "System.Guid","uniqueidentifier",4 });
dt.Rows.Add(new object[] { "System.Guid","guid",4 });
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to refine this information before merging.

Copy link
Author

@Thorium Thorium Oct 24, 2017

Choose a reason for hiding this comment

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

The information here describes what types of .NET can be mapped to types that will be understood by the database driver, and the corresponding System.Data.Common DbTypes. This information is taken from the System.Data.SQLite.dll library.

I think this is more of a design decision. Do you want to continue from here?
I'm already happy with the current implementation but feel free to change what ever you need.

@bricelam
Copy link
Contributor

Closing this per our discussion. Querying directly for this metadata (and hard-coding a list of supported types) is the correct way to handle this for the time being.

@bricelam bricelam closed this Oct 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants