From 0a5a6dfded8bd36ae0fa4c8ebaa4b374710b8d91 Mon Sep 17 00:00:00 2001 From: Sam Saffron Date: Tue, 30 Apr 2019 10:25:53 +1000 Subject: [PATCH] DEV: stop mutating inputs as a side effect We had quite a few cases in core where inputs are being mutated as a side effect of calling a method. This handles all the cases where specs caught this. Mutating inputs makes code harder to reason about. Eg: ``` frog = "frog" jump(frog) puts frog "fly" # ????? ``` This commit is part of a followup commit that adds # frozen_string_literal to all our specs. --- lib/admin_user_index_query.rb | 4 ++- lib/discourse_tagging.rb | 5 +-- lib/email/message_builder.rb | 9 +++-- lib/site_setting_extension.rb | 3 +- lib/text_cleaner.rb | 6 +++- lib/topic_query.rb | 34 +++++++++++++------ lib/user_name_suggester.rb | 4 ++- .../reply_by_email_address_validator.rb | 5 +-- lib/wizard/step_updater.rb | 4 ++- .../components/site_setting_extension_spec.rb | 4 ++- 10 files changed, 55 insertions(+), 23 deletions(-) diff --git a/lib/admin_user_index_query.rb b/lib/admin_user_index_query.rb index 19df51ab4f0..69498329c4f 100644 --- a/lib/admin_user_index_query.rb +++ b/lib/admin_user_index_query.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require_dependency 'trust_level' class AdminUserIndexQuery @@ -108,7 +110,7 @@ class AdminUserIndexQuery filter = params[:filter] if filter.present? - filter.strip! + filter = filter.strip if ip = IPAddr.new(filter) rescue nil @query.where('ip_address <<= :ip OR registration_ip_address <<= :ip', ip: ip.to_cidr_s) else diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb index ffd8b143e54..4ddc8875c9b 100644 --- a/lib/discourse_tagging.rb +++ b/lib/discourse_tagging.rb @@ -111,8 +111,9 @@ module DiscourseTagging term = opts[:term] if term.present? - term.gsub!("_", "\\_") - term = clean_tag(term).downcase + term = term.gsub("_", "\\_") + clean_tag(term) + term.downcase! query = query.where('lower(tags.name) like ?', "%#{term}%") end diff --git a/lib/email/message_builder.rb b/lib/email/message_builder.rb index 66a9230c775..55b64cd5246 100644 --- a/lib/email/message_builder.rb +++ b/lib/email/message_builder.rb @@ -118,8 +118,13 @@ module Email end def body - body = @opts[:body] - body = I18n.t("#{@opts[:template]}.text_body_template", template_args).dup if @opts[:template] + body = nil + + if @opts[:template] + body = I18n.t("#{@opts[:template]}.text_body_template", template_args).dup + else + body = @opts[:body].dup + end if @template_args[:unsubscribe_instructions].present? body << "\n" diff --git a/lib/site_setting_extension.rb b/lib/site_setting_extension.rb index e62ca8a4419..bcca40f6e6b 100644 --- a/lib/site_setting_extension.rb +++ b/lib/site_setting_extension.rb @@ -382,7 +382,7 @@ module SiteSettingExtension def filter_value(name, value) if HOSTNAME_SETTINGS.include?(name) - value.split("|").map { |url| get_hostname(url) }.compact.uniq.join("|") + value.split("|").map { |url| url.strip!; get_hostname(url) }.compact.uniq.join("|") else value end @@ -488,7 +488,6 @@ module SiteSettingExtension end def get_hostname(url) - url.strip! host = begin URI.parse(url)&.host diff --git a/lib/text_cleaner.rb b/lib/text_cleaner.rb index cdbccc1ba1f..fa136a4984d 100644 --- a/lib/text_cleaner.rb +++ b/lib/text_cleaner.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + # # Clean up a text # @@ -27,6 +29,8 @@ class TextCleaner end def self.clean(text, opts = {}) + text = text.dup + # Remove invalid byte sequences text.scrub!("") # Replace !!!!! with a single ! @@ -38,7 +42,7 @@ class TextCleaner # Capitalize first letter, but only when entire first word is lowercase first, rest = text.split(' ', 2) if first && opts[:capitalize_first_letter] && first == first.mb_chars.downcase - text = "#{first.mb_chars.capitalize}#{rest ? ' ' + rest : ''}" + text = +"#{first.mb_chars.capitalize}#{rest ? ' ' + rest : ''}" end # Remove unnecessary periods at the end text.sub!(/([^.])\.+(\s*)\z/, '\1\2') if opts[:remove_all_periods_from_the_end] diff --git a/lib/topic_query.rb b/lib/topic_query.rb index e7950d4b660..85e3739b7d6 100644 --- a/lib/topic_query.rb +++ b/lib/topic_query.rb @@ -685,17 +685,26 @@ class TopicQuery if SiteSetting.tagging_enabled result = result.preload(:tags) - if @options[:tags] && @options[:tags].size > 0 - @options[:tags] = @options[:tags].split unless @options[:tags].respond_to?('each') - @options[:tags].each { |t| t.downcase! if t.is_a? String } + tags = @options[:tags] + + if tags && tags.size > 0 + tags = tags.split if String === tags + + tags = tags.map do |t| + if String === t + t.downcase + else + t + end + end if @options[:match_all_tags] # ALL of the given tags: - tags_count = @options[:tags].length - @options[:tags] = Tag.where_name(@options[:tags]).pluck(:id) unless @options[:tags][0].is_a?(Integer) + tags_count = tags.length + tags = Tag.where_name(tags).pluck(:id) unless Integer === tags[0] - if tags_count == @options[:tags].length - @options[:tags].each_with_index do |tag, index| + if tags_count == tags.length + tags.each_with_index do |tag, index| sql_alias = ['t', index].join result = result.joins("INNER JOIN topic_tags #{sql_alias} ON #{sql_alias}.topic_id = topics.id AND #{sql_alias}.tag_id = #{tag}") end @@ -705,12 +714,17 @@ class TopicQuery else # ANY of the given tags: result = result.joins(:tags) - if @options[:tags][0].is_a?(Integer) - result = result.where("tags.id in (?)", @options[:tags]) + if Integer === tags[0] + result = result.where("tags.id in (?)", tags) else - result = result.where("lower(tags.name) in (?)", @options[:tags]) + result = result.where("lower(tags.name) in (?)", tags) end end + + # TODO: this is very side-effecty and should be changed + # It is done cause further up we expect normalized tags + @options[:tags] = tags + elsif @options[:no_tags] # the following will do: ("topics"."id" NOT IN (SELECT DISTINCT "topic_tags"."topic_id" FROM "topic_tags")) result = result.where.not(id: TopicTag.distinct.pluck(:topic_id)) diff --git a/lib/user_name_suggester.rb b/lib/user_name_suggester.rb index d9c72edebd6..f414351463e 100644 --- a/lib/user_name_suggester.rb +++ b/lib/user_name_suggester.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module UserNameSuggester GENERIC_NAMES = ['i', 'me', 'info', 'support', 'admin', 'webmaster', 'hello', 'mail', 'office', 'contact', 'team'] @@ -41,7 +43,7 @@ module UserNameSuggester end def self.sanitize_username(name) - name = name.to_s + name = name.to_s.dup if SiteSetting.unicode_usernames name.unicode_normalize! diff --git a/lib/validators/reply_by_email_address_validator.rb b/lib/validators/reply_by_email_address_validator.rb index a9110e0f850..02864beb3cd 100644 --- a/lib/validators/reply_by_email_address_validator.rb +++ b/lib/validators/reply_by_email_address_validator.rb @@ -1,15 +1,16 @@ +# frozen_string_literal: true + class ReplyByEmailAddressValidator def initialize(opts = {}) @opts = opts end def valid_value?(val) - val&.strip! - return true if val.blank? return false if !val.include?("@") value = val.dup + value.strip! if SiteSetting.find_related_post_with_key return false if !value.include?("%{reply_key}") diff --git a/lib/wizard/step_updater.rb b/lib/wizard/step_updater.rb index 5b1c88db7f1..81d9ba0934d 100644 --- a/lib/wizard/step_updater.rb +++ b/lib/wizard/step_updater.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + class Wizard class StepUpdater include ActiveModel::Model @@ -29,7 +31,7 @@ class Wizard end def update_setting(id, value) - value.strip! if value.is_a?(String) + value = value.strip if value.is_a?(String) if !value.is_a?(Upload) && SiteSetting.type_supervisor.get_type(id) == :upload value = Upload.get_from_url(value) || '' diff --git a/spec/components/site_setting_extension_spec.rb b/spec/components/site_setting_extension_spec.rb index 7593d0f514f..dc351614a9e 100644 --- a/spec/components/site_setting_extension_spec.rb +++ b/spec/components/site_setting_extension_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'rails_helper' require_dependency 'site_setting_extension' require_dependency 'site_settings/local_process_provider' @@ -776,8 +778,8 @@ describe SiteSettingExtension do describe "get_hostname" do it "properly extracts the hostname" do + # consider testing this through a public interface, this tests implementation details expect(settings.send(:get_hostname, "discourse.org")).to eq("discourse.org") - expect(settings.send(:get_hostname, " discourse.org ")).to eq("discourse.org") expect(settings.send(:get_hostname, "@discourse.org")).to eq("discourse.org") expect(settings.send(:get_hostname, "https://discourse.org")).to eq("discourse.org") end