Support for .NET Core

[tugberkugurlu]

⚠️ Not ready to be merged in. Just opening up the PR to have some discussion.

Fixes #135. First attempt to be able to distribute Neo4jClient for dotnet5.4. Got most of the transition handled but there are quite a few things to do:

• System.Transactions are not part of .NET Core and Neo4jClient seems to be havily taking advantage of the APIs available there. Need to decide what to do in this case before moving further now. An alternative would be (which is possible if the library would function without these transactions bits) to provide Transactions as a separate addition to core library and make that available only in net45 (means full desktop .net framework + mono).
• AppDomain API is not part of .NET core. However, it's only being used in TransactionPromotableSinglePhaseNotification. This would be resolved if the above proposed solution is viable.
• After above two are sorted out, the currently remaining 46 errors should disappear for Neo4jClient project. This should be checked and we need to make sure that Neo4jClient project compiles fine.
• Neo4jClient.Tests project uses a few libraries which doesn't support DNX based test runners and .NET Core. For now, we may get away with only running the tests under dnx451 but I propose to strip out NUnit and replace it with xunit whcih supports .NET Core. We may want to replace everything with suitable .NET core alternatives and make the tests run under dnxcore50.
• After all above are done and dusted, we need to remove nuspecs and NuGet relates stuff and be able to produce nuget packages through dnu pack.
• (Optional but nice to have) Lastly, it would be good to have some build scripts for the project. It would be good to be able to run the build and tests under AppVeyor and Travis CI at the same time.
• Extensive code review 💥 ✨
• Would be cool to squish the horrible commits to make them look meaningful (worth keeping them as is here for history reasons, but just merged the rebased/squished commits).

A few things to be aware of:

• The library was using ApplicationException. This seems to be gone and I replaced all of these with Exception.
• TypeDescriptor seems to be gone. I replaced it with Type.GetProperties. Should be OK but there is a slight change in the behaviour I guess. Needs to be looked at.
• dnx and dnu will be deprecated after RC2 AFAIK. There will be a new CLI tool available: dotnet. The concepts should be similar there.

How to Build

• Install dnvm.
• Run dnvm install 1.0.0-rc1-update1
• cd into Neo4jClient
• Run dnu restore and dnu build.

A few References

• .NET Platform Standard (permalink)
• .NET Reference Source
• .NET Core API Reference

[cskardon]

OK then, it looks like we're heading for a major version number update release! :)

First off though - WOW this is a lot of work, and fully appreciated, thank you very, very much... So as there is a lot of stuff there, I'll try to cover it all!

Transactions & AppDomain

My concern with removing Transactions into a separate package is that because transactions are such a fundamental part of using Neo4j (any DB really!) if we published a version without Tx support, we'd soon add it, and at that point, the Neo4jClient.Transactions implementation would be wasted effort. Is it perhaps worth removing the transaction code and re-doing it? It seems dangerously close to the bad times of a complete rewrite.

Having said that, separating the implementation of the transaction code would be benficial in many cases - in particular I'm thinking with reference to implementing the new Bolt protocol.

My gut feeling is that it's probably worth re-doing the Tx code, and abstracting it so the REST and Bolt clients can manage it in the way they each need to, but with the user seeing no code difference.

i.e. I'd like to see something like client.BeginTransaction(); /*shizzle here*/ client.CommitTransaction(); for both Bolt and REST.

Obviously the AppDomain stuff comes out of the wash here as well.

Downside to not using System.Transactions is the lack of distributed transaction handling, but maybe that's a price we should be willing to pay.

Tests

Sure, I'm happy with xunit - I presume you're wanting to hit dnxcore50 to get the lowest common denominator? The more running under dnxcore50 the better.

Nuget & Build scripts

Agreed

Code Review

Bang indeed!

Squishing

Feel free to squish if you want, I like to keep the history, but as it's your pull, you can do what you want, I wouldn't make you squish :)

ApplicationException

