search

jsonapi-rb / jsonapi-renderer

Efficiently render JSON API documents.
http://jsonapi-rb.org
MIT License
27 stars 11 forks source link

Let cache handle marshalling of results. #28

Open rovermicrover opened 6 years ago

rovermicrover commented 6 years ago

Also multi_fetch should use the splat operator. http://api.rubyonrails.org/classes/ActiveSupport/Cache/Store.html#method-i-fetch_multi

beauby commented 6 years ago

Hi @rovermicrover – thanks for the PR. Would you mind splitting this into two? (One with the splat change, and one with the marshalling)

I'll merge the splat one immediately, but I'd like to discuss a bit more about the marshalling part. The idea is that most of the time is spent transforming a hash into a JSON string, hence the JSON fragments I introduced. What's your rationale behind the change?

rovermicrover commented 6 years ago

When we turned on caching the first issue we ran into was that it was thinking the array of cache keys was one giant cache key. Thus the splat operator.

Next once that was done the JSON result had instead of data being an array of objects, it was an array of strings of escaped JSON. This was just a fix to get things working, and thought I should make a PR for feedback.

Will submit a separate PR tomorrow for the splat operator and awaiting your feedback on the other part.

beauby commented 6 years ago

The splat part was clearly a bug.

Next once that was done the JSON result had instead of data being an array of objects, it was an array of strings of escaped JSON. This was just a fix to get things working, and thought I should make a PR for feedback.

I assume you're using rails. After some investigation, it seems ActiveSupport is putting its nasty fingers inside JSON generation. I'll think of a fix.

Edit: Problem actually comes from there. Edit 2: Easy fix: use JSON.generate instead of #to_json in rails renderers (here).

beauby commented 6 years ago

@rovermicrover See my above edits. Would you mind issuing a PR with the mentioned fix?

rovermicrover commented 6 years ago

Still results in an array of escaped JSON strings.

https://github.com/rails/rails/blob/08754f12e65a9ec79633a605e986d0f1ffa4b251/activesupport/lib/active_support/json/encoding.rb#L79

Rails wraps all strings it turns into JSON in 'EscapedString's. This removes the to_json method on JSONString.

If we make JSONString not inherit from String then it will call as_json. We can't return self because then it end up in an endless loop. We could try tricking rails into thinking JSONString is a Numeric, NilClass, TrueClass, FalseClass but that just seems really hacky.

Also note marshing the objects from cache to hash to JSON takes 400 MS to go through for 250 objects. While cache to JSON takes like 300MS. Going to move forward with this PR version for for now on my project as it saves a lot of processing power even if it's not perfect. Not sure of the right solution for this.

beauby commented 6 years ago

Hi @rovermicrover – the relevant part for ActiveSupport's modification of the JSON gem behavior is there: https://github.com/rails/rails/blob/56c1326abb11ed275f04b6e0592ca66975e37f24/activesupport/lib/active_support/core_ext/object/json.rb#L23

As you can see, calling JSON.generate will prevent the ActiveSupport overrides to kick in. I did test the fix locally and it worked.

it saves a lot of processing power even if it's not perfect.

I'm glad to hear that (:

beauby commented 6 years ago

You may want to give https://github.com/jsonapi-rb/jsonapi-rails/pull/70 a try.

rovermicrover commented 6 years ago

@beauby I think the issue is we are using rails 4

beauby commented 6 years ago

Oh I see. I’ll try to come up with a rails 4 compatible fix – do not hesitate if you have any suggestions in the meantime.

Also,I’d like to setup Travis to test against rails 4 on the jsonapi-rails repo. If you have some extra free time and feel like issuing a PR for that, it would be super helpful and would get us started on the caching issue.

On Fri 17 Nov 2017 at 22:40, Andrew Rove (Rover) notifications@github.com wrote:

@beauby https://github.com/beauby I think the issue is we are using rails 4

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/jsonapi-rb/jsonapi-renderer/pull/28#issuecomment-345373005, or mute the thread https://github.com/notifications/unsubscribe-auth/ACHPYp-0dSnlgzLGtJWLypM6GEE0BN7Eks5s3f1TgaJpZM4QfcJ8 .

-- Lucas Hosseini lucas.hosseini@gmail.com