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

Add fluent configuration with IDbConnection #499

Merged
merged 6 commits into from Feb 19, 2019

Conversation

Projects
None yet
2 participants
@asherber
Copy link
Collaborator

asherber commented Dec 23, 2018

This adds the ability to use the fluent config syntax with an existing connection, either using the default provider for the connection or providing a custom one:

var config = DatabaseConfiguration.Build()
    .UsingConnection(myDbConnection);

var config  = DatabaseConfiguration.Build()
    .UsingConnection(myDbConnection)
    .UsingProvider<MyCustomProvider>();

As part of this, I refactored the section of the Database constructor that takes a config object -- specifically, the section that deals with connection strings, connection string names, and providers. The nesting of the success and failure callbacks in TryGetSetting() was getting out of control and making it hard to follow the logic flow. So I rewrote this to get the needed settings up front and then just rely on good old if..then blocks for flow control.

There's one thing I noticed but did not change. In the framework section of the code, if we're initializing from a connection string entry, we fallback to the SQL server provider if the connection string is missing the provider section. But in the netstandard section, if there's no provider and no provider name supplied in the config, we just throw an exception instead of falling back. Is this intentional? If not, I can add the fallback to the PR.

@pleb

This comment has been minimized.

Copy link
Member

pleb commented Dec 24, 2018

There's one thing I noticed but did not change. In the framework section of the code, if we're initializing from a connection string entry, we fallback to the SQL server provider if the connection string is missing the provider section. But in the netstandard section, if there's no provider and no provider name supplied in the config, we just throw an exception instead of falling back. Is this intentional? If not, I can add the fallback to the PR.

Likely an oversight. We should have the same default IMO.

@asherber

This comment has been minimized.

Copy link
Collaborator Author

asherber commented Dec 24, 2018

Looking further at the various constructors, I found the following behavior for resolving providers.

Non-fluent

  1. Database()
    • Uses default if entry.ProviderName is null or empty
    • Throws if provider can't be resolved
  2. Database(connStringName)
    • Uses default if entry.ProviderName is null or empty
    • Throws if provider can't be resolved
  3. Database(connection)
    • Throws if provider can't be resolved
  4. Database(connectionString, providerName)
    • Uses default if providerName is empty (Resolve() will throw an NRE if it's null)
    • Uses default if provider can't be resolved
    • Should throw if provider can't be resolved
    • Should have null/empty check for providerName
  5. Database(connectionString, factory)
    • Throws if provider can't be resolved

Fluent

  1. Framework: UsingConnectionString() has no option to pass in provider name (needs to use UsingProvider<T>())
  2. Framework: UsingConnectionStringName()
    • Uses default if entry.ProviderName is null or empty
    • Throws if provider can't be resolved
  3. Standard: UsingConnectionString() + UsingProviderName()
    • Throws if provider name is null or empty (checked in config setter)
    • Throws if provider can't be resolved

The one that sticks out to me now is Non-fluent number 4. If we're asking the user to pass in an explicit name, we should make sure he does so and throw an exception if providerName is null or empty. This is also the only constructor that gives the default provider if the name can't be resolved, which I think should be changed.

Overall, this means that the ruleset is that we're lenient if we're delegating to a connection string entry that the user may not have control over, but stricter if we're asking the user to give us things directly. I think this makes good sense

With that in mind, my earlier comment about Fluent number 3 was wrong. I didn't see that the config setter disallows null/empty values for the provider name up front, which is good. The fluent constructor then throws if no provider name was set at all and throws if the given provider name can't be resolved, both of which are correct.

Please let me know what you think, and I'll amend the PR accordingly.

Edit 2/18/19: Clarified Fluent 1 and 3

@asherber

This comment has been minimized.

Copy link
Collaborator Author

asherber commented Dec 24, 2018

In case you decide you don't want any changes made, let me know that as well so that I can go over the PR once more before you merge, to make sure that my refactoring does maintain the status quo.

@pleb

This comment has been minimized.

Copy link
Member

pleb commented Feb 12, 2019

Overall, this means that the ruleset is that we're lenient if we're delegating to a connection string entry that the user may not have control over, but stricter if we're asking the user to give us things directly. I think this makes good sense

I think you nailed it.

With that said, it should probably throw.

Happy to do another release when you say this PR is ready

@asherber

This comment has been minimized.

Copy link
Collaborator Author

asherber commented Feb 18, 2019

Okay, I'm satisfied with this -- I have changed non-fluent constructor number 4 as detailed above, and I've walked through all the code to make sure my refactorings didn't break anything.

Note that this could be considered a breaking change, since currently if someone uses that constructor and passes in an empty or invalid string, they'll get the default provider -- and now they'll get an exception.

There's one other thing I noticed. For the fluent syntax, UsingProviderName() exists only in .NET Standard. I think there's a case to be made that this should be available for Framework as well -- UsingProvider<T>() is better if you're setting this in code, but maybe the connection string and provider name are being read from some other config source.

Also, note that in .NET Standard you could apply both UsingProviderName() and UsingProvider<T>() to an IDatabaseBuildConfiguration. The latter would take precedence, although this isn't called out anywhere. I think I would suggest adding UsingProviderName() and then tweaking the XML docs to indicate that UsingProvider<T>() takes precedence.

Let me know if you'd like me to deal with this. Otherwise, we're good to go.

@pleb

This comment has been minimized.

Copy link
Member

pleb commented Feb 18, 2019

Note that this could be considered a breaking change, since currently if someone uses that constructor and passes in an empty or invalid string, they'll get the default provider -- and now they'll get an exception.

I think this should be ok. Yes, it's a breaking change, but it's not an obvious redefinition of use. I feel it's more about ensuring the correct usage is adhered to.

Let me know if you'd like me to deal with this

Yep go for it. I'm also happy to take this PR as-is if you wanted to sort that out at a later time

@asherber

This comment has been minimized.

Copy link
Collaborator Author

asherber commented Feb 18, 2019

No worries, I'll have it finished tonight.

@asherber

This comment has been minimized.

Copy link
Collaborator Author

asherber commented Feb 19, 2019

I think this is finally good to go!

@pleb pleb merged commit 69edf7d into CollaboratingPlatypus:development Feb 19, 2019

1 check failed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.