Make System.Security.Cryptography.Oid write-once (initializable read-only)

[bartonjs]

The Oid class, reductio ad absurdum, is

public class Oid
{
    public Oid() {}
    public Oid(string value, string friendlyName) { Value = value; FriendlyName = friendlyName }
    public string Value { get; set; }
    public string FriendlyName { get; set; }
}

There's a bit of "build FriendlyName for you" logic and some other lookup routines, but that's essentially what it is.

Because the properties are mutable, things like ECCurves.NamedCurves.get_nistP256() return a new Oid instance on every call; lest static state get corrupted.

This proposal suggests changing the properties to allow "assign-once" semantics. Building the property via an object initializer would still be permitted, but once the property gets a non-null value it cannot be changed.

Reductio ad absurdum implementation:

public partial class Oid
{
    public string Value
    {
        get => _value;
        set
        {
            if (_value != null && !_value.Equals(value))
                throw new PlatformNotSupportedException(...);

            _value = value;
        }
    }

    public string FriendlyName
    {
        get => _friendlyName ??= LookupFriendlyName(_value);
        set
        {
            if (_friendlyName != null && !_friendlyName.Equals(value))
                throw new PlatformNotSupportedException(...);

            _friendlyName = value;
        }
    }
}

Telemetry shows very few callers of the setters, but not "zero".

Benefits:

• We can stop making defensive copies of Oid values
• We could make accelerators for well-known (frequently utilized) Oid values.
  • Or libraries that want to can do so without the risk of static value corruption.

[bartonjs]

fyi: @GrabYourPitchforks @terrajobst

[vcsjones]

I would like this, I wonder if that means it would be feasible to implement equality members on Oid correctly (a proposal I made but withdrew: https://github.com/dotnet/corefx/issues/39057)

[GrabYourPitchforks]

What would the equality operators do on an uninitialized instance?

[bartonjs]

I thought about going further and suggesting IEquatable; but then we'd need to be ctor-only. I started at that position, then saw non-zero set calls in telemetry, and softened my stance.

I'm personally content to go whole-hog and make the setters be throw-if-not-my-value (or just straight throw, but that sometimes leads to bad debugger experiences).

We'd also need to decide if equality was only on Value (probably what someone wanted) or on both properties (differently makes sense).

Oid normalized = new Oid(OidValues.RsaEncryption);
Oid valueValue = new Oid(OidValues.RsaEncryption, OidValues.RsaEncryption);
Oid legalButWeird = new Oid(OidValues.RsaEncryption, "par-tay!");

bool decide1 = normalized.Equals(valueValue);
bool decide2 = normalized.Equals(legalButWeird);

[vcsjones]

I'm personally content to go whole-hog and make the setters be throw-if-not-my-value (or just straight throw, but that sometimes leads to bad debugger experiences).

Well, I can understand that is an even bigger break, so I’m happy regardless. Less defensive copies would be good enough, but I’ve always though Oid should be a “typed string” similar to HashAlgorithmName.

I’ll admit the friendly name never made too much sense to me, secondary display text if anything. I’d be in favor of the Oid ignoring it for equality purposes.

[terrajobst]

Video

• Looks good. It's a runtime breaking change but we believe the value is high and the risk is low (enough)

public partial class Oid
{
    public string Value
    {
        get => _value;
        set
        {
            if (_value != null && !_value.Equals(value))
                throw new PlatformNotSupportedException(...);

            _value = value;
        }
    }

    public string FriendlyName
    {
        get => _friendlyName ??= LookupFriendlyName(_value);
        set
        {
            if (_friendlyName != null && !_friendlyName.Equals(value))
                throw new PlatformNotSupportedException(...);

            _friendlyName = value;
        }
    }
}

[vcsjones]

@bartonjs are you working on this? If not, mind if I do? General thought would be to 1. Make changes 2. follow up PRs to remove defensive copies where possible.

[bartonjs]

It's currently like 6th on my todo list, so go for it. I agree with the approach of making the small PR first (just the new throw and tests verifying it), then followup PRs to stop being wasteful. (Be careful of the Pkcs library, since it ships netstandard, so it would probably want a helper method like

internal static Oid CloneOid(Oid toClone)
{
    Debug.Assert(toClone != null);

#if NETSTANDARD
    return new Oid(toClone.Value, null);
#else
    return toClone;
#endif
}

)

[ericstj]

@bartonjs can you please file a breaking change issue? https://github.com/dotnet/docs/issues/new?template=dotnet-breaking-change.md

[bartonjs]

Breaking change doc PR is dotnet/docs#19680.