Skip to content

Adding Microsoft.CodeAnalysis.BannedApiAnalyzers for SignalR MessagePackHubProtocol #18291

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 30 commits into from
Closed

Adding Microsoft.CodeAnalysis.BannedApiAnalyzers for SignalR MessagePackHubProtocol #18291

wants to merge 30 commits into from

Conversation

tebeco
Copy link
Contributor

@tebeco tebeco commented Jan 11, 2020

TLDR

This PR were closed because the feature is already available out of the box from MessagePack itself
it was done here : #19989

Summary of the changes (Less than 80 chars)

  • Adding Microsoft.CodeAnalysis.BannedApiAnalyzer
  • Be sure MessagePackSerializer is properly called with MessagePackSerializerOptions

Addresses #18290
(Sorry to ping you here @BrennanConroy , but as you took part in the Review of #18133 I'd love your feedback here too)

Warning

This PR now contains the history of #18133, in order to already build against MessagePack 2.0.x.
the very first commit of this PR is :
Adding a new dependency to the solution for Microsoft.AspNetCore.SigalR.Protocols.MessagePack.csproj (URL can change/be broken on rebase)

Main doubts here :

  • Do i use Banned Methods (aka: is this what you had in mid @AArnott) ?
  • Do i ban all overloads of MessagePackSerializer.Serialize() and MessagePackSerializer.Deserialize() ?
  • Will it works since MessagePackSerializerOptions is defined as optional (defaulted to null) ?
  • Do i need to declare possible combination with CancellationToken too ? (for now that's what i did)
  • Does the files BannedTypes.txt supports empty lines in the middle ? (yeah dummy questions ^^)
  • Does it need to be done for MessagePackSerializer.Typeless too ?
  • I'm not sure about the syntax for ref parameters, so far i've kept it :
    public static void Serialize(Type type, ref MessagePackWriter writer, object obj, MessagePackSerializerOptions options = null);
    M:MessagePackSerializer.Serialize``1(ref MessagePack.MessagePackWriter,``0)
  • not sure I've used the right syntax for generic especially when it does mix <T> and <byte> like :
    public static void Serialize<T>(IBufferWriter<byte> writer, T value, MessagePackSerializerOptions options = null, CancellationToken cancellationToken = default);
    M:MessagePackSerializer.Serialize``1(System.Buffers.IBufferWriter<byte>,``0);
    should it be :
    M:MessagePackSerializer.Serialize``1(System.Buffers.IBufferWriter``byte,``0); ?

@tebeco tebeco changed the title Adding bannedapi analyzer for messagepack Adding Microsoft.CodeAnalysis.BannedApiAnalyzers for SignalR MessagePackHubProtocol Jan 11, 2020
@AArnott
Copy link
Contributor

AArnott commented Jan 11, 2020

It's hard to sift out the banned changes from all the other changes. If you changed your PR to target your source branch for the other PR, then the diff github would show would just include the banned API changes.

From what I can tell, these changes are along the lines of what I had in mind. In terms of syntax for the .txt file, I wouldn't write any of these by hand. It's the syntax used in an xml doc comment file when it references a member, so I would always copy and paste from there, but that's really tedious.

Given your interest in getting this done, I'll spend some time today on an analyzer in the messagepack repository that you may be able to just activate to accomplish the same goal. This way you won't have to maintain the list of overloads to ban.

@tebeco
Copy link
Contributor Author

tebeco commented Jan 11, 2020

That would be possible only if the branch was in dotnet/aspnetcore repository, so i don't think i can set the base branch pointing to the one that update MessagePack
Or did i missed something ?
i could close this PR and create it only as part of my repository so it would also generate noise here afterward

For now, as you said the way to diff it is to navigate through commit
if you go the commit with the first change (link above) then use p(previous) and n(next) it can help
i tried to make the current changes "logic" and easy to review
That's also why I locked it as a Draft PR

@tebeco
Copy link
Contributor Author

tebeco commented Jan 11, 2020

Out of curiosity, what means Expected -- ....

image
It seems stuck since ~3h
I guess that the CI does not react the same way depending on the base branch or the draft but i did not expect one Waiting indefinitly

@tebeco
Copy link
Contributor Author

tebeco commented Jan 11, 2020

Given your interest in getting this done, I'll spend some time today on an analyzer in the messagepack repository that you may be able to just activate to accomplish the same goal. This way you won't have to maintain the list of overloads to ban.

Don't mind me here, this PR can be closed and the issue kept open waiting to be discussed by AspNetCore team.
To be honest i did that because i was really curious about your suggestion, so i tried to take a look at what it looked like.
... And i had time to kill

@Pilchie Pilchie added the area-signalr Includes: SignalR clients and servers label Jan 13, 2020
tebeco added 21 commits January 14, 2020 15:05
…tem.Threading.Tasks.Extensions from 4.5.2 to 4.5.3)
…ckReader itself, expection done for reader.ReadBytes()
…ing MessagePackReader that is already consuming the same "ReadOnlyMemory<byte> data
…w the length of the payload before writing it
@tebeco
Copy link
Contributor Author

tebeco commented Jan 15, 2020

@AArnott
If i understand correctly, Should i close this PR, in favor of MessagePack-CSharp/MessagePack-CSharp#769 ?

@AArnott
Copy link
Contributor

AArnott commented Jan 15, 2020

Yes

@tebeco tebeco closed this Jan 15, 2020
@tebeco tebeco deleted the adding-bannedapi-analyzer-for-messagepack branch January 15, 2020 16:58
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-signalr Includes: SignalR clients and servers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants