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

Be explicit that the returned object should be JSON #259

Merged
merged 2 commits into from
May 14, 2019

Conversation

dbrattli
Copy link
Contributor

Got a bug in a private repo that took me forever to debug down through Thoth not wanting to run the extra encoders/decoders given to the ThothSerializer. The error I got was:

09:24:47 [Error] () Connection id ""0HLMNEG9T1D5V"", Request id ""0HLMNEG9T1D5V:00000001"": An unhandled exception was thrown by the application.
System.AggregateException: One or more errors occurred. (Could not determine JSON object type for type Shared.Model+Msg+HomeMsg.) ---> System.ArgumentException: Could not determine JSON object type for type Shared.Model+Msg+HomeMsg.
   at Newtonsoft.Json.Linq.JValue.GetValueType(Nullable`1 current, Object value)
   at Thoth.Json.Net.Encode.autoEncoder@454-14.Invoke(Object v)
   at Thoth.Json.Giraffe.ThothSerializer.Giraffe-Serialization-Json-IJsonSerializer-SerializeToBytes[T](T o)
   at Giraffe.ResponseWriters.HttpContext.WriteJsonAsync[T](HttpContext this, T dataObj) in C:\projects\giraffe\src\Giraffe\ResponseWriters.fs:line 137
   at Server.webApp@72-10.Invoke(FSharpOption`1 _arg2) in /Users/dbrattli/Developer/GitHub/xyz/src/Server/Server.fs:line 74
   at FSharp.Control.Tasks.TaskBuilder.StepStateMachine`1.nextAwaitable() in C:\Users\humbo\source\repos\TaskBuilder.fs\TaskBuilder.fs:line 38

The fix was to be explicit and use the json function e.g:

return! Successful.ok (json msg) next ctx

instead of the current:

return! Successful.OK msg next ctx

I propose this fix so that other developers that start off with the SAFE template will not get the same error when they extend their app with more complex types that eventually will make the handler fail with the bug above. The Saturn template btw already uses the json function for returning the object, so this fix should make the Giraffe and Saturn templates equivalent with regards to JSON handling.

Got a bug in a private repo that took me forever to debug down though Thoth not wanting to run the extra encoders/decoders given to the `ThothSerializer`. The error I got was:

```txt
09:24:47 [Error] () Connection id ""0HLMNEG9T1D5V"", Request id ""0HLMNEG9T1D5V:00000001"": An unhandled exception was thrown by the application.
System.AggregateException: One or more errors occurred. (Could not determine JSON object type for type Shared.Model+Msg+HomeMsg.) ---> System.ArgumentException: Could not determine JSON object type for type Shared.Model+Msg+HomeMsg.
   at Newtonsoft.Json.Linq.JValue.GetValueType(Nullable`1 current, Object value)
   at Thoth.Json.Net.Encode.autoEncoder@454-14.Invoke(Object v)
   at Thoth.Json.Giraffe.ThothSerializer.Giraffe-Serialization-Json-IJsonSerializer-SerializeToBytes[T](T o)
   at Giraffe.ResponseWriters.HttpContext.WriteJsonAsync[T](HttpContext this, T dataObj) in C:\projects\giraffe\src\Giraffe\ResponseWriters.fs:line 137
   at Server.webApp@72-10.Invoke(FSharpOption`1 _arg2) in /Users/dbrattli/Developer/GitHub/xyz/src/Server/Server.fs:line 74
   at FSharp.Control.Tasks.TaskBuilder.StepStateMachine`1.nextAwaitable() in C:\Users\humbo\source\repos\TaskBuilder.fs\TaskBuilder.fs:line 38
```

The fix was to be explicit and use the `json` function e.g:

```fs
return! Successful.ok (json msg) next ctx
```
instead of the current:

```fs
return! Successful.OK msg next ctx
````

I propose this fix so that other developers that start off with the SAFE template will not get the same error when they extend their app with more complex types that eventually will make the handler fail with the bug above. The Saturn template btw already uses the `json` function for returning the object, so this fix should make the Giraffe and Saturn templates equivalent with regards to JSON handling.
@theimowski
Copy link
Member

/cc @isaacabraham is that the same change you made a while ago for Saturn?

@dbrattli
Copy link
Contributor Author

I guess it's OK to simplify this to:

return! json counter next ctx

Should I update the PR?

@isaacabraham
Copy link
Member

@theimowski yes that sounds familiar - we should use json and not ok - json counter next ctx should be all we need.

@theimowski theimowski merged commit 239f3db into SAFE-Stack:master May 14, 2019
@theimowski
Copy link
Member

Thanks! released in 1.5.1

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.

3 participants