search

graylog-labs / gelf-rb

Ruby GELF library (Graylog Extended Log Format)
https://rubygems.org/gems/gelf
MIT License
153 stars 104 forks source link

Remove opportunistic loading of `yajl/json_gem` #78

Open jnraine opened 5 years ago

jnraine commented 5 years ago

Requiring yajl/json_gem causes JSON.parse, JSON.generate, #to_json, and other built-in methods to be overridden with the faster YAJL implementation. Faster is, as a rule, better so this seems like a positive change overall. However, because this change extends outside of gelf-rb and impacts any place JSON is parsed or generated using the standard library interface, it can have surprising and unexpected consequences.

When I updated gelf-rb to version 3.1.0, parts of my app started to behave differently. Specifically, U+2028 and U+2029 characters stopped being escaped within JSON strings. For a Rails app, which already overrides the standard library interface, this can be a problem [1]. (This will only impact folks who use YAJL without require 'yajl/json_gem'.)

To remedy this, yajl/json_gem is no longer required by default. This makes the optimization of JSON parsing the responsibility of the app/script/person requiring gelf-rb.

[1] http://timelessrepo.com/json-isnt-a-javascript-subset

CLAassistant commented 5 years ago

CLA assistant check
All committers have signed the CLA.

jnraine commented 5 years ago

@AlekSi does this seem like a change useful to the project? Right now, we maintain an empty file to make sure yajl/json_gem isn't loaded when graylog-rb is required. 😅

AlekSi commented 5 years ago

I have no idea, I stopped maintaining this gem (and using it, and using Ruby) years ago.

farvour commented 4 years ago

Well, perhaps it would be a good idea to merge the change? Does anyone else have the ability to merge this in? @AlekSi ? That would be nice. "I don't know, I don't maintain this" is very unhelpful and results in a bunch of useless forks getting created for a project. The impact of this change is pretty significant and I think merging this would make 3.1.x usable for a wider audience.

AlekSi commented 4 years ago

I don't have access to merge this change. I'm not a maintainer for a long time already. Sorry.

/cc @lennartkoopmann

farvour commented 4 years ago

Alexey,

Thanks. I did open up a separate issue asking who can maintain. 👌👍🏻

T

On Mar 5, 2020, at 09:15, Alexey Palazhchenko notifications@github.com wrote:

 I don't have access to merge this change. I'm not a maintainer for a long time already. Sorry.

/cc @lennartkoopmann

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.