Un des spéc de TopicEmbed.import échoue avec Ruby 3

yahonda · Octobre 6, 2021, 2:31

J’aimerais obtenir de l’aide concernant l’échec de la spécification TopicEmbed.import avec Ruby 3.

Étapes pour reproduire

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

Comportement attendu

Elle devrait se terminer avec succès.

Comportement réel avec 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

$

Comportement réel avec 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

$

Ce que j’ai découvert jusqu’à présent

J’ai ajouté du code de débogage avec des impressions et découvert que long_content dans topic_embed_spec.rb était écrasé par contents de app/models/topic_embed.rb.

Code de débogage avec impressions

$ 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
$

Résultat avec 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

$

Résultat avec 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 · Octobre 8, 2021, 6:00

J’ai compris pourquoi cette spécification échoue uniquement avec Ruby 3.0.2.

Depuis Ruby 3, les chaînes interpolées ne sont pas figées, même si le commentaire magique frozen-string-literal est activé.

Voici donc des solutions possibles pour corriger le problème.

Utiliser contents.dup au lieu de +contents

github.com/discourse/discourse

app/models/topic_embed.rb

016524e24


      
          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)

$

Geler long_content interpolé

github.com/discourse/discourse

spec/models/topic_embed_spec.rb

016524e24


      
              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
$

Je penche pour ouvrir une pull request afin d’utiliser contents.dup et j’ai constaté que cette idée avait été implémentée puis annulée via Revert "FIX: Use #dup instead of #+@ since content could be an instan… · discourse/discourse@d01c938 · GitHub

Je n’ai pas trouvé d’autres explications concernant cette annulation ; j’aimerais avoir l’avis des contributeurs/maintainers.

yahonda · Octobre 10, 2021, 1:23

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

Sujet		Réponses	Vues
Embed poll_feed fail with UTF8 title Bug	6	1574	Juillet 21, 2022
phpBB 3 Importer (old) Feature	90	17302	Octobre 8, 2014
TopicEmbed.embed_url length limit 255 characters Bug	3	991	Avril 21, 2015
Embedding comments widget on localhost causes FrozenError Support	1	457	Mai 16, 2020
"Show Full Post" button doesn't work in subfolder installations Support embedding , subfolder	33	505	Janvier 2, 2026