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

"Optimized" implementations of ActiveSupport methods not used with render json: ... #913

Open
KJTsanaktsidis opened this issue Feb 1, 2024 · 0 comments

Comments

@KJTsanaktsidis
Copy link
Contributor

I believe it's a fairly common idiom in Rails to return JSON by writing render json: some_object in a controller. That causes Rails to call some_object.to_json, and thus produce a json string to return.

Calling #to_json like this invokes Oj::Rails::Encoder when Oj.optimize_rails has been set up, which is what we want. However, the problem is that Rails actually calls to_json with a non-nil, non-empty options hash (it seems to have several actionview related keys like :prefixes, :template, and :layout, from my observation). Oj realises that it was called with options it doesn't understand, and thus saves those options here and forces a real call to the object's #as_json method with them here (because argc > 0. That meas real ActiveSupport Hash#as_json gets called, which actually makes a recursive duplicate of the hash - very expensive and not nescessary!

In our codebase, I'm doing something like the following to strip out these options in the call to to_json, and ensure that Oj can use the optimized, inlined implementations of those methods:

  module JSONActionControllerRenderer
    ACTIONVIEW_RENDERER_KEYS = Set.new(%i[prefixes template layout]).freeze

    RENDER_FUNC = ->(json, options) do
      # Make sure we continue handling JSONP callbacks - the renderer needs this option, but Oj doesn't.
      callback = options.delete(:callback)
      # This bit makes the options go away, and Oj fast, if there are no options we don't understand.
      options = nil if options.all? { |key, _value| ACTIONVIEW_RENDERER_KEYS.include?(key) }
      json = json.to_json(options) unless json.is_a?(String)

      if callback.present?
        if media_type.nil? || media_type == Mime[:json]
          self.content_type = Mime[:js]
        end

        "/**/#{callback}(#{json})"
      else
        self.content_type = Mime[:json] if media_type.nil?
        json
      end
    end

    # Called from config/initializers/json.rb to register the renderer
    def self.register!
      ActionController.remove_renderer :json
      ActionController.add_renderer :json, &RENDER_FUNC
    end
  end

It would be good if this problem was solved "out of the box" somehow in Oj though. Options include:

  • Ship a custom ActionController renderer for :json just like the above snippet in Oj and enable it with Oj.optimize_rails
  • Make Oj::Rails::Encoder detect the options :template, :prefixes, and :layout explicitly, and strip them out
  • Suggest to the Rails people that these ActionView related options shouldn't be passed in to non-template renderers like :json, :xml & friends?

Do we think this is something we can solve in Oj?

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

No branches or pull requests

1 participant