One of TopicEmbed.import spec fails with Ruby 3

yahonda · October 6, 2021, 2:31pm

I’d like to get some help about failing TopicEmbed.import spec with Ruby 3.

Steps to reproduce

cd discourse
bundle exec rspec ./spec/models/topic_embed_spec.rb:154

Expected behavior

It should finish successfully.

Actual behavior with Ruby 3.0.2

$ bundle exec rspec ./spec/models/topic_embed_spec.rb:154
Run options: include {:locations=>{"./spec/models/topic_embed_spec.rb"=>[154]}}

Randomized with seed 41701
F

Failures:

  1) TopicEmbed.import embedded content truncation keeps everything in the imported post when truncation is disabled
     Failure/Error: expect(post.raw).to include(long_content)

       expected "<p>aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa...for the original entry at <a href=\"http://eviltrout.com/123\">http://eviltrout.com/123</a></small>" to include "<p>aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa...for the original entry at <a href='http://eviltrout.com/123'>http://eviltrout.com/123</a></small>\n"
       Diff:
       @@ -1,4 +1,7 @@
       -<p>aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa</p>\n<p>more</p>\n<hr>\n<small>This is a companion discussion topic for the original entry at <a href='http://eviltrout.com/123'>http://eviltrout.com/123</a></small>\n
       +<p>aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa</p>
       +<p>more</p>
       +<hr>
       +<small>This is a companion discussion topic for the original entry at <a href="http://eviltrout.com/123">http://eviltrout.com/123</a></small>

     # ./spec/models/topic_embed_spec.rb:158:in `block (4 levels) in <top (required)>'
     # ./spec/rails_helper.rb:279:in `block (2 levels) in <top (required)>'

Finished in 2.48 seconds (files took 2.73 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/models/topic_embed_spec.rb:154 # TopicEmbed.import embedded content truncation keeps everything in the imported post when truncation is disabled

Randomized with seed 41701

$

Actual behavior with Ruby 2.7.4

$ ruby -v
ruby 2.7.4p191 (2021-07-07 revision a21a3b7d23) [x86_64-linux]
$ bundle exec rspec ./spec/models/topic_embed_spec.rb:154
Run options: include {:locations=>{"./spec/models/topic_embed_spec.rb"=>[154]}}

Randomized with seed 4126
.

Finished in 2.51 seconds (files took 2.31 seconds to load)
1 example, 0 failures

Randomized with seed 4126

$

What I’ve found so far

Added some print debug code and found the long_content inside the topic_embed_spec.rb was overwritten by contents of app/models/topic_embed.rb

Print debug code

$ git diff
diff --git a/app/models/topic_embed.rb b/app/models/topic_embed.rb
index 1e9f06a322..ab407d8005 100644
--- a/app/models/topic_embed.rb
+++ b/app/models/topic_embed.rb
@@ -28,6 +28,7 @@ class TopicEmbed < ActiveRecord::Base

   # Import an article from a source (RSS/Atom/Other)
   def self.import(user, url, title, contents, category_id: nil, cook_method: nil, tags: nil)
+    p "contents.length at the begining of self.import: #{contents.length}"
     return unless url =~ /^https?\:\/\//

     if SiteSetting.embed_truncate && cook_method.nil?
@@ -102,6 +103,7 @@ class TopicEmbed < ActiveRecord::Base
       end
     end

+    p "contents.length at the end of self.import: #{contents.length}"
     post
   end

diff --git a/spec/models/topic_embed_spec.rb b/spec/models/topic_embed_spec.rb
index 8659eb7d56..1266441da7 100644
--- a/spec/models/topic_embed_spec.rb
+++ b/spec/models/topic_embed_spec.rb
@@ -153,7 +153,9 @@ describe TopicEmbed do

       it 'keeps everything in the imported post when truncation is disabled' do
         SiteSetting.embed_truncate = false
+        p "long_content.length before calling TopicEmbed.import: #{long_content.length}"
         post = TopicEmbed.import(user, url, title, long_content)
+        p "long_content.length after calling TopicEmbed.import: #{long_content.length}"

         expect(post.raw).to include(long_content)
       end
$

Result with Ruby 3.0.2

$ bundle exec rspec ./spec/models/topic_embed_spec.rb:154
Run options: include {:locations=>{"./spec/models/topic_embed_spec.rb"=>[154]}}

Randomized with seed 24988
"long_content.length before calling TopicEmbed.import: 119"
"contents.length at the begining of self.import: 119"
"contents.length at the end of self.import: 267"
"long_content.length after calling TopicEmbed.import: 267"
F

Failures:

  1) TopicEmbed.import embedded content truncation keeps everything in the imported post when truncation is disabled
     Failure/Error: expect(post.raw).to include(long_content)

       expected "<p>aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa...for the original entry at <a href=\"http://eviltrout.com/123\">http://eviltrout.com/123</a></small>" to include "<p>aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa...for the original entry at <a href='http://eviltrout.com/123'>http://eviltrout.com/123</a></small>\n"
       Diff:
       @@ -1,4 +1,7 @@
       -<p>aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa</p>\n<p>more</p>\n<hr>\n<small>This is a companion discussion topic for the original entry at <a href='http://eviltrout.com/123'>http://eviltrout.com/123</a></small>\n
       +<p>aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa</p>
       +<p>more</p>
       +<hr>
       +<small>This is a companion discussion topic for the original entry at <a href="http://eviltrout.com/123">http://eviltrout.com/123</a></small>

     # ./spec/models/topic_embed_spec.rb:160:in `block (4 levels) in <top (required)>'
     # ./spec/rails_helper.rb:279:in `block (2 levels) in <top (required)>'

Finished in 2.55 seconds (files took 3.4 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/models/topic_embed_spec.rb:154 # TopicEmbed.import embedded content truncation keeps everything in the imported post when truncation is disabled

Randomized with seed 24988

$

Result with Ruby 2.7.4

$ bundle exec rspec ./spec/models/topic_embed_spec.rb:154
Run options: include {:locations=>{"./spec/models/topic_embed_spec.rb"=>[154]}}

Randomized with seed 1802
"long_content.length before calling TopicEmbed.import: 119"
"contents.length at the begining of self.import: 119"
"contents.length at the end of self.import: 267"
"long_content.length after calling TopicEmbed.import: 119"
.

Finished in 2.56 seconds (files took 3.06 seconds to load)
1 example, 0 failures

Randomized with seed 1802

$

yahonda · October 8, 2021, 6:00am

I’ve got why this spec fails only with Ruby 3.0.2.

Since Ruby 3, interpolated strings are not frozen even if frozen-string-literal magic comment is enabled.
https://bugs.ruby-lang.org/issues/17104

So here are possible ways to fix it.

Use contens.dup instead of +contents

github.com

discourse/discourse/blob/016524e244e5a179c357ed278ed2fd6355b2206f/app/models/topic_embed.rb#L37

    
      
          end
          
          
# Import an article from a source (RSS/Atom/Other)
          def self.import(user, url, title, contents, category_id: nil, cook_method: nil, tags: nil)
            return unless url =~ /^https?\:\/\//
          
          
  if SiteSetting.embed_truncate && cook_method.nil?
              contents = first_paragraph_from(contents)
            end
            contents ||= ''
            contents = +contents << imported_from_html(url)
          
          
  url = normalize_url(url)
          
          
  embed = TopicEmbed.find_by("lower(embed_url) = ?", url)
            content_sha1 = Digest::SHA1.hexdigest(contents)
            post = nil
          
          
  # If there is no embed, create a topic, post and the embed.
            if embed.blank?
              Topic.transaction do

$ git diff
diff --git a/app/models/topic_embed.rb b/app/models/topic_embed.rb
index 1e9f06a322..502409d3c1 100644
--- a/app/models/topic_embed.rb
+++ b/app/models/topic_embed.rb
@@ -34,7 +34,7 @@ class TopicEmbed < ActiveRecord::Base
       contents = first_paragraph_from(contents)
     end
     contents ||= ''
-    contents = +contents << imported_from_html(url)
+    contents = contents.dup << imported_from_html(url)

     url = normalize_url(url)

$

Freeze interpolated long_content

github.com

discourse/discourse/blob/016524e244e5a179c357ed278ed2fd6355b2206f/spec/models/topic_embed_spec.rb#L145

    
      
              expect(post).to be_present
          
          
    # It uses regular rendering
              expect(post.cook_method).to eq(Post.cook_methods[:regular])
            end
          end
          
          
describe 'embedded content truncation' do
            MAX_LENGTH_BEFORE_TRUNCATION = 100
          
          
  let(:long_content) { "<p>#{'a' * MAX_LENGTH_BEFORE_TRUNCATION}</p>\n<p>more</p>" }
          
          
  it 'truncates the imported post when truncation is enabled' do
              SiteSetting.embed_truncate = true
              post = TopicEmbed.import(user, url, title, long_content)
          
          
    expect(post.raw).not_to include(long_content)
            end
          
          
  it 'keeps everything in the imported post when truncation is disabled' do
              SiteSetting.embed_truncate = false

diff --git a/spec/models/topic_embed_spec.rb b/spec/models/topic_embed_spec.rb
index 8659eb7d56..a25351a586 100644
--- a/spec/models/topic_embed_spec.rb
+++ b/spec/models/topic_embed_spec.rb
@@ -142,7 +142,7 @@ describe TopicEmbed do
     describe 'embedded content truncation' do
       MAX_LENGTH_BEFORE_TRUNCATION = 100

-      let(:long_content) { "<p>#{'a' * MAX_LENGTH_BEFORE_TRUNCATION}</p>\n<p>more</p>" }
+      let(:long_content) { "<p>#{'a' * MAX_LENGTH_BEFORE_TRUNCATION}</p>\n<p>more</p>".freeze }

       it 'truncates the imported post when truncation is enabled' do
         SiteSetting.embed_truncate = true
$

I’m inclined to open a pull request to use contens.dup and found the idea was implemented but reverted via Revert "FIX: Use #dup instead of #+@ since content could be an instan… · discourse/discourse@d01c938 · GitHub

I did not find some other backgrounds for the revert, I’d like to hear from contributors/maintainers.

yahonda · October 10, 2021, 1:23pm

Opened FIX: Ruby 3 does not freeze interpolated string by yahonda · Pull Request #14567 · discourse/discourse · GitHub

Topic		Replies	Views
Comments not embedding, 500 Error Feature pr-welcome	15	1922	October 3, 2017
Ruby 3 support related to mocha gem Dev	4	874	October 12, 2021
Migrating vBulletin 5 database - Import script errors Migration vbulletin5	46	2122	March 8, 2023
Update failing due to rate limiting by RubyGems Support	40	5537	December 18, 2020
Best tools, materials & practices to onboard a hoarde of new users? Community	22	3455	January 30, 2018