Skip to content

Collect MemberInfo same as Json.NET when building a formatter #5009

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

Merged
merged 7 commits into from
Sep 17, 2020

Conversation

russcam
Copy link
Contributor

@russcam russcam commented Sep 11, 2020

This PR changes how type property MemberInfo are collected when using MetaType to IL generate the formatter for a type. This change brings collection with Utf8Json in line with how Json.NET collects properties in

https://github.com/JamesNK/Newtonsoft.Json/blob/666d9760719e5ec5b2a50046f7dbd6a1267c01c6/Src/Newtonsoft.Json/Utilities/ReflectionUtils.cs#L928-L957

In addition, remove the GetMember call when resolving MemberInfo for a type, whilst creating the property mapping. This aligns 7.x implementation with 6.x and 5.x

Fixes #4958

@russcam
Copy link
Contributor Author

russcam commented Sep 11, 2020

I will refactor this further and provide additional documentation to clarify how it works

@russcam russcam force-pushed the property-serialization branch 2 times, most recently from d537c5c to 6f34e4b Compare September 14, 2020 06:25
This commit removes the GetMember call when resolving MemberInfo
for a type, whilst creating the property mapping. This aligns 7.x
implementation with 6.x and 5.x
This commit changes how type property MemberInfo are collected
when using MetaType to IL generate the formatter for a type. This change
brings collection with Utf8Json in line with how Json.NET collects
properties in

https://github.com/JamesNK/Newtonsoft.Json/blob/666d9760719e5ec5b2a50046f7dbd6a1267c01c6/Src/Newtonsoft.Json/Utilities/ReflectionUtils.cs#L928-L957

Fixes #4958
This commit removes the use of TypeInfo from the projects.
TypeInfo was introduced when full reflection was not available
in .NET Core and the libraries supported NetStandard 1.3.

Now that the minimum TFMs supported are .NET 4.6.1 and
.NET Standard 2.0, there's no need to use TypeInfo.
@russcam russcam force-pushed the property-serialization branch from 6f34e4b to 4bab3cd Compare September 14, 2020 07:39
@russcam russcam marked this pull request as ready for review September 15, 2020 05:13
@russcam
Copy link
Contributor Author

russcam commented Sep 15, 2020

This is now ready for review

@russcam russcam requested a review from Mpdreamz September 15, 2020 05:13

// requires lock on mono environment. see: https://github.com/neuecc/MessagePack-CSharp/issues/161
// requires lock on mono environment. see: https://github.com/neuecc/MessagePack-CSharp/issues/161
Copy link
Member

Choose a reason for hiding this comment

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

Should we reformat the files as well?

Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

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

Awesome! LGTM 👍

This commit removes Utf8Json types that are not used in the client,
and formats the remaining code to align with the conventions
of the codebase. Remove the .editorconfig in the Utf8Json
directory
This commit aligns the implementation for retrieving properties when
determining property mapping for a type with the way that properties
are retrieved when generating a serialization formatter for a type,
in relation to how member hiding and overridden members are determined.

Replace usages of GetGetMethod() with the GetMethod property and
GetSetMethod() with the SetMethod property.
This commit adds documentation for how to model
documents in Elasticsearch with types in a .NET
project. It aims to show some of the nuances
around serialization in relation to type hierarchies.
@russcam
Copy link
Contributor Author

russcam commented Sep 17, 2020

@elasticmachine run elasticsearch-ci/docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DefaultMappingFor<T> not working for DeleteByQueryAsync<T>
2 participants