ActionText::RichText attempts to render attachments when creating a new object

adamdebono commented 3 years ago

A feature of my application is automatic translation of user generated content, which is captured using ActionText's rich text fields. In order to achieve this, we essentially take the raw body of the ActionText::RichText, run it through a translation service, and then assign that content to a new object.

One (intentional) side effect of this is that any attachments are now shared across the rich text objects. After looking through rails source code, it looks like the ActionText::Content renders the content to html before storing it in the database.

https://github.com/rails/rails/commit/614e813161ad8f6e13ad19aedd577acdd976d9d6#diff-2845a2dd736db0371741dbae62c17e7fd997c7df8e66596020cff068f456854aL91 This particular commit has changed the renderer, which shouldn't cause an issue, except for the fact that I have a custom view for active_storage/blobs/_blob, which includes custom helpers. Since the renderer is not part of application controller anymore, I get an error when saving these objects.

This issue doesn't occur when creating/editing objects from the browser, only when copying these objects from within by background jobs.

Steps to reproduce

Create a custom blob.html:

<%# views/active_storage/blobs/_blob.html.erb %>
<figure class="attachment attachment--<%= blob.representable? ? "preview" : "file" %> attachment--<%= blob.filename.extension %>">
  <%= custom_image_helper(blob) %>
</figure>

Create document classes:

class Document < ApplicationRecord
  has_rich_text :content
end
class DocumentTranslation
  belongs_to :document
  has_rich_text :content
end

Attempt to copy the rich text object in a background job:

class DocumentTranlateJob < ApplicationJob
  def perform document
    translation = DocumentTranslation.find_or_initialize_by(document: document, language: 'es')
    translation.content = TranslationService.translate(document.content)
    translation.save!
  end
end

You can also replicate this in a rails console:

blob = ActiveStorage::Blob.create_and_upload!(io: File.open('path/to/a/file'), filename: 'filename')
attachment = ActionText::Attachment.from_attachable(blob)
document = Document.create!(content: "<div>Sample content with attachment</div><div>#{attachment.to_html}</div>")

Expected behavior

Making inserts or edits to rich text content should not attempt to render attachments.

Actual behavior

Making inserts or edits to rich text content attempts to render attachments which causes my custom blob view to fail.

System configuration

Rails version: 6.1.0

Ruby version: 2.7.1

abhaynikam commented 3 years ago

@adamdebono Tried to reproduce the issue on Rails 6.1. But since we don't know what custom_image_helper is doing. It is hard to reproduce the actually issue you are facing.

If possible could you create a sample application that reproduces this issue?

jonathanhefner commented 3 years ago

@georgeclaghorn Building on https://github.com/rails/rails/pull/40222#discussion_r487454184, what are your thoughts on extracting with_renderer and the accompanying around_action to ActionController proper? Then we could make a best effort to set default_renderer = ApplicationController.renderer via an inherited hook on ActionController::Base, or just expose with_renderer as a public API.

adamdebono commented 3 years ago

I've made a sample rails project that reproduces the issue with a test https://github.com/adamdebono/rich_text_rendering

One thing which might look a bit odd - I've put the helper inside lib and included it manually. This is because in my actual project the helper method I'm trying to call is in a gem, which doesn't automatically include the helper into action controller.

In this case, I'm guessing using ApplicationController.renderer would fix the issue, since I'm including the helper in there, however I don't think that there even needs to be a render call for my test case.

The stack trace from my spec is as follows:

