Use our own JSON encoder #118
Conversation
Raven doesn't need to be fast but it HAS to work. We need control on the JSON encoding to avoid serialization issues.
|
Wasn't this fixed by #107? I much prefer allowing users to select their own adapter rather then to force them to use our vendored one. |
|
Nope, #107 defers the decision to the user which will have to discover Given that the goal of sentry is to report exception I believe that it's
|
Circular dependencies and JSON serialization don't play well together.
Depending on the library it might throw a SystemStackTooDeep or even segfault
the interpreter.
Here we replace the circular depency with a "[...]" or "{...}" string
representation that should make it clear later on what happened.
|
Ok so as a recap: commit 19d3810 + 05eff65 fixes circular references on the given input by replacing them with a string. BUT, if any adapter uses #as_json / #to_json, additional structures might get generated later on that might themselves contain circular references. This is the case for at least ActiveRecord::JSON and the json gem, maybe others. This is an issue because we cannot guarantee that the input data is clean and JSON-compatible. The rack env can contain arbitrary data and is in practice used to exchange data between various middlewares. Because we want to report and exception it is important that the JSON generation doesn't fail. On a side note, ActiveRecord defines Enumerable#as_json which coerces the data with #to_a. IO objects are Enumerable and #to_a reads the file. This means that IO objects are read completely when generating the JSON which might either lead to a big payload or an exception if the IO object is not opened for read. So in conclusion, the last two commits are probably good to have anyways. I also propose that we use our own version of OkJson that favours coercion over exception but it's probably also possible to use another external library if it satisfies our needs. I also believe that choosing one JSON generator will lead to less support issues that are hard to debug. |
|
Thanks for making the whole issue clear, @zimbatm. So, overall, for me: The json_adapter configuration setting, as shipping right now, breaks for fairly common use cases in Rails environments (as you wrote above) or other apps using ActiveRecord. Your PR fixes this by shipping a specialized adapter that prefers coercion over exception, which is unusual but appropriate for this use case (we'd rather send an incomplete exception than no exception at all). It is difficult to debug because you may not even be aware that it's happening (silent failure). So we're trading configurability (multi_json) for stability (vendored OkJson). Still haven't decided if I'm ok with this tradeoff or not - @dcramer? Others? |
|
Configurability is overrated IMO |
|
Good enough for me, then. Thanks again @zimbatm for writing such a clear explanation of the issue. |
The goal of raven is to report exceptions. By introducing our own tuned version of OkJson that coerces values to a serialisable state we can guarantee that a JSON is going to be generated.
This branch fixes #97.
It also avoids issues with ActiveRecord's #to_json/#as_json that trigger "IOError: not opened for reading" when serialising an IO (like the rack.input)