Fix serialization of exceptions by BinaryFormatter in .NET Framework #5054
Conversation
|
/backport to rel/3.8 |
|
Started backporting to rel/3.8: https://github.com/microsoft/testfx/actions/runs/13387310059 |
src/TestFramework/TestFramework/Exceptions/AssertFailedException.cs
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5054 +/- ##
==========================================
- Coverage 66.79% 66.67% -0.12%
==========================================
Files 603 603
Lines 36243 36279 +36
==========================================
- Hits 24207 24189 -18
- Misses 12036 12090 +54
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Reviewing the link dotnet/docs#34893, I think we should be really following the advices there and add the protected ctor always and make it obsolete for NET8.0+:
#if NET8_0_OR_GREATER
[Obsolete(DiagnosticId = "SYSLIB0051")]
#endif
microsoft-github-policy-service
bot
added
Needs: Author Feedback
Needs: Attention 👋
and removed
Needs: Author Feedback
Needs: Attention 👋
labels
|
As I read it that recommendation does not address the case where we did not have the serialization apis before we decided to Obsolete them. So the recommendation does not apply here. They recommend to not rely on binary formatter first and foremost. This we cannot influence, but we can avoid people using the new constructor by accident, hance the obsoletion and browsable Never. They recommend to remove the serialization constructor if we can, which we effectively do in .NET, by having compiler condition. If we cannot remove in .net (which is not our case) we should obsolete it. For that they recommend to conditionally obsolete it in .NET 8. So afaik my code is correct. |
|
I am referring to: If you're creating a custom System.Exception-derived type Consider whether you truly need your custom exception type to be serializable. Chances are you do not need it to be serializable, ... <- yup we don't need that in .net. Consider simply removing the [Serializable] attribute, the serialization constructor, and the GetObjectData method override, as shown below. <- done by not defining them for .NET There may be some cases where you cannot remove these APIs from your custom exception types. This might occur if you produce a library constrained by API compatibility requirements. <- we did not have them till now so we don't need to keep compatibility. |
|
I'd prefer to have consistent behavior. If we want to support .NET Framework, let's support everywhere with obsolete on net8.0+ as suggested by the docs. My second preference is to only support .NET Framework, and not include the serialization constructor on netstandard2.0. In all cases, it feels wrong to have a publicly visible API on netstandard2.0 but not on net6.0+. Consider a library that only targets netstandard2.0 and uses such API. That library is then consumed in net6.0+ app, then we are welcoming a MissingMethodException. To me, generally an API contract of netstandard should be compatible with both .NET Core and .NET Framework |
|
amary will continue here on the suggestion above, with having the api everywhere and only conditional obsolete. Thanks @Evangelink I took the obsolete message from here: internal const string LegacyFormatterImplMessage = "This API supports obsolete formatter-based serialization. It should not be called or extended by application code."; internal const string LegacyFormatterImplDiagId = "SYSLIB0051"; internal const string SharedUrlFormat = "https://aka.ms/dotnet-warnings/{0}"; https://github.com/dotnet/runtime/blob/main/src/libraries/Common/src/System/Obsoletions.cs#L8 |
|
/backport to rel/3.8 |
|
Started backporting to rel/3.8: https://github.com/microsoft/testfx/actions/runs/13393573124 |
|
@Evangelink backporting to rel/3.8 failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Add tests
Applying: Fix binary formatter serialization for .NET Framework
Applying: Add obsolete to test using obsolete api
Applying: Address review feedback
Using index info to reconstruct a base tree...
M src/TestFramework/TestFramework/PublicAPI/PublicAPI.Unshipped.txt
Falling back to patching base and 3-way merge...
Removing src/TestFramework/TestFramework/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt
Removing src/TestFramework/TestFramework/PublicAPI/netstandard2.0/PublicAPI.Shipped.txt
Removing src/TestFramework/TestFramework/PublicAPI/netframework/PublicAPI.Unshipped.txt
Removing src/TestFramework/TestFramework/PublicAPI/netframework/PublicAPI.Shipped.txt
Auto-merging src/TestFramework/TestFramework/PublicAPI/PublicAPI.Unshipped.txt
CONFLICT (content): Merge conflict in src/TestFramework/TestFramework/PublicAPI/PublicAPI.Unshipped.txt
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0004 Address review feedback
Error: The process '/usr/bin/git' failed with exit code 128NOTE: A PR will be created, but needs to be revised manually!** |
The change adds protected constructors that binary formatter needs to deserialize exceptions. All added api targets .NET Framework, and .NET Standard 2.0, to avoid bringing the new apis to .NET.
All those apis are obsoleted the same way they are obsoleted in .NET.
Fix #5048