Fix Out-GridView by replacing use of obsolete BinaryFormatter with custom implementation. #25497
Conversation
…stom implementation.
| this.AvailableRules = new ValidatingSelectorValue<FilterRule>(); | ||
| foreach (DataErrorInfoValidationRule rule in source.AvailableRules.ValidationRules) | ||
| { | ||
| this.AvailableRules.AddValidationRule(rule); |
There was a problem hiding this comment.
Should we clone the rule too?
| this.AvailableRules.AddValidationRule(rule); | |
| this.AvailableRules.AddValidationRule(rule.Clone()); |
There was a problem hiding this comment.
DataErrorInfoValidationRule is stateless and not derived from FilterRule. Perhaps I should use different names for the loop variables to avoid confusion.
There was a problem hiding this comment.
Are there classes based on DataErrorInfoValidationRule?
There was a problem hiding this comment.
In any case we should create new object to follow "DeepCopy".
There was a problem hiding this comment.
Only IsNotEmptyValidationRule, which is stateless as well.
There was a problem hiding this comment.
Thanks! I see. In any case we should create new object to follow "DeepCopy" as BinaryFormatter does.
| public interface IDeepCloneable | ||
| { | ||
| /// <summary> | ||
| /// Creates a deep copy of the current instance. | ||
| /// </summary> | ||
| /// <returns>A new object that is a deep copy of the current instance.</returns> | ||
| object DeepClone(); | ||
| } |
There was a problem hiding this comment.
I'd prefer strong typed interface. It is more safe.
| public interface IDeepCloneable | |
| { | |
| /// <summary> | |
| /// Creates a deep copy of the current instance. | |
| /// </summary> | |
| /// <returns>A new object that is a deep copy of the current instance.</returns> | |
| object DeepClone(); | |
| } | |
| public interface IDeepCloneable<TSelf> | |
| where TSelf : class | |
| { | |
| /// <summary> | |
| /// Creates a deep copy of the current instance. | |
| /// </summary> | |
| /// <returns>A new object that is a deep copy of the current instance.</returns> | |
| TSelf DeepClone(); | |
| } |
There was a problem hiding this comment.
I wanted to do that initially, but then I can no longer check if a type derived from TSelf provides the interface. Example:
public abstract class FilterRule : IDeepCloneable<FilterRule> {}
public class SomeRule<T> : FilterRule {}
public class Foo<T> {
public T TypedValue { get; set; }
public object Value { get; set; }
public bool IsTypedValueDeepCloneable => typeof(IDeepCloneable<T>).IsAssignableFrom(typeof(T));
public bool IsValueDeepCloneable => Value is IDeepCloneable<T>;
}
var foo = new Foo<SomeRule<int>>();
foo.TypedValue = new SomeRule<int>();
foo.Value = new SomeRule<int>();
var oops1 = foo.IsTypedValueDeepCloneable; // wrongly returns false.
var oops2 = foo.IsValueDeepCloneable; // wrongly returns false.There was a problem hiding this comment.
Strong typed interface assumes that we have to add it to every class and override DeepClone() in every class - such specific DeepClone() should use right constructor of the class.
There was a problem hiding this comment.
FilterRule implements DeepClone() and instantiates the derived classes via copy constructor. The copy constructors are needed anyway, because initialization has to be done slightly different than the default ctor does. (what BinaryFormatter did via [OnDeserialized]).
There was a problem hiding this comment.
Here is a more comprehensive demo of the problems with the strongly typed interface, be it implemented on the base class or on every derived class. Neither works properly for all use cases.
There was a problem hiding this comment.
I understand what you want but it doesn't work in real life the scenario we are dealing with.
We have cloneable types that contain other cloneable types as properties, the exact types of which are not precisely known. The generic type of such a property may be instantiated as a base class like FilterRule or ValidatingValueBase, some derived class like TextEqualsFilterRule or ValidatingValue, or it may be some other type that doesn't implement our clone interface, like DateTime or string.
We need to check for the interface being available to decide whether to (deep)clone or do a simple copy - which is impossible with your suggestion.
There was a problem hiding this comment.
Perhaps I skip something. I believe all the work is about static FilterRule DeepCopy(this FilterRule rule) method using BinaryFormatter. So I don't understand "or it may be some other type that doesn't implement our clone interface, like DateTime or string". Can you clarify?
There was a problem hiding this comment.
The reason why the original author chose to use Binaryformatter instead of doing the work himself (presumably) is the complexity of the involved generic and non-generic types and how they are nested. If you recall, there also was an attempt at replacing Binaryformatter two years ago - which failed, because it did not fully take that complexity into account.
FilterRule is the base class of a hierarchy of ~20 classes, nested up to six levels. Some of them are generic. Their generic properties are of type ValidatingValue<T> and ValidatingSelectorValue<T>.
That type T can be an "incoming" type corresponding to the columns of the gridview, which I referred to with "some other type like string etc".
But T can also be another class derived from FilterRule like PropertyValueSelectorFilterRule<T2>, or another ValidatingValue<T3>. So you get multiple nested generics, some of which are generated dynamically at runtime via MakeGenericType().
There was a problem hiding this comment.
All the copying work is done by the new copy constructor (you have already created these constructors), which is called by a virtual method from the interface. Therefore, there should be no problems that you are talking about.
There was a problem hiding this comment.
Again:
- I cannot simply call the interface method, I first have to check if the interface exists.
- I cannot check for the interface if it is typed to its implementing type, because I don't know which type that is.
- I don't know which type that is, because the
TI have to perform a type check liketypeof(IDeepCloneable<T>)may not refer exactly to its implementing type, causing me to wrongly skip the cloning (as demoed by the gist I linked earlier).
I'm sorry I cannot explain it better, even though I spent almost more time on it than on the code itself.
Maybe ask someone else on your team to have a look at it. Otherwise, if my explanation is not sufficient, I'm closing this PR, because it will exceed my available capacity.
...oft.Management.UI.Internal/ManagementList/FilterCore/FilterRules/IsNotEmptyValidationRule.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@mawosoft Sorry for the long delay! The changes look great. Thank you very much for getting this done!
There are 4 Initialize methods declared with the [OnDeserialized] attribute. They were used with the BinaryFormatter to initialize an object right after deserialization. I think we can remove them now, what do you think?
I'm happy to go ahead remove those 4 methods if you cannot get to it any time soon :)
|
I would only remove them if we also remove the If that's okay, I can do it tomorrow. |
|
Of course, I forgot about the I will merge this PR as is, so that only the necessary changes will be backported to v7.5. I will submit a PR after merging this one to clean up the attributes. Thanks for your contribution, @mawosoft! |
|
I presume this will land in both 7.5.2 and 7.6.0-preview.5? |
|
I verified that the changes in this PR fixed #20223 too. I also smoke tested other filters, and all seem to work fine. |
…h custom implementation (PowerShell#25497)
PR Summary
Fix Out-GridView by replacing use of obsolete BinaryFormatter with custom deep-copy implementation for the filter-related classes.
Fixes #24749. Closes #14054.
PR Context
After switching to .NET 9 in Powershell 7.5, filtering in the Out-GridView window is broken, because BinaryFormatter has been removed from the runtime.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header