Skip to content
This repository has been archived by the owner on Jan 5, 2019. It is now read-only.

ActionText::Content#to_s triggers unnecessary queries #28

Closed
lsylvester opened this issue Oct 10, 2018 · 2 comments
Closed

ActionText::Content#to_s triggers unnecessary queries #28

lsylvester opened this issue Oct 10, 2018 · 2 comments
Assignees

Comments

@lsylvester
Copy link
Contributor

When rendering the content - each of the embedded blobs is individually loaded via the GlobalID::Locator - ignoring an preloaded values from with_rich_text_#{name}_and_embeds.

If the attachments are inside a gallery they seem to be loaded multiple time.

Failing test (requires some query assertions copied from ActiveRecord).

  test "with preloaded embeds it doesn't N+1 query" do
    ActiveStorage::Current.host = "http://localhost:3000"
    attachables = [create_file_blob(filename: "racecar.jpg", content_type: "image/jpg"), create_file_blob(filename: "racecar.jpg", content_type: "image/jpg")]
    html = attachables.map{ |attachable|
      <<~HTML
      <action-text-attachment sgid="#{attachable.attachable_sgid}" content-type="image/jpg" previewable="true" presentation="gallery" filename="racecar.jpg" url=#{attachable.service_url} filesize="418099" width="649" height="647" ></action-text-attachment>
      HTML
    }.join
    message = Message.create!(subject: "Greetings", content: "<div>#{html}</div>")
    message = Message.with_rich_text_content_and_embeds.find(message.id)
    sleep 1 # Wait for background processing
    assert_no_queries do
      message.content.to_s
    end
  end
Failure:
ActionText::ModelTest#test_with_preloaded_embeds_it_doesn't_N+1_query [/Users/lsylvester/Rails/actiontext/test/unit/model_test.rb:46]:
6 instead of 0 queries were executed.
Queries:
SELECT "active_storage_blobs".* FROM "active_storage_blobs" WHERE "active_storage_blobs"."id" = ? LIMIT ?
SELECT "active_storage_blobs".* FROM "active_storage_blobs" WHERE "active_storage_blobs"."id" = ? LIMIT ?
SELECT "active_storage_blobs".* FROM "active_storage_blobs" WHERE "active_storage_blobs"."id" = ? LIMIT ?
SELECT "active_storage_blobs".* FROM "active_storage_blobs" WHERE "active_storage_blobs"."id" = ? LIMIT ?
SELECT "active_storage_blobs".* FROM "active_storage_blobs" WHERE "active_storage_blobs"."id" = ? LIMIT ?
SELECT "active_storage_blobs".* FROM "active_storage_blobs" WHERE "active_storage_blobs"."id" = ? LIMIT ?.
Expected: 0
  Actual: 6

Code for this test is at https://github.com/lsylvester/actiontext/tree/unnecessary-queries-test but it is not ready for a PR.

@lsylvester
Copy link
Contributor Author

I think I misunderstood the purpose of this scope. The attachments in the content are not necessarily Blobs- but can be any kind thing that includes Attachable - so the scope to preload embeds isn't going to be sufficient.

Regardless - we should only be doing one query per kind of attachment type and not queuing multiple times for the same record.

It would also be good to be able to preload all of the records referenced within the content - but that doesn't seem to be possible with the current associations set up.

@javan javan assigned sstephenson and unassigned javan Oct 19, 2018
@georgeclaghorn
Copy link
Contributor

Action Text has been merged into Rails. Can you please reopen this in rails/rails? Thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants