Skip to content
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

BinaryFormatter deprecation for System.Configuration.ConfigurationManager #50531

Merged
merged 3 commits into from
May 13, 2021

Conversation

pgovind
Copy link

@pgovind pgovind commented Apr 1, 2021

This is a draft PR to turn off the BinaryFormatter code paths by default in System.Configuration.ConfigurationManager. Essentially all I'm doing here is deprecating SettingsSerializeAs.Binary and hiding the BinaryFormatter related code paths behind an AppContext switch.

@buyaa-n @joperezr @krwq @safern @steveharter @maryamariyan (these are the current/past area-owners). I'm not super familiar with this area so I'd like feedback about the approach here from the area owners. Specifically, if you can think of the following, that would be super appreciated:

  1. Scenarios that this PR breaks (I can't think of any since no code is being deleted and current behavior is enabled by the AppContext switch)
  2. Test cases I haven't added
  3. Anything else that I've missed

This is NOT the most minimal change. We might be able to get away with just marking SettingsSerializeAs.Binary as Obsolete. I, however, chose to be more conservative and hid all the BinaryFormatter code paths behind an AppContext switch since this way we actually deprecate the BinaryFormatter code paths.

Fixes #39295

cc @GrabYourPitchforks

@ghost
Copy link

ghost commented Apr 1, 2021

Tagging subscribers to this area: @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

This is a draft PR to turn off the BinaryFormatter code paths by default in System.Configuration.ConfigurationManager. Essentially all I'm doing here is deprecating SettingsSerializeAs.Binary and hiding the BinaryFormatter related code paths behind an AppContext switch.

@buyaa-n @joperezr @krwq @safern @steveharter @maryamariyan (these are the current/past area-owners). I'm not super familiar with this area so I'd like feedback about the approach here from the area owners. Specifically, if you can think of the following, that would be super appreciated:

  1. Scenarios that this PR breaks
  2. Test cases I haven't added
  3. Anything else that I've missed

This is NOT the most minimal change. We might be able to get away with just marking SettingsSerializeAs.Binary as Obsolete. I, however, chose to be more conservative and hid all the BinaryFormatter code paths behind an AppContext switch.

Fixes #39295

cc @GrabYourPitchforks

Author: pgovind
Assignees: -
Labels:

area-System.Configuration

Milestone: -

@jeffhandley jeffhandley marked this pull request as draft April 1, 2021 00:28
@pgovind pgovind marked this pull request as ready for review April 1, 2021 00:33
@pgovind pgovind marked this pull request as draft April 1, 2021 00:34
@pgovind
Copy link
Author

pgovind commented Apr 8, 2021

@@ -667,4 +667,4 @@
<data name="DuplicateFileName" xml:space="preserve">
<value>The file name '{0}' was already in the collection.</value>
</data>
</root>
</root>
Copy link
Author

Choose a reason for hiding this comment

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

Reset changes to this file

@pgovind
Copy link
Author

pgovind commented Apr 19, 2021

@steveharter : I'm guessing you have the most context with this library at the moment?

@steveharter
Copy link
Member

Note the test failure in CI (not support on .NET Framework)

C:\h\w\BF4A0985\w\9FE508EC\e>xunit.console.exe System.Configuration.ConfigurationManager.Tests.dll -xml testResults.xml -nologo -nocolor -noappdomain -maxthreads 1 -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing  
  Discovering: System.Configuration.ConfigurationManager.Tests (app domain = off, method display = ClassAndMethod, method display options = None)
  Discovered:  System.Configuration.ConfigurationManager.Tests (found 485 of 506 test cases)
  Starting:    System.Configuration.ConfigurationManager.Tests (parallel test collections = on, max threads = 1)
    System.ConfigurationTests.BinaryFormatterDeprecationTests.SerializeAndDeserializeWithSettingsSerializeAsBinary [FAIL]
      System.InvalidOperationException : RuntimeConfigurationOptions are only supported on .NET Core
      Stack Trace:
        /_/src/Microsoft.DotNet.RemoteExecutor/src/RemoteExecutor.cs(395,0): at Microsoft.DotNet.RemoteExecutor.RemoteExecutor.GetConsoleAppArgs(RemoteInvokeOptions options, IEnumerable`1& toDispose)
        /_/src/Microsoft.DotNet.RemoteExecutor/src/RemoteExecutor.cs(368,0): at Microsoft.DotNet.RemoteExecutor.RemoteExecutor.Invoke(MethodInfo method, String[] args, RemoteInvokeOptions options, Boolean pasteArguments)
        /_/src/libraries/System.Configuration.ConfigurationManager/tests/System/Configuration/BinaryFormatterDeprecationTests.cs(29,0): at System.ConfigurationTests.BinaryFormatterDeprecationTests.SerializeAndDeserializeWithSettingsSerializeAsBinary()
  Finished:    System.Configuration.ConfigurationManager.Tests

@@ -0,0 +1,44 @@
// Licensed to the .NET Foundation under one or more agreements.
Copy link
Member

Choose a reason for hiding this comment

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

Do we also care about adding a test case using reflection as well?

cc: @GrabYourPitchforks

Copy link
Author

Choose a reason for hiding this comment

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

I think in general we don't care about binary formatter usage if it is called explicitly via reflection by a user.

Copy link
Member

@maryamariyan maryamariyan left a comment

Choose a reason for hiding this comment

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

LGTM, aside from CI test issues

@pgovind pgovind marked this pull request as ready for review May 11, 2021 22:33
Prashanth Govindarajan added 2 commits May 11, 2021 15:34
@pgovind pgovind merged commit 2fb544c into dotnet:main May 13, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove BinaryFormatter usage from SettingsPropertyValue
5 participants