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

Allow optionally not requiring explicit connection strings #234

Closed
isaacabraham opened this issue Aug 9, 2016 · 27 comments
Closed

Allow optionally not requiring explicit connection strings #234

isaacabraham opened this issue Aug 9, 2016 · 27 comments

Comments

@isaacabraham
Copy link

Description

In the latest release of SQLClient, behaviour changed so that if you supplied a connection string as the literal (rather than the named config value), all the provided types would request an explicit connection string.

Whilst I understand why this has been done, for us it's a real problem as we use named connection strings but occasional want to test with a real one. The TP should allow you to revert back to the old behaviour whereby it won't change the signatures of the generated types simply based on the contents of the literal.

@smoothdeveloper
Copy link
Collaborator

The most useful and only needed constructor in my opinion would be (commandFactory: unit -> SqlCommand), it contains all which is needed (connection, eventual transaction and desired timeout) and refrains one to cater connection string literals around.

@dmitry-a-morozov
Copy link
Member

@isaacabraham Sorry for troubles. I knew it's going to be painful. :(
Can you elaborate? What do you mean exactly by "... occasional want to test with a real one ..."
You still can use named connection string at design time but provide literal conn str that read from some sort of configuration at run-time.

@isaacabraham
Copy link
Author

@dmitry-a-morozov Hi! In our project code, the TP connection string is the config style one (rather than explicit connection). What we're currently doing is compiling our F# projects, and then referencing the output DLL in an F# script (rather than manually referencing all the .fs files).

The problem is that once you do this, the TP looks in fsi.exe.config to resolve the connection string, rather than the config file of the project. So when (on occasion) we need to call our code from a script (testing, exploratory development etc.), we used to resort to manually changing the connection string from a config one to an explicit connection string. Now, this is no longer applicable because all our code stops compiling as soon as we change the connection string format.

I'm open to suggestions on a "better way" to get to our code through scripts as well though!

But at the same time - changing the structure of a string literal like this and have it change the provided type signatures is, whilst powerful, a little hidden - I spent a few hours with a colleague of mine trying to figure out why our code was behaving differently!

@dmitry-a-morozov
Copy link
Member

dmitry-a-morozov commented Aug 10, 2016

I apologize again that I caused troubles. Something at my defense

  1. The change was done at request of Jet.com guys. They said it is the biggest source of errors related to the type provider when devs forgot to provide run-time override. But they always use connection string literals at design time.

  2. I mentioned breaking change in release notes

  3. I tried to improve scripting support for connection string management Can't get connection string from app.config through .fsx file. #185
    but of course it only works when the types instantiated in *.fsx file not in pre-compiled dll

  4. I open F# user voice item that can slightly improve scripting store but won't solve your problem anyway.
    https://fslang.uservoice.com/forums/245727-f-language/suggestions/15462291-have-typeproviderconfig-ishostedexecution-true-f

  5. For your particular use case I suggest to have name-based config at design time and always provide connection string at run-time within scripts. That should work.

  6. If you have some suggestion on how I can improve the situation please provide some pseudo-code in this thread.

@isaacabraham
Copy link
Author

I understand - you don't have to apologise! And yes, it was documented :-) And yes, appreciate your work on the other side - fsx files etc.

Would it be possible to have an argument that's passed in to select the behaviour of the TP? I don't mind if the default is the "new" way - but at least giving us the option of reverting to the old behaviour would be really great.

@dmitry-a-morozov
Copy link
Member

Let's tackle this from different perspective.
At run-time it can be detected that code runs under fsi.exe.
Based on that a command can locate config file using non-standard logic.
There are 2 reasonable options:

  1. Try to load config file from current folder. Keep in mind this will require to execute
System.Environment.CurrentDirectory <- __SOURCE_DIRECTORY__  

at beginning of each script
2) Reuse connection string captured from config file at design time. That's what I did in to address #185
3) Try 1. if file not present fallback to 2.

Let me know what you thoughts.

@isaacabraham
Copy link
Author

isaacabraham commented Aug 11, 2016

Either of those solutions would work for me - I think I prefer the second as it's probably what one would expect to happen anyway i.e. re-use the captured connection string.

Can you think of any situations where that wouldn't be the expected behaviour?

The first suggestion is probably more configurable - it allows you to redirect to another config file at script-time - but it's not entirely clear to me what the best working process / practice for that would be. Should the config file live in the script folder? Should you set the CD to the location of a config file?

@dmitry-a-morozov
Copy link
Member

I can imagine situation when assembly was compiled somewhere else like CI server and you pull it down from let's say local nuget feed. It might have unexpected connection string value. But this is mostly speculation.
I would certainly prefer to start with 2) because it's most straightforward.