It's on my todo list to replace all instances of ApplicationException and Exception with appropiate exceptions.

TypeDescriptor

I seem to recall that whilst I was looking into this as well :/

[tugberkugurlu]

@cskardon great, got answers to most of the questions and we can get going.

is it perhaps worth removing the transaction code and re-doing it?

To be perfectly honest, I thought the same 😄 but didn't understand what it was being done with the transaction APIs. However, from what you said, I can understand that Transactions were just wrapping the HTTP calls to send info in chucks and commit them at the end, am I understanding this correct? Am I also correct assuming that this is the bit where registers the transaction support? https://github.com/tugberkugurlu/Neo4jClient/blob/c560f231aa64ac27fb1c0b71a4d089f7dd5772b8/Neo4jClient/GraphClient.cs#L159

[tugberkugurlu]

I presume you're wanting to hit dnxcore50 to get the lowest common denominator?

Yes and validating that the assumptions which are being made under tests are also valid under .NET Core execution (which would be dnxcore50). However, as an initial start, dnx451 would be more than enough on tests which means being able to run the tests under good-old Desktop .NET Framework 4.51 and Mono.

[cskardon]

Yes, I believe that is the point where the Tx stuff is handled...
I guess we could look at switching out the internals of the TransactionManager to manage it not using the TransactionScope stuff, which would be a breaking change for some code, but could be simpler.

It does wrap the calls to the Tx endpoint, but also does things like 'keep-alive' and manages interaction with the MSDTC, but as we know that's not in core.

[tugberkugurlu]

Just had a chance to come back to this.

MSDTC

First time I have ever heard of it, not sounds super fancy but not sure what it does 😄 Is it an important feature?

I guess we could look at switching out the internals of the TransactionManager to manage it not using the TransactionScope stuff

I guess this would be most logical next step, right?

[cskardon]

Yes, my general thoughts are, once this is done the codebase will need to be a new version, and breaking the tx stuff into a simpler model might be better in the long run.

[cskardon]

Hey @tugberkugurlu - Quick question - probably down to my lack of knowledge surrounding dnx etc - why are the .csproj files no longer there? Are you editing in Visual Studio - or Vs Code/Sublime etc?

Cheers!

[tugberkugurlu]

@cskardon csproj files are no longer needed in new project structure. It now works in a "include by default" manner for the files under the project folder. You still have xproj but that's just to satisfy the needs to VS which is not required.

[cskardon]

Cool so again, please excuse my ignorance, how do I open it in visual studio?

-----Original Message-----
From: "Tugberk Ugurlu" [email protected]
Sent: ‎14/‎03/‎2016 12:54
To: "Readify/Neo4jClient" [email protected]
Cc: "Chris Skardon" [email protected]
Subject: Re: [Neo4jClient] Support for .NET Core (#140)

@cskardon csproj files are no longer needed in new project structure. It now works in a "include by default" manner for the files under the project folder. You still have xproj but that's just to satisfy the needs to VS which is not required.
—
Reply to this email directly or view it on GitHub.

[tugberkugurlu]

@cskardon you can still have a .sln file which will point to .xproj files. So, opening the .sln file will open up the solution in VS.

Also, you can directly open a project.json file as a project in VS. When you do this, it auto generates a small xproj file which just has some msbuild stuff, not files references.

This is all today, might be changed before they hit RTM.

[cskardon]

Ahh cool, it's all new to me! :)

On 14 March 2016 at 13:03, Tugberk Ugurlu [email protected] wrote:

@cskardon https://github.com/cskardon you can still have a .sln file
which will point to .xproj files. So, opening the .sln file will open up
the solution in VS.

Also, you can directly open a project.json file as a project in VS. When
you do this, it auto generates a small xproj file which just has some
msbuild stuff, not files references.

This is all today, might be changed before they hit RTM.

—
Reply to this email directly or view it on GitHub
#140 (comment).

[cskardon]

I'm just going to close this for now, as well - you know!