ActionView::Template::Error:
  undefined method `custom_image_helper' for #<ActionView::Base:0x000000000072d8>
# ./app/views/active_storage/blobs/_blob.html.erb:2:in `_app_views_active_storage_blobs__blob_html_erb__618535868824058076_14680'
# /Users/debono/.rvm/gems/ruby-2.7.1/gems/actiontext-6.1.0/app/helpers/action_text/content_helper.rb:25:in `block (2 levels) in render_action_text_attachments'
# /Users/debono/.rvm/gems/ruby-2.7.1/gems/actiontext-6.1.0/app/helpers/action_text/content_helper.rb:24:in `tap'
# /Users/debono/.rvm/gems/ruby-2.7.1/gems/actiontext-6.1.0/app/helpers/action_text/content_helper.rb:24:in `block in render_action_text_attachments'
# /Users/debono/.rvm/gems/ruby-2.7.1/gems/actiontext-6.1.0/lib/action_text/content.rb:62:in `block in render_attachments'
# /Users/debono/.rvm/gems/ruby-2.7.1/gems/actiontext-6.1.0/lib/action_text/fragment.rb:40:in `block (2 levels) in replace'
# /Users/debono/.rvm/gems/ruby-2.7.1/gems/nokogiri-1.10.10/lib/nokogiri/xml/node_set.rb:238:in `block in each'
# /Users/debono/.rvm/gems/ruby-2.7.1/gems/nokogiri-1.10.10/lib/nokogiri/xml/node_set.rb:237:in `upto'
# /Users/debono/.rvm/gems/ruby-2.7.1/gems/nokogiri-1.10.10/lib/nokogiri/xml/node_set.rb:237:in `each'
# /Users/debono/.rvm/gems/ruby-2.7.1/gems/actiontext-6.1.0/lib/action_text/fragment.rb:39:in `block in replace'
# /Users/debono/.rvm/gems/ruby-2.7.1/gems/actiontext-6.1.0/lib/action_text/fragment.rb:33:in `update'
# /Users/debono/.rvm/gems/ruby-2.7.1/gems/actiontext-6.1.0/lib/action_text/fragment.rb:38:in `replace'
# /Users/debono/.rvm/gems/ruby-2.7.1/gems/actiontext-6.1.0/lib/action_text/content.rb:61:in `render_attachments'
# /Users/debono/.rvm/gems/ruby-2.7.1/gems/actiontext-6.1.0/app/helpers/action_text/content_helper.rb:22:in `render_action_text_attachments'
# /Users/debono/.rvm/gems/ruby-2.7.1/gems/actiontext-6.1.0/app/helpers/action_text/content_helper.rb:14:in `render_action_text_content'
# /Users/debono/.rvm/gems/ruby-2.7.1/gems/actiontext-6.1.0/app/views/action_text/content/_layout.html.erb:2:in `___sers_debono__rvm_gems_ruby_______gems_actiontext_______app_views_action_text_content__layout_html_erb__3058938178293185412_14660'
# /Users/debono/.rvm/gems/ruby-2.7.1/gems/actiontext-6.1.0/lib/action_text/rendering.rb:26:in `render'
# /Users/debono/.rvm/gems/ruby-2.7.1/gems/actiontext-6.1.0/lib/action_text/rendering.rb:13:in `render'
# /Users/debono/.rvm/gems/ruby-2.7.1/gems/actiontext-6.1.0/lib/action_text/content.rb:87:in `to_rendered_html_with_layout'
# /Users/debono/.rvm/gems/ruby-2.7.1/gems/actiontext-6.1.0/lib/action_text/content.rb:91:in `to_s'
# /Users/debono/.rvm/gems/ruby-2.7.1/gems/actiontext-6.1.0/lib/action_text/content.rb:104:in `=='
# /Users/debono/.rvm/gems/ruby-2.7.1/gems/activestorage-6.1.0/lib/active_storage/attached/model.rb:199:in `changed_for_autosave?'
# ./spec/models/document_spec.rb:7:in `block (2 levels) in <main>'
# ------------------
# --- Caused by: ---
# NoMethodError:
#   undefined method `custom_image_helper' for #<ActionView::Base:0x000000000072d8>
#   ./app/views/active_storage/blobs/_blob.html.erb:2:in `_app_views_active_storage_blobs__blob_html_erb__618535868824058076_14680'

What appears to be happening, is that active record is checking if the ActionText::Content instance has changed by comparing the original with the new instance using ==.

This comparison is performed by converting both instances to strings using to_s https://github.com/rails/rails/blob/79de99271763833158f87e0ed050570ccf72c86d/actiontext/lib/action_text/content.rb#L104 Which in turn calls to_rendered_html_with_layout https://github.com/rails/rails/blob/79de99271763833158f87e0ed050570ccf72c86d/actiontext/lib/action_text/content.rb#L91

Rendering attachments in to_s is probably fine, but I don't think it should be called when comparing two instances. Perhaps using to_html would be better suited, since that's what is stored in the database?

rails-bot[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not been commented on for at least three months. The resources of the Rails team are limited, and so we are asking for your help. If you can still reproduce this error on the 6-1-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open. Thank you for all your contributions.

rails / rails