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

Enable serialization IEnumerable<T> and IReadOnlyDictionary<T> descendants that have constructor with relevant collection #1187

Merged

Conversation

Alezy80
Copy link
Contributor

@Alezy80 Alezy80 commented Feb 16, 2021

Is your feature request related to a problem? Please describe.

I have heavy usage of IReadOnlyCollection and IReadOnlyDictionary descendants in my code and confused, why I can serialize (IReadOnlyCollection<int>)myCollection, but can't serialize myCollection itself? And also I can't deserialize some collections, because they doesn't have ICollection<T> interface (which is overengineered).

Describe the solution you'd like.

I think, that collections that have constructor with parameter of IEnumerable<T> or its descendants must be serialized/deserialized from out the box. Also any IReadOnlyDictionary object having constructor with parameter of IDictionary/IReadOnlyDictionary must be supported.

Describe alternatives you've considered

I can write formatters for each collection, but that's has some issues:

  1. I need to write some boilerplate code by hand, which is not fun.
  2. It tends to reference MessagePack by DTO library. I don't want that.

@Alezy80
Copy link
Contributor Author

Alezy80 commented Mar 15, 2021

Can anyone review this, or reject if the idea is weird?

Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

This looks good. I am a bit concerned because a class that happens to implement a collection interface may not be simply a collection, but it is in the last checks of the dynamic resolver, so risk of regression seems minimal.

{
ParameterInfo[] parameters = constructor.GetParameters();
if (parameters.Length == 1 &&
allowedParameterTypes.Any(allowedType => allowedType.IsAssignableFrom(parameters[0].ParameterType)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you have the IsAssignableFrom backwards. If the ctor allowed MyCustomDictionary, then IDictionary<K,V> may be assignable from it, but ultimately our Activator.CreateInstance call would fail because our Dictionary<K,V> object cannot be assigned to MyCustomDictionary

If you swap it to be:

if (parameters.Length == 1 && parameters[0].ParameterType.IsAssignableFrom(typeof(Dictionary<,>).MakeGenericType(keyType, valueType))

Then we know definitively that the ultimate invocation should succeed, and all the 3 types you were previously checking individually will all just work.

foreach (var constructor in ti.DeclaredConstructors)
{
var parameters = constructor.GetParameters();
if (parameters.Length == 1 && paramInterface.IsAssignableFrom(parameters[0].ParameterType))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here: it's backwards.

@AArnott AArnott requested a review from neuecc March 19, 2021 22:51
@AArnott AArnott changed the base branch from master to develop March 19, 2021 22:51
@AArnott AArnott added this to the v2.3 milestone Mar 19, 2021
Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

Looks good. Although detecting IEnumerable<T> in an arbitrary object could lead to false matches, also matching on the constructor signature as you're doing seems like it will lead to a high match. And if it ever misses, the user can always specify the formatter to use.

Copy link
Member

@neuecc neuecc left a comment

Choose a reason for hiding this comment

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

Thank you, I think it's good.

@AArnott AArnott merged commit a77187e into MessagePack-CSharp:develop May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants