Adding reference to MessagePackAnalyzer to check for MsgPack001 / MsgPack002 (Banned API)

[tebeco]

Description

Make sure MessagePack built in analyzers run to be sure the MessagePackSerializerOptions are not using BannedApi

Addresses #18290

Details :

• Add dependency on package MessagePackAnalyzer
• Update to 2.1.90
• Added .editorconfig for analyzers
• Use the same variable for both package
• MessagePackAnalyzer should not be "leaked as a transitive" to consumer as it wont be possible to enforce the same severity as this repo set in .editorconfig
• Make MessagePackSerializerOptions readonly

Results

Generated nuget / nuspec :

<?xml version="1.0" encoding="utf-8"?>
<package xmlns="http://schemas.microsoft.com/packaging/2013/05/nuspec.xsd">
  <metadata>
    <id>Microsoft.AspNetCore.SignalR.Protocols.MessagePack</id>
    <version>5.0.0-dev</version>
    <authors>Microsoft</authors>
    <owners>Microsoft</owners>
    <requireLicenseAcceptance>true</requireLicenseAcceptance>
    <license type="expression">Apache-2.0</license>
    <licenseUrl>https://licenses.nuget.org/Apache-2.0</licenseUrl>
    <icon>Icon.png</icon>
    <projectUrl>https://asp.net/</projectUrl>
    <description>Implements the SignalR Hub Protocol over MsgPack.

This package was built from the source code at https://github.com/dotnet/aspnetcore/tree/19c629ffd143a6a666c6a8912889156a595185de</description>
    <copyright>© Microsoft Corporation. All rights reserved.</copyright>
    <tags>aspnetcore signalr</tags>
    <serviceable>true</serviceable>
    <repository type="git" url="https://github.com/dotnet/aspnetcore" commit="19c629ffd143a6a666c6a8912889156a595185de" />
    <dependencies>
      <group targetFramework=".NETStandard2.0">
        <dependency id="Microsoft.AspNetCore.SignalR.Common" version="5.0.0-dev" exclude="Build,Analyzers" />
        <dependency id="MessagePack" version="2.1.90" exclude="Build,Analyzers" />
      </group>
    </dependencies>
  </metadata>
</package>

[AArnott]

Should it be a "transitive leaked" to consumer to make sure the analyzer runs for them too ?

Probably not, since you can't transitively leak the per-rule severity settings and I think the defaults aren't what you want anyway.

[tebeco]

Should it be a "transitive leaked" to consumer to make sure the analyzer runs for them too ?

Probably not, since you can't transitively leak the per-rule severity settings and I think the defaults aren't what you want anyway.

Here is where i lack of knowledge,
Will it work with <Reference> ?

<Reference ...>
  <PrivateAssets>analyzers;</PrivateAssets>
</Reference>

Do i just use all so it will be complitly cut off ?

[AArnott]

Will it work with <Reference> ?

Will what work with <Reference>? You don't add any Reference items in this PR, nor should you. This is a reference to an analyzer package, so you should always use PackageReference. You can prevent it leaking by adding PrivateAssets=all metadata to the PackageReference item.

[tebeco]

I meant that because this repo does not directly use <PackageReference but <Reference, also i should have look for it in the repo before asking, sorry about that.
Yes it seems to be already used the same way in the repo

Updated this PR : https://github.com/tebeco/AspNetCore/commit/0737ecc65431733d594f4a15f86135eadbe352b5#diff-54a03b3b1112866eeff607edc2c3ec95L20

[AArnott]

Sorry. I guess when I said you don't have any Reference items in this PR I wasn't looking carefully. I guess you're using Reference instead of PackageReference, but I have no idea why or how that works.

[tebeco]

because it's specific to this repository.
After digging into the .targets file from dotnet/arcade and so on, to my very limited understanding, this is done like this to enable "partial solution load" / Baseline ...

So if you

• run restore.cmd (it will restore all packages from eng/Dependencies.props + eng/Versions.props ==> restore to specific folder
• build.cmd at root level ==> probably build to specific folder too (or make if "findable" later)
• startvs.cmd from a subfolder

the <Reference will trigger some MsBuild Targets that will properly "Reference" what it actually need to reference, that being either a Project or a Package.
The idea is that you should not care about this as it's would be a huge "sln" and might be more difficult to load

[AArnott]

How curious. Thanks for sharing.

[tebeco]

i can't find anymore what i found at that precise moment, but here are some places that seems related to that logic :
https://github.com/dotnet/aspnetcore/blob/master/eng/targets/ResolveReferences.targets#L3

https://github.com/dotnet/aspnetcore/blob/master/eng/targets/ResolveReferences.targets#L148-L160
https://github.com/dotnet/aspnetcore/blob/master/eng/targets/ResolveReferences.targets#L182-L184

[AArnott]

I guess this makes sense as to why this would appear in the aspnetcore repo. IIRC @davidfowl wanted this kind of "loaded project vs. fallback to nuget package" behavior since way back in the project.json days. I guess in this repo he/someone actually built it.

[analogrelay]

• Do i keep or drop the update to 2.1.90 ?

Keep it, there's no problem updating while we're in the middle of 5.0.

• Do i add root = false to the .editorconfig

I think false is the default, so I don't see a need to add it. It's fine for this to cascade. I can't speak to why MVC decided their .editorconfig should not cascade but for SignalR I'd prefer not setting root = true.

[tebeco]

(updated the first comment with these infos)
Anything else to do ?

I saw a potential way to change the MessagePackSerializerOption to be readonly, but not sure it should be in this PR or another very small one
also i don't like the naming i used for that, should i first open an issue to discuss about this ?

[analogrelay]

I saw a potential way to change the MessagePackSerializerOption to be readonly, but not sure it should be in this PR or another very small one

Making it readonly in this PR is fine.

also i don't like the naming i used for that, should i first open an issue to discuss about this ?

You mean the naming of the field? I'm fine with you changing that in this PR.

[tebeco]

You mean the naming of the field? I'm fine with you changing that in this PR.

You'll see in a second ^^

[tebeco]

(see PR Description, i inserted NPE screenshot and nuspec content that were generated by running build.cmd -pack)

[analogrelay]

Looks OK to me but would like @BrennanConroy and/or @halter73 to take a look.