ErikEJ / EFCorePowerTools

Entity Framework Core Power Tools - reverse engineering, migrations and model visualization in Visual Studio & CLI
MIT License
2.05k stars 287 forks source link

Add Connection Properties Dialog Close Automatically. #2212

Closed developeerr closed 3 months ago

developeerr commented 3 months ago

I'm trying to add PostgreSQL connection via this following dialog. But when I try to enter character to any of these fields (for example host), this dialog closes automatically.

Is there any possibility of downloading older version of this tool?

image

ErikEJ commented 3 months ago

I doubt an older extension version fixes this. Try upgrading to VS 17.9

developeerr commented 3 months ago

I doubt an older extension version fixes this. Try upgrading to VS 17.9

Thank you for the reply.

I upgraded to the latest version which is VS 17.9.2 and still the issue is not resolved. But once I disabled the plugin "BlackbirdSQL DDEX 2.0 with SqlEditor" that shipped with EF Core Power pack, it works fine now.

ErikEJ commented 3 months ago

I will remove that from the Power Pack! Thanks for the investigation

ErikEJ commented 3 months ago

@blackbirdsql FYI!

BlackbirdSQL commented 3 months ago

Okay, so this is because the Npgsql autoload contexts are causing the extension to load late, so when BlackbirdSql loads it's Running Connection Table (asynchronously) and requests "skeleton" view support, VS enumerates the providers. Any providers that haven't registered their provider factory will have their invariant assembly invalidated when this enumeration occurs. I will have to implement a validation that ensures all providers are loaded. I am pretty sure that if you left an Npgsql edmx model open on closing a project and then restarted the IDE, the extension will also crash when VS auto reopens the model.

BlackbirdSQL commented 3 months ago

To replicate with SqlClient, add a Microsoft SQL Server Database File (Microsoft SqlClient) connection. Shutdown the IDE then restart and access the SE data nodes in the SqlClient connection quickly. The Npgsql extension will crash and be in an irrecoverable state and require an IDE restart.

ErikEJ commented 3 months ago

@BlackbirdSQL So you are saving this is caused by the Npgsql provider?

BlackbirdSQL commented 3 months ago

Well... ... ... it's BlackbirdSql that is exposing the flaw. Provider factory registration should be in a static constructor for async DDEX extensions, because it's a critical component, but I'm sure half the DDEX extensions out there don't do that, so the onus is on me to ensure they don't crash due to late loading when the BlackbirdSql running connection table is loaded. Also, async DDEX extensions should be able to recover in instances where they cannot load on time. For example the edmx and xsd extensions, and other MS packages, will always get in ahead of it.

BlackbirdSQL commented 3 months ago

Okay so that turned out to be far easier than I had anticipated. Problem solved. BlackbirdSql will now ensure all provider factories are registered before requesting any Firebird model initializations.

Do you want me to log the issue with Npgsql or will you?

ErikEJ commented 3 months ago

Can you elaborate or link to your fix, it will make it easier to fix for npgsql

BlackbirdSQL commented 3 months ago

Can you elaborate or link to your fix, it will make it easier to fix for npgsql

Sure. (But just so that no one feels offended... The only reason I know all this is because I encountered this issue myself, otherwise I would have been ignorant. So I am not beating my chest or patting myself on the back on this.)

This is the method that recovers invalidated provider factories (FYI): https://github.com/BlackbirdSQL/Firebird-DDEX-SqlEditor/blob/85957e6c32836c6f7620996286219cafdf24b3d0/BlackbirdSql.Core/Ctl/Extensions/DbProviderFactoriesEx.cs#L214

An example UIContext for auto-loading async DDEX extensions (as used in BlackbirdSql): https://github.com/BlackbirdSQL/Firebird-DDEX-SqlEditor/blob/85957e6c32836c6f7620996286219cafdf24b3d0/BlackbirdSql.VisualStudio.Ddex/BlackbirdSqlDdexExtension.cs#L71

The call to registering the factory using the package static constructor: https://github.com/BlackbirdSQL/Firebird-DDEX-SqlEditor/blob/85957e6c32836c6f7620996286219cafdf24b3d0/BlackbirdSql.VisualStudio.Ddex/BlackbirdSqlDdexExtension.cs#L133 and https://github.com/BlackbirdSQL/Firebird-DDEX-SqlEditor/blob/85957e6c32836c6f7620996286219cafdf24b3d0/BlackbirdSql.VisualStudio.Ddex/BlackbirdSqlDdexExtension.cs#L395

The factory registration method (Do not use ConfigurationManager. It likely won't be ready.): https://github.com/BlackbirdSQL/Firebird-DDEX-SqlEditor/blob/85957e6c32836c6f7620996286219cafdf24b3d0/BlackbirdSql.Core/Ctl/Extensions/DbProviderFactoriesEx.cs#L363

ErikEJ commented 3 months ago

@roji FYI - it's a black magic box for me.. and where is the source code for the Npgsql vsix?

roji commented 3 months ago

We no longer maintain the Npgsql VSIX - we stopped working on it (and publishing new versions) a while ago, the last version that was published was 4.1.12. However, that should still work well, and we may actually be publishing a 4.1.13 soon - so it may be a good time patch that.

Realistically, I don't think anyone on the Npgsql team is going to have time to do work on the VSIX any time soon; but @BlackbirdSQL if you (or anyone else) is willing to submit a PR with a fix, I'll be happy to merge and then release a new VSIX. This would be in the hotfix/4.1.13 branch on the Npgsql repo (https://github.com/npgsql/npgsql/tree/hotfix/4.1.13/src/VSIX).

ErikEJ commented 3 months ago

@roji Great news. @BlackbirdSQL I am very unsure what exactly needs to be fixed in the Npgsql DDEX provider based on your links above - interested in providing more info or even a PR?

BlackbirdSQL commented 3 months ago

@roji Great news. @BlackbirdSQL I am very unsure what exactly needs to be fixed in the Npgsql DDEX provider based on your links above - interested in providing more info or even a PR?

I'll give it a shot but I nothing about forks, branches etc. on github. The patches required are minimal.

ErikEJ commented 3 months ago

Yeah, it could be uphill for you then. You need to fork the repo, clone it locally , check out the hotfix branch and check out a feature branch based on that.

Alternatively you can just add them here, and I will do a PR.

BlackbirdSQL commented 3 months ago

The VSIX is so far behind the ball game that I tried pulling it into the latest npgsql, but that no longer includes .NetFramework, so it looks like I'll be sticking with the alternative, which is to recover it after it has crashed.

roji commented 3 months ago

@BlackbirdSQL the idea is definitely not to pull the VSIX into the latest Npgsql, but rather to submit a PR against the 4.1 version; this still has .NET Framework support etc. Do you see a problem with doing the fixes there?

BlackbirdSQL commented 3 months ago

... submit a PR against the 4.1 version; this still has .NET Framework support etc. Do you see a problem with doing the fixes there?

@roji When I have some time I will go back to 4.1.13 and provide you with a rundown of the issues I encountered. I spent some time on it (well several hours), and if I recall correctly, outdated packages were stonewalling me.

ErikEJ commented 3 months ago

If the fix is simple maybe you can just share it here, and I will do a PR?

BlackbirdSQL commented 3 months ago

If the fix is simple maybe you can just share it here, and I will do a PR?

Okay I'm looking at it now.

BlackbirdSQL commented 3 months ago

Okay it's implemented and working.

Sorry but how do I do a feature branch & PR. It will save some time if I don't have to go digging on how to do that.

BlackbirdSQL commented 3 months ago

The fixes are all contained within the package source file NpgsqlVSPackage.cs.

https://github.com/BlackbirdSQL/npgsql/commit/dc3f682004065a79017a345f9b49d250a6fd58de

ErikEJ commented 3 months ago

I will do a PR, thanks so much for your help improving the DDEX ecosystem

BlackbirdSQL commented 3 months ago

@ErikEJ @roji Yeah so the mission to mars was because it needed the Reflection package which I installed as the latest version instead of the existing. That lead down a rabbit hole.

BlackbirdSQL commented 3 months ago

I will do a PR, thanks so much for your help improving the DDEX ecosystem

@ErikEJ Do you need access to the repository?

ErikEJ commented 3 months ago

I have all the access I need thanks

ErikEJ commented 3 months ago

@developeerr I have re-added the Firebird provider to EF Core Power Pack, and the latest version of the provider avoids this issue.

ErikEJ commented 3 months ago

If you like my free tools, I would be very grateful for a rating or review on Visual Studio Marketplace or even a one-time or monthly sponsorship