@isaacabraham
Copy link
Author

Works for me :)

@dmitry-a-morozov
Copy link
Member

Hi,
Please try
https://www.nuget.org/packages/FSharp.Data.SqlClient/1.8.3-beta
It includes fixes for #233 and #234.
Keep in mind breaking change - #232

//cc @smoothdeveloper Please help to test this release.
It includes #224 for you.

@isaacabraham
Copy link
Author

All works - great.

I did notice that we seem to now need to reference System.Runtime.Caching though?

@dmitry-a-morozov
Copy link
Member

Good.
yes, it's sub-optimal that a reference to System.Runtime.Caching is required.
I'll fix that.

@isaacabraham
Copy link
Author

isaacabraham commented Aug 20, 2016

Seems I may have been wrong - I don't think that this is quite working. I'm getting: -

System.Collections.Generic.KeyNotFoundException: Cannot find name ... in <connectionStrings> section of config file.

when I execute a script that references a DLL with the TP (via #r). Not sure why. Here's the line that goes pop: -

https://github.com/fsprojects/FSharp.Data.SqlClient/blob/master/src/SqlClient/DataTable.fs#L51

It looks like the first .Value passes, but the second fails.

@isaacabraham
Copy link
Author

It might also be useful to see which file it's trying to reference.

@dmitry-a-morozov
Copy link
Member

@isaacabraham I deployed 1.8.3-beta2.
https://www.nuget.org/packages/FSharp.Data.SqlClient/1.8.3-beta2
It doesn't require ref to System.Runtime.Caching

Speaking of #234 (comment)
I cannot replicate this.
So you have named design time connection string defined for SqlProgrammabilityProvider but at runtime inside FSI it still is looking for *.config file, right?

@isaacabraham
Copy link
Author

I'll upgrade to beta2 - maybe that was just in the first beta.
Regarding the other issue - it's still looking in the config file - I'm pretty sure it's reading from the FSI AnyCPU executable config file, as I added the connection string in there and the problem went away.

@happypuddingday
Copy link

I see the same issue in 1.8.3-beta2. The app.config connection string value is not retained for use in FSI. I get a 'KeyNotFoundException'. Adding the connection string to the' FsiAnyCPU.exe.config' solves the problem, so it is definitely still looking there.

@dmitry-a-morozov
Copy link
Member

Fixed.
I forgot to account for process name diff between 32 and 64 bits fsi process.
Try 1.8.3-beta3.

@dmitry-a-morozov
Copy link
Member

@isaacabraham
It's unfortunate but after #195 was fixed
a way connection information passed into Update and BulkCopy methods is inconsistent with rest of the library.
Because those methods signatures part of run-time they don't become mandatory even when literal connection string used at run-time.
I still wonder if was worthwhile to introduce this breaking change.

@isaacabraham
Copy link
Author

@dmitry-a-morozov I've just tried 1.8.3-beta3 - still doesn't work I'm afraid :-( It's still looking at the FSI config file.

@isaacabraham
Copy link
Author

I take it back - just tried and it does work!

@isaacabraham
Copy link
Author

Question: which project does it "pull" the connection string from?

  1. For example, let's say we have a project A which contains the literal connection string (well, config connection string key) but the actual type provider command is in project B (but pulls the literal string from A). Which config file is used? Project A or B?
  2. What if you have Project A that has the literal connection string and the type provider type, but the actual call to create a TP command from the type is in Project B. What config file is used - A or B?

@dmitry-a-morozov
Copy link
Member

Good questions.
If you use absolute file path (which is rare) then project A config file will be used to capture design time connection string.
In case of relative path project B config file will be used. This is confusing but it mostly because a way erased type providers work.
I strongly suggest avoid pattern when erased type declared in one project but command instance created in another. Putting aside all erased type providers specifics this approach doesn't make sense.

@isaacabraham
Copy link
Author

@dmitry-a-morozov last question. Post-build, is there a way to override the connection string that the TP uses? :-) Sometimes after we build after a specific connection string, we want to change this afterwards.

If it's a non-starter, that's fine - but if it's possible, let me know and I'll open a separate issue for that.

@dsevastianov
Copy link
Contributor

Well I guess that's where explicit connection strings become handy again. :)

@smoothdeveloper
Copy link
Collaborator

@isaacabraham are we happy with how connection strings (explicit or not) are handled now in the library?

@isaacabraham
Copy link
Author

@smoothdeveloper Hey. I haven't touched this in a long time - I think we worked around (or with it) in some way - @theprash did we fix those issues with the SQL TP and F# scripts in the end?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants