Embedded license file ingestion for Gallery

[agr]

Addresses #6545, #6529, #6516, #6532

New column was added to GalleryDB to contain the type of the embedded license file (plain text or markdown). Storage is implemented for the case when no async validation pipeline is present.

I ended up diverging from the spec which prescribed storing a presence of an embedded file as a bool flag because this way for the local storage case we lose the document type information. So an enum was introduced that persists that information in DB.

Warning when package does not contain <license> but specifies <licenseUrl>:

Warning when package does not contain any license information:

License URL rejection message (when enabled):

License file rejection message:

Warning when license URL is not aligned with license expression:

I haven't seen the latest revision.

[loic-sharma]

https://aka.ms/deprecateLicenseUrl [](start = 53, length = 34)

FYI, this link redirects to nuget's documentation home page. Is that intended?

[scottbommarito]

We need to remember to update it to something useful when the doc becomes available.

[loic-sharma]

IsStreamLengthMatchesReportedAsync [](start = 40, length = 34)

This could be a nice Stream extension method. Maybe you could make a Stream extension class that also has LooksLikeUtf8TextStreamAsync

[agr]

I don't like to have a Stream extension methods that read from it. It should be very obvious that this operation consumes Stream (which is important for streams that are not seekable). I don't think extensions methods make it obvious.

[loic-sharma]

HasChildElements [](start = 28, length = 16)

What do you think of moving HasChildElements, GetLicenseVersion and GetLicenseType into LicenseCheckingNuspecReader?

[scottbommarito]

IMO LicenseCheckingNuspecReader should do all XML-based operations itself and completely abstract away the fact the XML-shape of the nuspec.

[agr]

I specifically moved them out not to call LicenseCheckingNuspecReader.LicenseElement repeatedly.

[scottbommarito]

Ideally, you could move all the functions into LicenseCheckingNuspecReader such that you can make LicenseElement private.

private class LicenseCheckingNuspecReader : NuspecReader
{
    private XElement _licenseElement => MetadataNode.Element(MetadataNode.Name.Namespace + LicenseNodeName);

    public string GetLicenseVersion() => _licenseElement.GetOptionalAttributeValue("version");

    /* other functions here */
}

[agr]

I don't know what is the cost of calling MetadataNode.Element(MetadataNode.Name.Namespace + LicenseNodeName) multiple times and I don't want to care about it: I call it once and reuse the result as much as possible. If I make them members of LicenseCheckingNuspecReader then I'd have to make that Element call in each method or do something atrocious to keep the value between calls.

[agr]

I also don't want LicenseCheckingNuspecReader to exist, since it's only purpose is to get access to one protected member of a base class.

[agr]

I tried to do that and rolled back the change. It looks ugly.

It makes LicenseCheckingNuspecReader to provide methods that check the content of the license element in various ways (a methods to check if it exists, a method to check its length, a method to check if there are child elements, etc.). Class methods cannot assume licenseElement is not null, so this check creeps into every method whereas in the original code its a single check.

[scottbommarito]

IMO you should split a lot of this code into separate files. Otherwise looks great.

[scottbommarito]

We need to remember to update it to something useful when the doc becomes available.

[scottbommarito]

IMO LicenseCheckingNuspecReader should do all XML-based operations itself and completely abstract away the fact the XML-shape of the nuspec.

[scottbommarito]

Move this to its own file.

[agr]

Why? It is only useful in the context of validation, I don't want to make it visible to all the code.

[scottbommarito]

This file is huge. Anything you can do to split it up into more logical parts would improve the readability of it.

[agr]

The file is under 1000 lines. I've see methods longer than that :)

On a more serious note: moving it into separate file and making it public as a result would require to add a lot of code to be more defensive about how its methods are called. That will bloat its code which will not help the readability.

[agr]

I created work item to refactor the PackageUploadService, we'll address these issues when we'd be working on it.

[scottbommarito]

What's the benefit of making the method static? IMO the lambda here just makes the code more confusing--you're never going to be making this function do anything else other than save the file, so you've created a method named SavePackageLicenseFile that needs to be told how to save the package license file. You should just pass in the Package and call SaveLicenseFileAsync directly.

One other option: if the stream returned by packageArchiveReader.GetEntry is still valid after packageArchiveReader is disposed, you could rename SavePackageLicenseFile to GetPackageLicenseFile and then return the stream so you don't have to pass the Package to the function.