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

Make low level client use System.text.json #4679

Merged
merged 21 commits into from
Sep 28, 2020

Conversation

Mpdreamz
Copy link
Member

@Mpdreamz Mpdreamz commented Apr 28, 2020

re-targeting #4211 to master

This introduces an IElasticsearchSerializer implementation for System.Text.Json.

TODO:

  • Remove LowLevelRequestResponseSerializer
  • Move Utf8Json to Nest
  • Make sure formatters the lowlevel client relies on are ported over
    • DynamicDictionaryFormatter

Long term plan is for Nest to move over to System.Text.Json as well but this will require significant investment in porting formatters over.

@Mpdreamz
Copy link
Member Author

Kick this off again to see why clients-ci was failing.

@russcam
Copy link
Contributor

russcam commented Aug 18, 2020

rebased against master

@russcam russcam force-pushed the feature/master/system-text-json branch from 83373e4 to 7daa640 Compare September 17, 2020 05:39
@russcam
Copy link
Contributor

russcam commented Sep 18, 2020

canary packages are currently failing to be built for this because ILRepack cannot resolve the netstandard20 System.Text.Json assembly when running the "merge" for netstandard21

[2020-09-17T05:55:41.46+00:00][Elasticsearch.Net ][AssemblyDefinition     ] finished rewriting Elasticsearch.Net into D:\a\1\s\build\output\_packages\tmp\lib\netstandard2.1\Elasticsearch.Net8.dll
INFO: IL Repack - Version 2.1.0
INFO: Adding assembly for merge: D:\a\1\s\build\output\_packages\tmp\lib\netstandard2.1\Elasticsearch.Net8.dll
INFO: Processing references
INFO: Processing types
INFO: Merging <Module>
INFO: Processing types
INFO: Processing resources
INFO: Fixing references
Mono.Cecil.AssemblyResolutionException: Failed to resolve assembly: 'System.Text.Json, Version=4.0.1.1, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'
   at Mono.Cecil.BaseAssemblyResolver.Resolve(AssemblyNameReference name, ReaderParameters parameters)
   at Mono.Cecil.DefaultAssemblyResolver.Resolve(AssemblyNameReference name, ReaderParameters pr)
   at Mono.Cecil.MetadataResolver.Resolve(TypeReference type)
   at Mono.Cecil.TypeReference.Resolve()
   at ILRepacking.ReferenceFixator.FixReferences(Collection`1 attributes)
   at ILRepacking.ReferenceFixator.FixReferences(TypeDefinition type)
   at ILRepacking.Steps.ReferencesFixStep.Perform()
   at ILRepacking.ILRepack.Repack()
   at ILRepacking.Application.Main(String[] args)

I've opened nullean/assembly-rewriter#7 to allow AssemblyRewriter to sign the rewritten versioned canary packages, which would allow ILRepack to be removed, since it is only being used to sign the assembly in the master and 7.x branches.

@russcam russcam mentioned this pull request Sep 22, 2020
@Mpdreamz Mpdreamz force-pushed the feature/master/system-text-json branch from 13b136e to 47ebd8b Compare September 22, 2020 08:39
Mpdreamz and others added 9 commits September 22, 2020 19:18
Currently part of the main distributable, possibly living in a separate
nuget package later

(cherry picked from commit 9d0427d)
(cherry picked from commit d8a188e)
…ly by relying on a default on ConnectionConfiguration.cs

(cherry picked from commit c86f255)
This commit removes the explicit reference to
System.Memory. It's a transitive dependency of
System.Text.Json, which references a higher
minimal version.
This commit adds an ExceptionConverter to serialize
exceptions with System.Text.Json in the same manner
as Utf8Json.

Do not serialize nulls with System.Text.Json, and
allow a derived serializer to change JsonSerializerOptions.
JsonSerializerOptions are lazily constructed in the ctor
because they are created by a virtual method.
@Mpdreamz Mpdreamz force-pushed the feature/master/system-text-json branch from 47ebd8b to 138bc04 Compare September 22, 2020 17:19
@Mpdreamz
Copy link
Member Author

@russcam this is starting to look good to me! Some updated on this on my end

  • clients-ci was silently failing open Fail the yaml runner if no folders are found #5024 to remedy this on all integration branches.
  • Rebased this PR, clients-ci started failing because YamlMap is Dict<object, object> which System.Text.Json does not support. Added an additional converter for it.
  • clients-ci is now failing on 1 test where the its asserting { value: 5.0 } where { value: 5 } is returned. We have some leniency if numbers are compared directly but we do not (and should not) recursively patch assertion values. This is related to the yaml test failures you identified in the rust client. We don't need to solve this on this PR IMO.
  • Noticed some assertions in PostData.doc.cs were commented out (pretty sure I did that), uncommented them again and everthing was still green.

If you have time to:

  • ignore that test for now
  • run the docs generator to fix the stale docs check

I'm happy if we merge this to master 🎉

ResponseBuilder was asking the Utf8Json serializer to deserialize to
DynamicDictonary.

This also updates DynamicValue to deal with the fact that deserializing
to `Dictionary<string, object>` does not materialize `object` these
remain `JsonElement`.
@Mpdreamz
Copy link
Member Author

ResponseBuilder was asking the Utf8Json serializer to deserialize to DynamicDictionary.
I added a new System.Text.Json converter to handle this now.

This also updates DynamicValue to deal with the fact that deserializing to Dictionary<string, object> does not materialize object these remain JsonElement.

@Mpdreamz
Copy link
Member Author

The failing integration tests relate to boost being available on some properties.

Merging this in!

@Mpdreamz Mpdreamz marked this pull request as ready for review September 28, 2020 14:24
@Mpdreamz Mpdreamz merged commit 3e0c6eb into master Sep 28, 2020
@Mpdreamz Mpdreamz deleted the feature/master/system-text-json branch September 28, 2020 14:25
@Mpdreamz
Copy link
Member Author

Will follow up with a PR to:

  • Remove LowLevelRequestResponseSerializer
  • Move Utf8Json to Nest

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.

2 participants