Migrating to System.Text.Json by xperiandri · Pull Request #176 · fsprojects/SwaggerProvider

xperiandri · 2021-01-07T20:48:31Z

As long as I don't have .NET 4.6.1 targets installed I moved to .NET 4.8, but I suppose we can stay on .NET 4.6.1 (previously it was .NET 4.6.0)

sergey-tihon · 2021-01-16T17:48:42Z

I cannot tell (yet) if we are are ready to merge it

If you like to proceed this work:
-Let's revert .NET version to net46 or net461

What's the reason to use latest FSharp.Core version? can use 4.3.4? let's use lowest possible
We need to fix CI (revert to net461 may help to build code)
Remove JSON.NET related code (at least from this branch)
Fix tests (I bet that deserializers behave differently and hope that tests reveal the difference)

# Conflicts: # paket.dependencies # paket.lock

sergey-tihon · 2021-01-17T21:21:54Z

src/SwaggerProvider.Runtime/ProvidedApiClientBase.fs

+        if isNull options then
+            let options = JsonSerializerOptions()
+            [
+                JsonFSharpConverter(


Why here used not default converters config JsonFSharpConverter()?

It only exists in .NET 5 as far as I know and it is not as powerful as FSharp.SystemtextJson

I mean, why do you use

let options = JsonSerializerOptions() [ JsonFSharpConverter( JsonUnionEncoding.InternalTag ||| JsonUnionEncoding.NamedFields ||| JsonUnionEncoding.UnwrapSingleCaseUnions ||| JsonUnionEncoding.UnwrapRecordCases ||| JsonUnionEncoding.UnwrapOption) :> JsonConverter ] |> List.iter options.Converters.Add

as a default configuration, instead of

let options = JsonSerializerOptions() [ JsonFSharpConverter() :> JsonConverter ] |> List.iter options.Converters.Add

for some reason, author of FSharp.SystemTextJson choose different default configuration for JsonFSharpConverter

We can go with the default configuration.
I don't know why he did that.
I see his defaults too verbose in JSON

…1565947/the-json-value-could-not-be-converted-to-system-byte

sergey-tihon · 2021-01-18T07:42:10Z

paket.dependencies

-nuget Newtonsoft.Json
+nuget System.Net.Http.Json
+nuget System.Text.Json
+nuget FSharp.SystemTextJson


Why do we need 3 explicitly referenced packages here?

nuget System.Net.Http.Json nuget System.Text.Json nuget FSharp.SystemTextJson

can we reference only FSharp.SystemTextJson?

I thought that we will use JSON extensions to HttpClient and HttpResponseMessage

I suppose we can

xperiandri · 2021-01-18T11:06:33Z

src/SwaggerProvider.DesignTime/v3/OperationCompiler.fs

+                    invokeCode = (fun args -> <@@ () @@>),
                    BaseConstructorCall = fun args -> (baseCtor, args))
                ProvidedConstructor(
-                    [ProvidedParameter("options", typeof<JsonSerializerOptions>, optionalValue = true)],


Do optional parameters not work?

optionalValue is not the the bool argument - see here

SDK expect default value for provided parameter here, you code means true :> JsonSerializerOptions.
You may try to fix default parameters, but it may be tricky with null as default value.

Ah, got it. I'll fix

Try this

[ ProvidedConstructor( [ProvidedParameter("httpClient", typeof<HttpClient>); ProvidedParameter("options", typeof<JsonSerializerOptions>, optionalValue = (null:JsonSerializerOptions))], invokeCode = (fun args -> match args with | [] -> failwith "Generated constructors should always pass the instance as the first argument!" | _ -> <@@ () @@>), BaseConstructorCall = fun args -> (baseCtor, args)) ProvidedConstructor( [ProvidedParameter("options", typeof<JsonSerializerOptions>, optionalValue = (null:JsonSerializerOptions))], invokeCode = (fun args -> <@@ () @@>), BaseConstructorCall = fun args -> let httpClient = <@ RuntimeHelpers.getDefaultHttpClient defaultHost @> let args' = args @ [httpClient] (baseCtor, args')) ] |> ty.AddMembers

sergey-tihon · 2021-01-20T20:29:47Z

src/SwaggerProvider.DesignTime/v2/OperationCompiler.fs

-                        let httpClient = <@@ RuntimeHelpers.getDefaultHttpClient defaultHost @@>
-                        let args' = args @ [ httpClient; <@@ null @@> ]
+                        let httpClient = <@ RuntimeHelpers.getDefaultHttpClient defaultHost @>
+                        let args' = args @ [httpClient]


This arguments order is incorrect

What is the correct one?

The one that you revered in your commit [this; httpClient; options] as base class ctor expects

8459021#diff-4b6d8e966f147a1fd45efd232f8c3d4d993b57ddb43baa435c99211e4bb3e2ddL273-L276

still no, you build [this; options; httpClient; null] instead of [this; httpClient; options]

Like this?

let httpClient = <@ RuntimeHelpers.getDefaultHttpClient defaultHost @> :> Expr let this :: tail = args let args' = this :: httpClient :: tail

…Core#1258 (comment)

sergey-tihon · 2021-01-29T20:10:35Z

@xperiandri let me know about the result of your testing of NuGet package built from this branch

xperiandri · 2021-01-29T23:40:00Z

It works. Just max version FSharp.System.Text.Json is strange

sergey-tihon · 2021-01-30T09:12:22Z

Basically it is the version that is used to build and test the package.
https://github.com/fsprojects/SwaggerProvider/blob/net5/src/SwaggerProvider.Runtime/paket.template#L33

What version you propose to use?

xperiandri · 2021-01-30T09:19:28Z

< 1.0.0 I suppose

xperiandri · 2021-01-30T11:05:54Z

Also have a look at warnings from NuGe Package Explorer

WIP migrating to System.Text.Json

68146c3

Merge branch 'net5' into 'system_text_json'

4521376

# Conflicts: # paket.dependencies # paket.lock

sergey-tihon changed the base branch from master to net5 January 17, 2021 18:19

sergey-tihon closed this Jan 17, 2021

sergey-tihon reopened this Jan 17, 2021

sergey-tihon added 3 commits January 17, 2021 19:24

ci for pr to net5

1dbc19c

naive fix for provided .ctors

ed812a1

uses JsonFSharpConverter on the server

ac94419

sergey-tihon reviewed Jan 17, 2021

View reviewed changes

fix for byte[] serialization =( https://stackoverflow.com/questions/6…

fb12c65

…1565947/the-json-value-could-not-be-converted-to-system-byte

sergey-tihon reviewed Jan 18, 2021

View reviewed changes

xperiandri commented Jan 18, 2021

View reviewed changes

sergey-tihon and others added 5 commits January 18, 2021 21:42

removed dependency on System.Net.Http.Json

cc1869a

default config for JsonFSharpConverter

88c7b3f

Fixed generation of constructors with optional parameters

8459021

Added ability to run Expecto tests from Visual Studio

1046318

Removed unnecessary reference from tests

4e002c9

xperiandri marked this pull request as ready for review January 19, 2021 22:23

sergey-tihon closed this Jan 20, 2021

sergey-tihon reopened this Jan 20, 2021

sergey-tihon reviewed Jan 20, 2021

View reviewed changes

Fixed client constructor parameters

06654ed

xperiandri force-pushed the system_text_json branch from cce1ac0 to 06654ed Compare January 21, 2021 00:35

sergey-tihon added 2 commits January 21, 2021 09:43

enable v2 & v3 schema generation - domaindrivendev/Swashbuckle.AspNet…

d30b8ca

…Core#1258 (comment)

explicit error on args mismatch

b39696f

sergey-tihon changed the title ~~WIP migrating to System.Text.Json~~ Migrating to System.Text.Json Jan 21, 2021

sergey-tihon added 2 commits January 21, 2021 09:51

enable SwaggerProvider tests

f4f8838

fix SwaggerProvider tests

61e3306

sergey-tihon merged commit bdfbd94 into fsprojects:net5 Jan 29, 2021

sergey-tihon mentioned this pull request Jan 29, 2021

.NET 6 #168

Merged

4 tasks

Conversation

xperiandri commented Jan 7, 2021

Uh oh!

sergey-tihon commented Jan 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergey-tihon Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xperiandri Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergey-tihon commented Jan 29, 2021

Uh oh!

xperiandri commented Jan 29, 2021

Uh oh!

sergey-tihon commented Jan 30, 2021

Uh oh!

xperiandri commented Jan 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xperiandri commented Jan 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sergey-tihon commented Jan 16, 2021 •

edited

Loading

sergey-tihon Jan 20, 2021 •

edited

Loading

xperiandri Jan 21, 2021 •

edited

Loading

xperiandri commented Jan 30, 2021 •

edited

Loading