FIX: PG::StringDataRightTruncation when linking posts (#13134)

Users who use encoded slugs on their sites sometimes run into 500 error when pasting a link to another topic in a post. The problem happens when generating a backward "reflection" link that would appear in a linked topic. Link URL restricted on the database level to 500 chars in length. At first glance, it should work since we have a restriction on topic title length.

But it doesn't work when a site uses encoded slugs, like here (take a look at the URL). The link to a topic, in this case, can be much longer than 500 characters.

By the way, an error happens only when generating a "reflection" link and doesn't happen with a direct link, we truncate that link. It works because, in this case, the original long link is still present in the post body and can be used for navigation. But we can't do the same for backward "reflection" links (without rewriting their implementation), the whole link must be saved to the database.

The simplest and cleanest solution will be just to remove the restriction on the database level. Abuse is impossible here since we are already protected by the restriction on topic title length. There aren’t performance benefits in using length-constrained columns in Postgres, in fact, length-constrained columns need a few extra CPU cycles to check the length when storing data.
This commit is contained in:
Andrei Prigorshnev 2021-06-02 15:27:04 +04:00 committed by GitHub
parent b7b8f5e6f3
commit 932a2fe419
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 48 additions and 1 deletions

View File

@ -386,7 +386,7 @@ end
# topic_id :integer not null # topic_id :integer not null
# post_id :integer # post_id :integer
# user_id :integer not null # user_id :integer not null
# url :string(500) not null # url :string not null
# domain :string(100) not null # domain :string(100) not null
# internal :boolean default(FALSE), not null # internal :boolean default(FALSE), not null
# link_topic_id :integer # link_topic_id :integer

View File

@ -0,0 +1,11 @@
# frozen_string_literal: true
class RemoveLengthConstrainFromTopicLinkUrl < ActiveRecord::Migration[6.1]
def up
change_column :topic_links, :url, :string, null: false
end
def down
raise ActiveRecord::IrreversibleMigration
end
end

View File

@ -201,6 +201,42 @@ describe TopicLink do
expect(other_topic.reload.topic_links.where(link_post_id: linked_post.id)).to be_blank expect(other_topic.reload.topic_links.where(link_post_id: linked_post.id)).to be_blank
end end
it 'truncates long links' do
SiteSetting.slug_generation_method = 'encoded'
long_title = "Καλημερα σε ολους και ολες" * 9 # 234 chars, but the encoded slug will be 1224 chars in length
other_topic = Fabricate(:topic, user: user, title: long_title)
expect(other_topic.slug.length).to be > TopicLink.max_url_length
other_topic.posts.create(user: user, raw: 'initial post')
other_topic_url = "http://#{test_uri.host}/t/#{other_topic.slug}/#{other_topic.id}"
post_with_link = topic.posts.create(user: user, raw: "Link to another topic: #{other_topic_url}")
TopicLink.extract_from(post_with_link)
topic.reload
link = topic.topic_links.first
expect(link.url.length).to eq(TopicLink.max_url_length)
end
it 'does not truncate reflection links' do
SiteSetting.slug_generation_method = 'encoded'
long_title = "Καλημερα σε ολους και ολες" * 9 # 234 chars, but the encoded slug will be 1224 chars in length
topic = Fabricate(:topic, user: user, title: long_title)
expect(topic.slug.length).to be > TopicLink.max_url_length
topic_url = "http://#{test_uri.host}/t/#{topic.slug}/#{topic.id}"
other_topic = Fabricate(:topic, user: user)
other_topic.posts.create(user: user, raw: 'initial post')
other_topic_url = "http://#{test_uri.host}/t/#{other_topic.slug}/#{other_topic.id}"
post_with_link = topic.posts.create(user: user, raw: "Link to another topic: #{other_topic_url}")
expect { TopicLink.extract_from(post_with_link) }.to_not raise_error
other_topic.reload
reflection_link = other_topic.topic_links.first
expect(reflection_link.url.length).to be > (TopicLink.max_url_length)
expect(reflection_link.url).to eq(topic_url)
end
end end
context "link to a user on discourse" do context "link to a user on discourse" do