Skip to content
This repository has been archived by the owner on May 8, 2020. It is now read-only.

Port to .NET Core 3 #40

Closed
wants to merge 56 commits into from
Closed

Conversation

0xced
Copy link

@0xced 0xced commented Nov 5, 2019

Here's a pull request porting ICanHasDotnetCore to .NET Core 3 with a few side improvements. I have tried to break things in isolated commits so in case you don't want to merge it altogether you can cherry-pick commits of interest.

Here are the most important changes:

  • Use Entity Framework Core instead of raw SQL
    This change makes it super easy to develop with SQLite instead of SQL Server (I did all the work on macOS using JetBrains Rider). To develop on SQLite, just add this in appsettings.Development.json:
{
  "Database": {
    "Provider": "Sqlite",
    "ConnectionString": "DataSource=/path/to/ICanHasDotnetCore.sqlite3"
  }
}

And maybe you could even use SQLite in production and save money on an SQL Server instance?

  • Use NuGet.Protocol instead of NuGet.Core which is not compatible with .NET Core. This change has brought to light some packages that were incorrectly identified, the tests have been adapted accordingly.
  • Improved logs overall. The logs added for Octokit allowed me to identify how to improve performance of GitHub scanning.
  • Web project modernization

Updated the web project to work with Node.js 12 and Gulp 4. I also added a new Scan A NuGet package tab to get the dependency graph of a NuGet package without having to upload a file.

And here are less important changes:

  • Updated NuGet packages to their latest versions
  • Replaced AutoFac with built-in ASP.NET Core equivalent features
  • Replace Newtonsoft.Json with System.Text.Json
  • Asyncified all code
  • Updated knowledge
  • Fixed a potential ArgumentNullException

0xced added 16 commits October 18, 2019 13:27
This fixes GitHubScannerTests.ICanHasDotnetRepository and GitHubScannerTests.RepoDoesNotExist
Also replace the retired Serilog.Sinks.ColoredConsole and Serilog.Sinks.LiterateConsole with Serilog.Sinks.Console
Also use the IMiddleware interface for RedirectWwwMiddleware
@CLAassistant
Copy link

CLAassistant commented Nov 5, 2019

CLA assistant check
All committers have signed the CLA.

@droyad droyad self-requested a review November 6, 2019 02:55
@droyad droyad self-assigned this Nov 6, 2019
@0xced 0xced force-pushed the port-to-dotnet-core branch 2 times, most recently from 71d42ed to efbe61c Compare November 13, 2019 01:10
@0xced 0xced force-pushed the port-to-dotnet-core branch 2 times, most recently from 2533bd3 to 4bfb5ec Compare November 15, 2019 18:04
Scanning a GitHub repository now supports the following syntax: Username/Repository@Reference with the last part "@reference" being optional. This allows to scan a repository for a given tag, branch or specific commit with its SHA1.
This solves the case of "Not a .NET Library" being only available in pre-release such as Microsoft.NETFramework.ReferenceAssemblies.

Before this commit it would diagnose Microsoft.NETFramework.ReferenceAssemblies as "Not Found". After this commit, it is correctly diagnosed as "Not a .NET Library".
@0xced 0xced force-pushed the port-to-dotnet-core branch from 4bfb5ec to d1e5ecd Compare November 16, 2019 02:04
This fixes ReSharper/Rider warnings
They now share a common implementation (EmbeddedResourceRepository) and are not static anymore
And use await using for IAsyncDisposable
@0xced 0xced force-pushed the port-to-dotnet-core branch from d1e5ecd to 3bc9e7f Compare November 17, 2019 21:56

try
await using var context = _contextFactory();
var packageStatistic = await context.PackageStatistics.FindAsync(new object[] {package.PackageName}, cancellationToken);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no longer atomic. I used the MERGE command as it guaranteed (from my understanding) atomic insert/update of that record. With this change two threads could update the same statistic record and only the last in wins.

Copy link
Author

Choose a reason for hiding this comment

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

Good catch! I addressed this issue in 49561db. I used FlexLabs.EntityFrameworkCore.Upsert as EF Core does not handle upsert out of the box.

By increasing the EF Core log level to information (.MinimumLevel.Override("Microsoft.EntityFrameworkCore", LogEventLevel.Information)) and enabling sensitive data logging (optionsBuilder.EnableSensitiveDataLogging();) we can see how FlexLabs.Upsert works:

With SQLite:

-- Executing DbCommand [Parameters=[@p0='Newtonsoft.Json' (Nullable = false) (Size = 15), @p1='1' (DbType = String), @p2='Supported' (Nullable = false) (Size = 9), @p3='1' (DbType = String)], CommandType='Text', CommandTimeout='30']
INSERT INTO "PackageStatistics" AS "T" ("Name", "Count", "LatestSupportType") VALUES (@p0, @p1, @p2) ON CONFLICT ("Name") DO UPDATE SET "Count" = ( "T"."Count" + @p3 )

With SQL Server:

-- Executing DbCommand [Parameters=[@p0='Newtonsoft.Json' (Nullable = false) (Size = 450), @p1='1', @p2='Supported' (Nullable = false) (Size = 4000), @p3='1'], CommandType='Text', CommandTimeout='30']
MERGE INTO [PackageStatistics] WITH (HOLDLOCK) AS [T] USING ( VALUES (@p0, @p1, @p2) ) AS [S] ([Name], [Count], [LatestSupportType]) ON [T].[Name] = [S].[Name] WHEN NOT MATCHED BY TARGET THEN INSERT ([Name], [Count], [LatestSupportType]) VALUES ([Name], [Count], [LatestSupportType]) WHEN MATCHED THEN UPDATE SET [Count] = ( [T].[Count] + @p3 );

@droyad
Copy link
Contributor

droyad commented Nov 19, 2019

@0xced I haven't forgotten about this and will get to doing a full review. I did a quick scan and only one thing stood out to me so far (added a comment)

EF Core does not support upsert out of the box, see dotnet/efcore#4526

But Artiom Chilaru created a NuGet package (FlexLabs.EntityFrameworkCore.Upsert) that handle upsert for PostgreSQL/Sqlite, SqlServer and MySQL.
@0xced
Copy link
Author

0xced commented Nov 20, 2019

Excellent, I’ll stop force-pushing small changes while you are reviewing.

Would you consider using SQLite in production? I think for a simple two tables database (cache + stats) it would be well enough and cheaper than an SQL Server instance. Are you currently using an Azure SQL Database? Is it costly?

@0xced
Copy link
Author

0xced commented Mar 17, 2020

I have pushed a few more improvements. We could probably shave a few more milliseconds when scanning GitHub repositories if octokit/octokit.net#2151 gets merged and a new release of Octokit is released.

@droyad
Copy link
Contributor

droyad commented May 7, 2020

@0xced Thank you for this contribution. Unfortunately we've decided to shutter the site. The traffic has decreased significantly since .NET Core 3.1 was released (and previous 2.0). The ecosystem as a whole has transitioned over and the majority of use cases now work.

@droyad droyad closed this May 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants