invio / invio.immutable Goto Github PK

Background

As of right now, there is only one way to render every single property's value on an ImmutableBase<TImmutable> inheriting class: the IStringFormatter initialized with the DefaultStringFormatter implementation. However, further down in our closed source code, there are ImmutableBase<TImmutable> implementations that store values which are sensitive - like reset tokens for emails. Those value objects that should not output their sensitive values if they are to be printed into loggers.

Therefore, I need to add the ability for an inheriting class to control which IStringFormatter is used for a given property. This can be done in a similar manner to #45.

Task

• Create two new interfaces, as follows:

  public interface IStringFormatterFactory {
      IStringFormatter Create(PropertyInfo property);
  }
  
  public interface IStringFormatterProvider {
      bool IsSupported(PropertyInfo property);
  }

• Create one implementation, DefaultStringFormatterProvider, that supports all types and does nothing but return the current DefaultStringFormatter implementation.

• Create an AggregateStringFormatterProvider that takes in a list of IStringFormatterProvider implementations and processes them in order. The first match is used to create an IStringFormatter.

• Use an IStringFormatterFactory in the ImmutableBase<TImmutable> static constructor to hydrate IStringFormatter implementations for each property instead of hard-coding all property values to use DefaultStringFormatter as it does today. It should have a public getter and setter such that others can change how properties are cached. Additionally, when it changes its value, that should cause all of the cached IStringFormatter implementations to be rebuilt using the new IStringFormatterFactory.

public static IStringFormatterFactory StringFormatterFactory { 
    get { return formatterFactory; }
    set { formatterFactory = value; ClearFormattersCache(); }
}

static ImmutableBase() {
    formatterFactory = new AggregateStringFormatterProvider(
        new DefaultStringFormatterProvider()
    );

    // ... other initialization code.
}

Background

As part of #37, we now render IEnumerable as strings in a manner similar to JSON arrays by default. However, the values within those enumerables are rendered using the default ToString() implementation for that type, rather than using what has been determined as the desired way to render that type across a parent ImmutableBase<TImmutable>. This means a DateTime property will have its values rendered one way, but a DateTime value within an IEnumerable will be rendered another way.

Task

Create an IStringFormatter concept that can either be injected into each IPropertyHandler to determine how to render each type as a string, or used directly on ImmutableBase when rendering property values via its ToString() implementation.

After an update to v1.0.1 using dotnet migrate, our code coverage tool (coveralls.io) accepts code coverage reporting files and show the correct percentage, but they no longer correctly map the file in our repositories so you can see what lines of code were actually covered.

Given this test:

        public static IEnumerable<Object[]> NullableValues { get; } =
            ImmutableList.Create(
                new Object[] { (DateTime?)null },
                new Object[] { (DateTime?)new DateTime(1934, 1, 11) });
        
        [Theory]
        [MemberData(nameof(NullableValues))]
        public void GetPropertyValueImpl_NullableProperty(DateTime? originalValue) {

            // Arrange
            
            var propertyName = nameof(Fake.NullableDateTime);
            var fake = this.NextFake().SetNullableDateTime(originalValue);

            // Act

            var value = fake.GetPropertyValue(propertyName);

            // Assert
            
            Assert.IsType<DateTime?>(value);
            Assert.Equal(originalValue, value);
        }

The following test failure occurs:

Failed   Invio.Immutable.PropertyValueImplTests.GetPropertyValueImpl_NullableProperty(originalValue: 1934-01-11T00:00:00.0000000)
Error Message:
 Assert.IsType() Failure
Expected: System.Nullable`1[[System.DateTime, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]]
Actual:   System.DateTime
Stack Trace:
   at Invio.Immutable.PropertyValueImplTests.GetPropertyValueImpl_NullableProperty(Nullable`1 originalValue) in /test/Invio.Immutable.Tests/PropertyValueImplTests.cs:line 208
Failed   Invio.Immutable.PropertyValueImplTests.GetPropertyValueImpl_NullableProperty(originalValue: null)
Error Message:
 Assert.IsType() Failure
Expected: System.Nullable`1[[System.DateTime, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]]
Actual:   (null)
Stack Trace:
   at Invio.Immutable.PropertyValueImplTests.GetPropertyValueImpl_NullableProperty(Nullable`1 originalValue) in /test/Invio.Immutable.Tests/PropertyValueImplTests.cs:line 208

Background
When I created this library with a 1.01 version of the SDK I set the DebugType to full and updated the CI scripts to explicitly reference XML-based *.csproj files due to the removal of global.json and project.json. The DebugType really should be portable for .NET Core when we pack the code (though it needs to be full for when we run opencover on it), and we should add a *.sln to manage our *.csproj files.

Task

• Update the repository to have a Invio.Immutable.sln solution at the root of the repository that references the source and test projects.
• Update Invio.Immutable.csproj such that <IsPackable>false</IsPackable> is specified.
• Restore set-debug-type.ps1, but with it updating the DebugType via the XML-based Invio.Immutable.csproj instead of JSON-based project.json.

Background

As of right now, when someone wants to create a setter that updates two properties simultaneously on ImmutableBase<TImmutable>, they have to create their own setter that manually invokes the constructor or invokes SetPropertyValueImpl twice. This is because SetPropertyValueImpl only allows one property to be set at a time.

This is problematic for two reasons:

1. It is an unnecessary performance hit because you have to create two instances even though you knew the value of each property before the operation began.
2. If two properties are coupled and have argument validation perform in the constructor, you cannot update them both simultaneously so they avoid a conflicting state.

The latter is particularly frustrating. Consider that you have, for example, aMinimum and Maximum property on the immutable class. In your constructor, you throw an ArgumentException if Minimum is greater than Maximum. This means if you want to change the range of Minimum and Maximum from 1 to 5, to 6 to 10, or -5 to -1, you have to know which order to set the values in, adding complexity to what should be a simple setter.

Adding the ability to set both of these properties at the same time removes this complexity.

Task

Add a SetPropertyValuesImpl implementation that takes in a IDictionary<string, object> that maps property names to property values so they can be assigned simultaneously, like so:

protected SetPropertyValuesImpl(IDictionary<string, object> propertyValues);

Caveats:

• Support case insensitivity in property names
• Throw on same property name listed twice (which could happen in a case-sensitive dictionary)
• Throw on non-existent property name
• Throw on incompatible property values for given property type.

Background

The primary workflow for the ImmutableBase<T> is for it to be a bag of properties with a lot of boilerplate implementations for equality checks, hash code generation, to string representations, and the getting or setting of property values. It does this fine.

What it does not do fine is if someone wants to expose a read-only property that is generated from a collection of other properties. For example, in Invio.Validation.OperationResult, we have a IsSuccessful property that checks if there are any error-level validation issues. This is not a property that is part of the property bag - it is a helper property that is a derivative of other pieces of data.

Based upon the patterns of how we use this data structure, it is the auto-implemented properties that we should be detecting automatically as properties managed by ImmutableBase<TImmutable> - not all properties.

Task

Update our property identification implementation to only include auto-implemented properties by default.

Note

Out of curiosity, I asked on StackOverflow if there was a way to do this. My question was (correctly) marked as a duplicate and was redirected here. This isn't an exact science, but it is worth some tests that can and should be ran on different platforms.

When attempting a deployment of Invio.Immutable by tagging the a particular commit, we get the message as follows from appveyor:

Collecting artifacts...
No artifacts found matching 'artifacts\**\*.nupkg' path
Deploying using NuGet provider
No packages were pushed.
Build success

Here's an example.

I need to re-enable automatic deployments for tagged commits to master.

The Invio.Immutable library is the only one library using v1.0.1 of the dotnet CLI. However, we already have other repositories using the v1.0.4 version of the SDK. Let's get Invio.Immutable standardized on the latest version supported by our CI tools, which is v1.0.4.

Background

Right now, all of the ways that IPropertyHandler implementations can be defined is hard-coded into the static constructor of ImmutableBase<TImmutable>. This sucks for two big reasons:

