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

#83 Add DbProviderFactory with inflicted changes #84

Closed
wants to merge 2 commits into from

Conversation

ikudjoi
Copy link
Contributor

@ikudjoi ikudjoi commented Oct 17, 2018

Add DbProviderFactory implementation SnowflakeDbFactory.
Had to add parameterless constructors to existing classes.

@ikudjoi
Copy link
Contributor Author

ikudjoi commented Oct 29, 2018

I added implementation of SnowflakeDbCommandBuilder, although it seems not to be important for FluentMigrator. We urgently need these changes for this PR in FluentMigrator project.

fluentmigrator/fluentmigrator#944

@howryu
Copy link
Contributor

howryu commented Oct 29, 2018

I will try to merge it today. Thanks for your contribution!

@@ -25,6 +25,14 @@ public class SnowflakeDbCommand : DbCommand

private SFLogger logger = SFLoggerFactory.GetLogger<SnowflakeDbCommand>();

public SnowflakeDbCommand()
Copy link
Contributor

Choose a reason for hiding this comment

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

So the reason why you add parameterless constructor is because .net driver allows application to set Connection object after DbCommand is created, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specially in the added Factory class this is needed. It's late already here, I won't probably answer further questions today.

@howryu
Copy link
Contributor

howryu commented Oct 29, 2018

@ikudjoi Thanks. I am doing some testing right now. It seems to me that I have to add this line to SnowflakeDbFactory.cs to make it work.

       public static readonly SnowflakeDbFactory Instance = new SnowflakeDbFactory();

Otherwise, it will complain

Error Message:
 System.InvalidOperationException : The requested .Net Framework Data Provider's implementation does not have an Instance field of a System.Data.Common.DbProviderFactory derived type.
Stack Trace:
   at System.Data.Common.DbProviderFactories.GetFactory(DataRow providerRow)
   at System.Data.Common.DbProviderFactories.GetFactory(String providerInvariantName)
   at Snowflake.Data.Tests.SFDbFactoryIT.TestSimpleDbFactory() in c:\Snowflake\git\snowflake-connector-net\snowflake-connector-net\Snowflake.Data.Tests\SFDbFactoryIT.cs:line 24

So I have to merge it tomorrow. Sorry for that.

@ikudjoi
Copy link
Contributor Author

ikudjoi commented Oct 30, 2018

Hi, sorry, I didn't write that test you are referring to but that change sounds cool to me. This is how I use the factory in FluentMigrator project
https://github.com/fluentmigrator/fluentmigrator/blob/238061cf4fef4e46cf5a08392dcecf34b960e21f/src/FluentMigrator.Runner.Snowflake/Processors/Snowflake/SnowflakeDbFactory.cs

So only full assembly name of the factory is given. The factory specifies the different classes of the Snowflake connector.

Btw, is there a reason that the command connection is immutable? I just tried to change as little as possible and made it throw an exception if the connection is changed.

@howryu
Copy link
Contributor

howryu commented Oct 30, 2018

I guess command's connection can be changed. However, I am from JDBC. So in JDBC there is no way to change Statement's Connection object. So I just mimic this behavior here.

@howryu
Copy link
Contributor

howryu commented Oct 30, 2018

Also, a quick note. Our current SnowflakeDbAdaptor did not support delete or update or insert. Only select statement is supported. Not sure if this will influence your project.

@howryu
Copy link
Contributor

howryu commented Oct 30, 2018

Merged in #89

@howryu howryu closed this Oct 30, 2018
@ikudjoi
Copy link
Contributor Author

ikudjoi commented Oct 30, 2018

Thanks for accepting the PR! You are going to release new NuGet package version sometime soon, right?
FluentMigrator is mostly about DDL statements, the integration tests were working just fine with my own builds of the connector.

@ikudjoi ikudjoi deleted the factory branch October 31, 2018 10:07
@howryu
Copy link
Contributor

howryu commented Nov 1, 2018

@ikudjoi I will release it today.

@howryu
Copy link
Contributor

howryu commented Nov 1, 2018

FYI, 1.0.5 is out. https://www.nuget.org/packages/Snowflake.Data/

@ikudjoi
Copy link
Contributor Author

ikudjoi commented Nov 2, 2018

Cool, thanks!

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.

2 participants