Fix oj/rails conflict #166
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rohitpaulk
force-pushed
the
rohitpaulk/oj-rails-conflict-test-cases
branch
from
cf487e1 to
5d88c4d
Compare
rohitpaulk
changed the title
rohitpaulk
force-pushed
the
rohitpaulk/oj-rails-conflict-test-cases
branch
2 times, most recently
from
df7fa15 to
9db3cc4
Compare
This comment has been minimized.
This comment has been minimized.
rohitpaulk
force-pushed
the
rohitpaulk/oj-rails-conflict-test-cases
branch
from
9db3cc4 to
13b75bc
Compare
rohitpaulk
force-pushed
the
rohitpaulk/oj-rails-conflict-test-cases
branch
from
13b75bc to
220011d
Compare
f2prateek
approved these changes
Jun 13, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #161
This bug only occurs when both
ojandrailsare loaded. The code works fine with eitherojorrails. I've added tests for these conflicts, they run in sub-processes so that they're isolated.The issue here is that a custom
to_jsondefinition isn't honored when objects are in an array. The 'ideal' fix for this must be applied in eitherojorrails. After a bit of experimentation, I think the simplest solution on our end is to just circumvent the problem. This PR removes theMessageclass so that we're relying solely on Ruby core classes for JSON generation. This way, if eitherojorrails(or any other gem that monkey patches JSON generation) misbehave, it'll be a widespread problem (affects eitherArrayorHash) and not just local to our use case.One downside here is that we'll be generating json for each message twice - once for checking the size, and another time when actually sending the message to segment. This wasn't the case with the earlier solution because our custom
to_jsondefinition cached this value. I think this is acceptable though, it isn't a huge penalty and working around it wouldn't be trivial.