1. If someone wants to add a custom IPropertyHandler implementation, the have to do it with hackery and reflection. It is essentially non-extensible.
2. If we add any complexity on how an IPropertyHandler is to be constructed (like, for example, as defined in #12 and #14), that logic gets stuck inside of ImmutableBase<TImmutable> when it could have its complexity compartmentalized.

Both of these could be solved with a few interfaces with default implementations.

Task

• Create two new interfaces, as follows:

  public interface IPropertyHandlerFactory {
      IPropertyHandler Create(PropertyInfo property);
  }
  
  public interface IPropertyHandlerProvider : IPropertyHandlerFactory {
      bool IsSupported(PropertyInfo propertyInfo);
  }

• Create three implementations, StringPropertyHandlerProvider, EnumerablePropertyHandlerProvider (for Set & List) and DefaultPropertyHandlerProvider, that inspect the relevant PropertyInfo as we do in ImmutableBase<TImmutable> static constructor today to determine support and desired approach to hydration.

• Create an AggregatePropertyHandlerProvider that takes in a list of IPropertyHandlerProvider implementations and processes them in order. The first match is used to create an IPropertyHandler.

• Use a IPropertyHandlerFactory in the ImmutableBase<TImmutable> static constructor to hydrate IPropertyHandler implementations instead of doing it all in a hard-coded fashion as it does today. It should have a public getter and setter such that others can change how properties are cached. Additionally, when it changes its value, that should cause all of the cached IPropertyHandler implementations to be rebuilt using the new IPropertyHandlerFactory.

Assuming all of these changes went through, I would expect the static constructor for ImmutableBase<TImmutable> to start to look something like this:

public static IPropertyHandlerFactory PropertyHandlerFactory { 
    get { return handlerFactory; }
    set { handlerFactory = value; ClearHandlersCache(); }
}

static ImmutableBase() {
    handlerFactory = new AggregatePropertyHandlerProvider(
        new StringPropertyHandlerProvider(),
        new EnumerablePropertyHandlerProvider(),
        new DefaultPropertyHandlerProvider()
    );

    // ... other initialization code.
}

Then, when we implement #12 and #14, we can keep that logic in the IPropertyHandlerProvider implementations instead of keeping it centrally located in the ImmutableBase<TImmutable> static constructor.

It is common for strings to be compared by ignoring case or performing ordinal comparisons versus culture-specific ones. However, you cannot do this type of comparison with ImmutableBase<TImmutable>. You're currently stuck with the default string comparison ordinal and case-sensitive. While I think complex comparisons, such as those where two properties need to be compared to determine equality, should be implemented by the developer, this is a common enough comparison that it warrants first-class support.

It would be done via something like this:

public class MyImmutable : ImmutableBase<MyImmutable> {

    public String UseDefaultComparison { get; }

    [StringComparisonAttribute(StringComparison.OrdinalIgnoreCase)]
    public String UseOrdinalIgnoreCaseComparison { get; }

    public MyImmutable(
        String useDefaultComparison,
        String useOrdinalIgnoreCaseComparison) {

        this.UseDefaultComparison = useDefaultComparison;
        this.UseOrdinalIgnoreCaseComparison = useOrdinalIgnoreCaseComparison;
    }

}

Give the above example, I would expect values in the UseOrdinalIgnoreCaseComparison property to be compared without regard to case, and values in the UseDefaultComparison property to be compared with regard to case. Here are some tests that codify these assumptions:

public class MyImmutableTests {

    [Fact]
    public void IgnoreCase() {
        var left = new MyImmutable("foo", "bar");
        var right = new MyImmutable("foo", "BAR");

        Assert.Equal(left, right);
    }

    [Fact]
    public void CaseSensitiveByDefault() {
        var left = new MyImmutable("foo", "bar");
        var right = new MyImmutable("FOO", "bar");

        Assert.NotEqual(left, right);
    }

}

The way we determine if two IEnumerable-typed properties are equal is by the IEnumerable implementation itself. Unfortunately, the standard implementation of this in the .NET Framework's System.Collections.* packages is referential equality. This means two IEnumerable implementations are determined to be equal only if they are the same object in memory - even if they contain the same items in the same order.

We need to update the ImmutableBase<TBase> implementation to detect IEnumerable properties on our immutable data objects, and, as we find them, perform one of the following two methodologies when determining if the values for those properties are equal:

• If the property is assignable to the type ISet<TItem>, the values within it should be compared without consideration their index. This is because sets, by their very definition, care about the inclusion and exclusion of values, not the order or frequency of individual values.
• If the property is assignable to the type IEnumerable but not to the type ISet<TItem>, the items within the IEnumerable should be compared with consideration of their order.

I believe we will eventually need to give the ability for consumers to define whether or not order matters for an individual property (for example, System.Collections.Generic.SortedSet<T> sits squarely in the middle, not allowing repeat values yet maintains order), which we can solve with something like a [UseComparison] attribute in the future. For now, these out-of-the-box equality definitions that do the needful.

Background

As a result of #1, I added some logic that changed how hash code and equality checks were done for types that implement IEnumerable. They are as follows:

• If the type for a given property implements ISet<T>, we perform an order agnostic comparison of all items in the collection.
• If the type otherwise implements IEnumerable and is not a string, we perform an order-specific comparison of all items in the collection.
• Finally, if the type is String use the default String comparison of Ordinal.

While these are reasonable defaults, there is no guarantee a user wishes this behavior to happen all of the time. For example, a property of type SortedSet<T> may care about order, despite implementing ISet<T>, or a property may be of a type that implements IEnumerable, but the instead should use the standard equality and hash code generation implementations. We need to support these one-off cases to override the default behavior in ImmutableBase<TImmutable>.

Task

Update ImmutableBase<TImmutable> to look for and respect an [EnumerableComparison] like attribute that can be applied to types of IEnumerable, letting the comparison logic know how that property is meant to have its hash code and equality calculated. For example, it could look like this:

public class CustomEnumerable : ImmutableBase<CustomEnumerable>, IEnumerable {

    public String Key { get; }
    public IEnumerable Values { get; }

    public CustomEnumerable(String key, IEnumerable values) {
        if (values == null) {
            throw new ArgumentNullException(nameof(values));
        }

        this.Key = key;
        this.Values = values;
    }

    public IEnumerator GetEnumerator() {
        return this.Values.GetEnumerator();
    }

}

public class MyImmutable : ImmutableBase<MyImmutable> {

    [EnumerableComparisonAttribute(EnumerableComparison.AsSet)]
    public IEnumerable<Guid> Ids { get; }

    [EnumerableComparisonAttribute(EnumerableComparison.Default)]
    public CustomEnumerable Custom { get; }

    public MyImmutable(ISet<Guid> ids, CustomEnumerable custom) {
        this.Ids = ids;
        this.Custom = custom;
    }

}

public class MyImmutableTests {

    [Fact]
    public void InequalityForIds() {

        // Arrange

        var idsOne = new [] { Guid.NewGuid(), Guid.NewGuid() };
        var idsTwo = new [] { idsOne[1], idsOne[0] };

        var immutableOne = new MyImmutable(idsOne, null);
        var immutableTwo = new MyImmutable(idsTwo, null);

        // Act

        var areEqual = immutableOne.Equals(immutableTwo);

        // Assert

        Assert.True(
            areEqual,
            "This would be 'false' with current logic, " +
            "since the default comparison for IEnumerable considers order."
        );
    }

    [Fact]
    public void InequalityForCustomEnumerable() {

        // Arrange

        var customOne = new CustomEnumerable("one", new [] { "foo", "bar" });
        var customTwo = new CustomEnumerable("two", new [] { "foo", "bar" });

        var immutableOne = new MyImmutable(new [] { Guid.NewGuid() }, customOne);
        var immutableTwo = new MyImmutable(new [] { Guid.NewGuid() }, customTwo);

        // Act

        var areEqual = immutableOne.Equals(immutableTwo);

        // Assert

        Assert.False(
            areEqual,
            "This would be 'true' with current logic, " +
            "since CustomEnumerable implements IEnumerable."
        );
    }

}

Note

If the application of an [EnumerableComparison] attribute changes an IEnumerable implementation to instead use the standard hash code and equality logic, the ToString() logic referenced in #13 should respect this as well.

Background

As a result of #1, the default comparison behavior for non-string IEnumerable properties is to compare the items in the enumerable rather than use referential equality. However, this behavior does not carry over to the default ToString() implementations. If we unwrap an enumerable to for checks of equality and hash code generation, we should certainly unwrap that enumerable in the default ToString() implementation.

Task

Update the ToString() implementation on ImmutableBase<TImmutable> to create array-looking string representations of properties treated as enumerables, like so:

public class MyImmutable : ImmutableBase<MyImmutable> {

    public int Number { get; }
    public IEnumerable<String> EnumerableProperty { get; }

    public MyImmutable(int number, IEnumerable<String> enumerableProperty) {
        this.Number = number;
        this.EnumerableProperty = enumerableProperty;
    }
}

public class MyImmutableTests {

    [Test]
    public void ToStringUnwraps() {
 
        // Arrange

        var immutable = new MyImmutable(5, new [] { "biz", "buzz" });

        // Act

        var output = immutable.ToString();

        // Assert

        Assert.Equal("{ Number: 5, EnumerableProperty: [ biz, buzz ] }", output);
    }

}

Background

The way setters work with ImmutableBase<TImmutable> works is pretty simple. Every time a value is to be changed for a given property, the code takes that value along with all of the other current values for all of the other properties, passes it to the class's constructor and returns the new object. However, we currently assume there is only one constructor. If you want to have several constructors, ImmutableBase<TImmutable> will need a hint as to which constructor is supposed to be called to create the new version of the caller's data. This can be especially useful if you want consumers of your immutable object to only be able to set specific values when initialing hydrating objects of this type.

Tasks

• Add an [ImmutableSetterConstructor] attribute that is applied to a constructor that is intended for use with calls to ImmutableBase<TImmutable>.SetPropertyValueImpl().
• Update the static constructor such that it looks for the [ImmutableSetterConstructor] attribute on all available constructors. If found, it uses that constructor for setting. If not, it looks to see if there is a single constructor. If there is more than one constructor, it throws an InvalidOperationException() stating the need for a single constructor or an explicit [ImmutableSetterConstructor]-tagged constructor. This exception could be thrown in the static constructor or when SetPropertyValueImpl is called for the first time.

Background

When we see an IEnumerable-based property on an ImmutableBase<TImmutable> class that implements ISet<> in some way, we ignore order when determining its equality and hash code. This is because the very definition of a set states that "the order in which the elements of a set are listed is irrelevant."

The branch for these types of properties can be seen here, which kicks in set-based property comparisons here.

However, this implementation fails to do the needful. There is an immutable implementation of sets in the .NET Framework, IImmutableSet<T>, that does not implement ISet<T>. This means it will use the standard list-based comparison within ImmutableBase<TImmutable>, which results in the order of the elements being taken into account when determining equality and hash code values for those properties.

Task

• Update the ImmutableBase<TImmutable> implementation to also also check for IImmutableSet<> in addition to ISet<> when performing these types of comparisons.
• Update and/or refactor CreateSetEqualsFunc() implementation to also work for IImmutableSet<T>, not just ISet<>.

Background

Classes that inherit from ImmutableBase<TImmutable> are nothing but glorified property bags which leverage reflection and common sense to manage the data stored in those properties. Given that they act like value types, it makes sense for us to implement the == and != operator overloads so that those operators are identical to calls to bool Equals(TImmutable other), as that is how traditional value types (such as String, int, and Guid) behave.

Task

Add == and != operator overloads to ImmutableBase<TImmutable>.

Background

There may be properties on the ImmutableBase<TImmutable> that are not storing relevant data, but instead represent a local caches or syntactic sugar that should actually be ignored when determining the immutable value object's equality, hash code, or string representation.

We should create a [Calculated] attribute that we have consumers flag these properties with so the ImmutableBase<TImmutable> can ignore them.

Task

Add a [Caculated] attribute that ImmutableBase<TImmutable> will look for on all of its child properties and, when found, ignore that property for the purposes of setting it, determining equality, hash code, or string representation. It could, however, be potentially fetched via the GetPropertyValueImpl method.

Background

As of #1, if a non-ISet<T> IEnumerable property is found on an ImmutableBase<T> class is found, we compare each item in the enumerables item by item to determine equality. Unfortunately, if you find two items that are null at the same index, we throw a NullReferenceException here instead of determining they are equal.

Task

Fix null/null item comparison when comparing items here.

When a child class calls ImmutableBase<TChild>.SetPropertyValueImpl and provides a property name that does not exist, we should throw an ArgumentException stating as much.

Since ImmutableBase<TImmutable> is intended to be the base object for a collection of values, the default ToString() implementation should print out those values in a legible format. Maybe JSON-like?