app/controllers/application_controller.rb | 7 +++- app/controllers/repositories_controller.rb | 9 ++++- app/models/project.rb | 33 ++++++++++++----- app/models/project_identifier.rb | 25 +++++++++++++ app/models/repository.rb | 33 ++++++++++++----- app/models/repository_identifier.rb | 26 ++++++++++++++ app/views/projects/_form.html.erb | 12 +++---- app/views/repositories/_form.html.erb | 4 +-- config/locales/en.yml | 7 ++-- db/migrate/20210402143502_create_identifiers.rb | 17 +++++++++ db/migrate/20210402153405_migrate_identifiers.rb | 11 ++++++ lib/redmine/core_ext/active_record.rb | 44 +++++++++++++++++++++++ test/functional/issues_controller_test.rb | 16 +++++++++ test/functional/repositories_controller_test.rb | 33 +++++++++++++++++ test/unit/project_test.rb | 19 +++++----- test/unit/repository_git_test.rb | 2 +- test/unit/repository_test.rb | 46 +++++++++--------------- 17 files changed, 273 insertions(+), 71 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index b5644e89d..a75a26116 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -325,7 +325,12 @@ class ApplicationController < ActionController::Base def find_project(project_id=params[:id]) @project = Project.find(project_id) rescue ActiveRecord::RecordNotFound - render_404 + project_alias = Project.find_alias(project_id) + if @project = project_alias&.project + flash[:warning] = l(:warning_identifier_renamed, alias: project_alias.identifier, identifier: @project.identifier) + else + render_404 + end end # Find project of id params[:project_id] diff --git a/app/controllers/repositories_controller.rb b/app/controllers/repositories_controller.rb index dfa96cc0e..4204f941d 100644 --- a/app/controllers/repositories_controller.rb +++ b/app/controllers/repositories_controller.rb @@ -333,9 +333,16 @@ class RepositoriesController < ApplicationController REV_PARAM_RE = %r{\A[a-f0-9]*\Z}i def find_project_repository - @project = Project.find(params[:id]) + return unless find_project if params[:repository_id].present? @repository = @project.repositories.find_by_identifier_param(params[:repository_id]) + unless @repository + repository_alias = @project.repositories.find_alias(params[:repository_id]) + @repository = repository_alias&.repository + if @repository + flash[:warning] = l(:warning_identifier_renamed, alias: repository_alias.identifier, identifier: @repository.identifier) + end + end else @repository = @project.repository end diff --git a/app/models/project.rb b/app/models/project.rb index ab864436a..28ab125c8 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -52,6 +52,8 @@ class Project < ActiveRecord::Base has_many :repositories, :dependent => :destroy has_many :changesets, :through => :repository has_one :wiki, :dependent => :destroy + has_one :project_identifier, :primary_key => 'identifier', :foreign_key => 'identifier' + has_many :project_identifiers, :dependent => :destroy # Custom field for the project issues has_and_belongs_to_many :issue_custom_fields, lambda {order(:position)}, @@ -72,17 +74,21 @@ class Project < ActiveRecord::Base :author => nil validates_presence_of :name, :identifier - validates_uniqueness_of :identifier, :if => proc {|p| p.identifier_changed?}, :case_sensitive => true + validates :identifier, :if => proc {|p| p.identifier_changed? && p.identifier.present?}, :project_identifier_uniqueness => true validates_length_of :name, :maximum => 255 validates_length_of :homepage, :maximum => 255 validates_length_of :identifier, :maximum => IDENTIFIER_MAX_LENGTH # downcase letters, digits, dashes but not digits only validates_format_of :identifier, :with => /\A(?!\d+$)[a-z0-9\-_]*\z/, :if => proc {|p| p.identifier_changed?} + after_validation :reset_invalid_identifier + # reserved words validates_exclusion_of :identifier, :in => %w(new) validate :validate_parent + after_save :update_identifier, + :if => proc {|project| project.saved_change_to_identifier? && project.identifier.present? && !project.new_record?} after_save :update_inherited_members, :if => proc {|project| project.saved_change_to_inherit_members?} after_save :remove_inherited_member_roles, :add_inherited_member_roles, @@ -137,13 +143,6 @@ class Project < ActiveRecord::Base end end - def identifier=(identifier) - super unless identifier_frozen? - end - - def identifier_frozen? - errors[:identifier].blank? && !(new_record? || identifier.blank?) - end # returns latest created projects # non public projects will be returned only if user is a member of those @@ -350,6 +349,14 @@ class Project < ActiveRecord::Base end end + def self.find_alias(*args) + if args.first && args.first.is_a?(String) && !/^\d*$/.match?(args.first) + scope = ProjectIdentifier + scope = scope.where(project_id: current_scope) if scope_attributes? + scope.find_by_identifier(*args) + end + end + def self.find_by_param(*args) self.find(*args) end @@ -993,6 +1000,10 @@ class Project < ActiveRecord::Base end end + def update_identifier + self.project_identifiers << project_identifiers.build(:identifier => identifier, :project_id => self.id) + end + def remove_inherited_member_roles member_roles = MemberRole.where(:member_id => membership_ids).to_a member_role_ids = member_roles.map(&:id) @@ -1032,6 +1043,12 @@ class Project < ActiveRecord::Base end end + def reset_invalid_identifier + if errors[:identifier].present? + self.identifier = identifier_was + end + end + # Copies wiki from +project+ def copy_wiki(project) # Check that the source project has a wiki first diff --git a/app/models/project_identifier.rb b/app/models/project_identifier.rb new file mode 100644 index 000000000..7f63d3bd6 --- /dev/null +++ b/app/models/project_identifier.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +# Redmine - project management software +# Copyright (C) 2006-2021 Jean-Philippe Lang +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + +class ProjectIdentifier < ActiveRecord::Base + belongs_to :project + + validates :identifier, :uniqueness => { :case_sensitive => true }, :presence => true + validates :project_id, :presence => true +end diff --git a/app/models/repository.rb b/app/models/repository.rb index e65c800e6..543700994 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -29,11 +29,15 @@ class Repository < ActiveRecord::Base belongs_to :project has_many :changesets, lambda{order("#{Changeset.table_name}.committed_on DESC, #{Changeset.table_name}.id DESC")} has_many :filechanges, :class_name => 'Change', :through => :changesets + has_one :repository_identifier, :primary_key => 'identifier', :foreign_key => 'identifier' + has_many :repository_identifiers, :dependent => :destroy serialize :extra_info before_validation :normalize_identifier before_save :check_default + after_save :update_identifier, + :if => proc {|repository| repository.saved_change_to_identifier? && !repository.new_record? && repository.project} # Raw SQL to delete changesets and changes in the database # has_many :changesets, :dependent => :destroy is too slow for big repositories @@ -43,13 +47,14 @@ class Repository < ActiveRecord::Base validates_length_of :password, :maximum => 255, :allow_nil => true validates_length_of :root_url, :url, maximum: 255 validates_length_of :identifier, :maximum => IDENTIFIER_MAX_LENGTH, :allow_blank => true - validates_uniqueness_of :identifier, :scope => :project_id, :case_sensitive => true + validates :identifier, :if => proc { |r| r.identifier_changed? }, :repository_identifier_uniqueness => true validates_exclusion_of :identifier, :in => %w(browse show entry raw changes annotate diff statistics graph revisions revision) # donwcase letters, digits, dashes, underscores but not digits only validates_format_of :identifier, :with => /\A(?!\d+$)[a-z0-9\-_]*\z/, :allow_blank => true # Checks if the SCM is enabled when creating a repository validate :repo_create_validation, :on => :create validate :validate_repository_path + after_validation :reset_invalid_identifier safe_attributes( 'identifier', @@ -124,14 +129,6 @@ class Repository < ActiveRecord::Base end end - def identifier=(identifier) - super unless identifier_frozen? - end - - def identifier_frozen? - errors[:identifier].blank? && !(new_record? || identifier.blank?) - end - def identifier_param if identifier.present? identifier @@ -150,6 +147,12 @@ class Repository < ActiveRecord::Base end end + def reset_invalid_identifier + if errors[:identifier].present? + self.identifier = identifier_was + end + end + def self.find_by_identifier_param(param) if /^\d+$/.match?(param.to_s) find_by_id(param) @@ -158,6 +161,14 @@ class Repository < ActiveRecord::Base end end + def self.find_alias(param) + if !/^\d+$/.match?(param.to_s) + scope = RepositoryIdentifier + scope = scope.where(repository_id: current_scope) if scope_attributes? + scope.find_by_identifier(param) + end + end + # TODO: should return an empty hash instead of nil to avoid many ||{} def extra_info h = read_attribute(:extra_info) @@ -481,6 +492,10 @@ class Repository < ActiveRecord::Base self.identifier = identifier.to_s.strip end + def update_identifier + self.repository_identifiers << repository_identifiers.build(:project_id => project.id, :identifier => identifier, :repository_id => self.id) + end + def check_default if !is_default? && set_as_default? self.is_default = true diff --git a/app/models/repository_identifier.rb b/app/models/repository_identifier.rb new file mode 100644 index 000000000..74d412409 --- /dev/null +++ b/app/models/repository_identifier.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +# Redmine - project management software +# Copyright (C) 2006-2021 Jean-Philippe Lang +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + +class RepositoryIdentifier < ActiveRecord::Base + belongs_to :repository + belongs_to :project + + validates :identifier, :uniqueness => { :scope => :project_id, :case_sensitive => true } + validates :repository_id, :presence => true +end diff --git a/app/views/projects/_form.html.erb b/app/views/projects/_form.html.erb index 7c988fb0e..be840eb07 100644 --- a/app/views/projects/_form.html.erb +++ b/app/views/projects/_form.html.erb @@ -5,10 +5,10 @@

<%= f.text_field :name, :required => true, :size => 60 %>

<%= f.text_area :description, :rows => 8, :class => 'wiki-edit' %>

-

<%= f.text_field :identifier, :required => true, :size => 60, :disabled => @project.identifier_frozen?, :maxlength => Project::IDENTIFIER_MAX_LENGTH %> -<% unless @project.identifier_frozen? %> +

+ <%= f.text_field :identifier, :required => true, :size => 60, :maxlength => Project::IDENTIFIER_MAX_LENGTH %> <%= l(:text_length_between, :min => 1, :max => Project::IDENTIFIER_MAX_LENGTH) %> <%= l(:text_project_identifier_info).html_safe %> -<% end %>

+

<%= f.text_field :homepage, :size => 60 %>

<%= f.check_box :is_public %> @@ -44,10 +44,8 @@ <% end %> -<% unless @project.identifier_frozen? %> - <% content_for :header_tags do %> - <%= javascript_include_tag 'project_identifier' %> - <% end %> +<% content_for :header_tags do %> + <%= javascript_include_tag 'project_identifier' %> <% end %> <% if !User.current.admin? && @project.inherit_members? && @project.parent && User.current.member_of?(@project.parent) %> diff --git a/app/views/repositories/_form.html.erb b/app/views/repositories/_form.html.erb index b0f145ac0..349788b67 100644 --- a/app/views/repositories/_form.html.erb +++ b/app/views/repositories/_form.html.erb @@ -10,12 +10,10 @@

<%= f.check_box :is_default, :label => :field_repository_is_default %>

-<%= f.text_field :identifier, :disabled => @repository.identifier_frozen? %> -<% unless @repository.identifier_frozen? %> + <%= f.text_field :identifier %> <%= l(:text_length_between, :min => 1, :max => Repository::IDENTIFIER_MAX_LENGTH) %> <%= l(:text_repository_identifier_info).html_safe %> -<% end %>

<% button_disabled = true %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 9d779a2fe..82693abb7 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -116,6 +116,7 @@ en: too_short: "is too short (minimum is %{count} characters)" wrong_length: "is the wrong length (should be %{count} characters)" taken: "has already been taken" + used: "has already been used once" not_a_number: "is not a number" not_a_date: "is not a valid date" greater_than: "must be greater than %{count}" @@ -1199,7 +1200,7 @@ en: text_tip_issue_begin_day: issue beginning this day text_tip_issue_end_day: issue ending this day text_tip_issue_begin_end_day: issue beginning and ending this day - text_project_identifier_info: 'Only lower case letters (a-z), numbers, dashes and underscores are allowed.
Once saved, the identifier cannot be changed.' + text_project_identifier_info: 'Only lower case letters (a-z), numbers, dashes and underscores are allowed.' text_caracters_maximum: "%{count} characters maximum." text_caracters_minimum: "Must be at least %{count} characters long." text_characters_must_contain: "Must contain %{character_classes}." @@ -1312,7 +1313,7 @@ en: description_all_columns: All Columns description_issue_category_reassign: Choose issue category description_wiki_subpages_reassign: Choose new parent page - text_repository_identifier_info: 'Only lower case letters (a-z), numbers, dashes and underscores are allowed.
Once saved, the identifier cannot be changed.' + text_repository_identifier_info: 'Only lower case letters (a-z), numbers, dashes and underscores are allowed.' text_login_required_html: When not requiring authentication, public projects and their contents are openly available on the network. You can edit the applicable permissions. label_login_required_yes: "Yes" label_login_required_no: "No, allow anonymous access to public projects" @@ -1356,3 +1357,5 @@ en: text_user_destroy_confirmation: "Are you sure you want to delete this user and remove all references to them? This cannot be undone. Often, locking a user instead of deleting them is the better solution. To confirm, please enter their login (%{login}) below." text_project_destroy_enter_identifier: "To confirm, please enter the project's identifier (%{identifier}) below." + + warning_identifier_renamed: 'The identifier has been renamed from "%{alias}" to "%{identifier}", please change your url' diff --git a/db/migrate/20210402143502_create_identifiers.rb b/db/migrate/20210402143502_create_identifiers.rb new file mode 100644 index 000000000..5b04b61d0 --- /dev/null +++ b/db/migrate/20210402143502_create_identifiers.rb @@ -0,0 +1,17 @@ +class CreateIdentifiers < ActiveRecord::Migration[5.2] + def change + create_table :project_identifiers do |t| + t.references :project, :null => false, :index => true + t.string :identifier, :null => false, :index => { :unique => true } + t.timestamps + end + + create_table :repository_identifiers do |t| + t.references :project, :null => false + t.references :repository, :null => false, :index => true + t.string :identifier, :null => false + t.timestamps + end + add_index :repository_identifiers, [:project_id, :identifier], :unique => true + end +end diff --git a/db/migrate/20210402153405_migrate_identifiers.rb b/db/migrate/20210402153405_migrate_identifiers.rb new file mode 100644 index 000000000..a3c3d7401 --- /dev/null +++ b/db/migrate/20210402153405_migrate_identifiers.rb @@ -0,0 +1,11 @@ +class MigrateIdentifiers < ActiveRecord::Migration[5.2] + def up + Project.where.not(identifier: nil).find_each do |project| + ProjectIdentifier.create!(project: project, identifier: project.identifier) + end + + Repository.where.not(identifier: nil).find_each do |repository| + RepositoryIdentifier.create!(project: repository.project, respository: repository, identifier: repository.identifier) + end + end +end diff --git a/lib/redmine/core_ext/active_record.rb b/lib/redmine/core_ext/active_record.rb index df366b04f..c2e200ffb 100644 --- a/lib/redmine/core_ext/active_record.rb +++ b/lib/redmine/core_ext/active_record.rb @@ -27,3 +27,47 @@ class DateValidator < ActiveModel::EachValidator end end end + +class IdentifierUniquenessValidator < ActiveRecord::Validations::UniquenessValidator + include Redmine::I18n + + def initialize(options) + options[:case_sensitive] = true + super + end +end + +class ProjectIdentifierUniquenessValidator < IdentifierUniquenessValidator + def validate_each(record, attribute, value) + super + if record.errors[attribute].empty? + identifier = ProjectIdentifier.new + ProjectIdentifier.validators_on(:identifier).each do |validator| + validator.validate_each(identifier, :identifier, value) + end + if identifier.errors.any? + record.errors.add attribute, l(:used, :scope => [:activerecord, :errors, :messages]) + end + end + end +end + +class RepositoryIdentifierUniquenessValidator < IdentifierUniquenessValidator + def initialize(options) + options[:scope] = :project_id + super + end + + def validate_each(record, attribute, value) + super + if record.errors[attribute].empty? + identifier = RepositoryIdentifier.new(project: record.project) + RepositoryIdentifier.validators_on(:identifier).each do |validator| + validator.validate_each(identifier, :identifier, value) + end + if identifier.errors.any? + record.errors.add attribute, l(:used, :scope => [:activerecord, :errors, :messages]) + end + end + end +end \ No newline at end of file diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb index ccebb4311..4540e72ee 100644 --- a/test/functional/issues_controller_test.rb +++ b/test/functional/issues_controller_test.rb @@ -119,6 +119,22 @@ class IssuesControllerTest < Redmine::ControllerTest end end + def test_index_find_previous_project_identifier + Project.where(:id => 1).update_all(:identifier => 'new_identifier') + ProjectIdentifier.create(project_id: 1, identifier: 'previous_identifier') + + get(:index, :params => {:project_id => 'new_identifier'}) + assert_response :success + assert_select '.flash.warning', :count => 0 + + get(:index, :params => {:project_id => 'previous_identifier'}) + assert_response :success + assert_select '.flash.warning', :text => I18n.t(:warning_identifier_renamed, alias: 'previous_identifier', identifier: 'new_identifier') + + get(:index, :params => {:project_id => 'unknown_identifier'}) + assert_response 404 + end + def test_index_should_list_issues_of_closed_subprojects @request.session[:user_id] = 1 project = Project.find(1) diff --git a/test/functional/repositories_controller_test.rb b/test/functional/repositories_controller_test.rb index 7ee84048a..9f152acb8 100644 --- a/test/functional/repositories_controller_test.rb +++ b/test/functional/repositories_controller_test.rb @@ -236,6 +236,39 @@ class RepositoriesControllerTest < Redmine::RepositoryControllerTest assert_select 'table.changesets' end + def test_revisions_with_previous_repository_identifier + repository = Repository::Subversion.create!(:project_id => 1, :identifier => 'new_identifier', :url => 'file:///foo') + RepositoryIdentifier.create(project_id: 1, identifier: 'previous_identifier', repository_id: repository.id) + get( + :revisions, + :params => { + :id => 1, + :repository_id => 'new_identifier' + } + ) + assert_response :success + assert_select '.flash.warning', :count => 0 + + get( + :revisions, + :params => { + :id => 1, + :repository_id => 'previous_identifier' + } + ) + assert_response :success + assert_select '.flash.warning', :text => I18n.t(:warning_identifier_renamed, alias: 'previous_identifier', identifier: 'new_identifier') + + get( + :revisions, + :params => { + :id => 1, + :repository_id => 'unknown_identifier' + } + ) + assert_response 404 + end + def test_revisions_for_other_repository repository = Repository::Subversion.create!(:project_id => 1, :identifier => 'foo', :url => 'file:///foo') get( diff --git a/test/unit/project_test.rb b/test/unit/project_test.rb index b154e6b90..b0b0d2e04 100644 --- a/test/unit/project_test.rb +++ b/test/unit/project_test.rb @@ -132,18 +132,17 @@ class ProjectTest < ActiveSupport::TestCase end end - def test_identifier_should_not_be_frozen_for_a_new_project - assert_equal false, Project.new.identifier_frozen? - end - - def test_identifier_should_not_be_frozen_for_a_saved_project_with_blank_identifier - Project.where(:id => 1).update_all(["identifier = ''"]) - assert_equal false, Project.find(1).identifier_frozen? + def test_identifier_should_validate_used_identifiers + p = Project.find(1) + p.identifier = 'test' + assert p.save + p.identifier = 'test2' + assert p.save + p.identifier = 'test' + assert !p.save + assert p.errors['identifier'].present? end - def test_identifier_should_be_frozen_for_a_saved_project_with_valid_identifier - assert_equal true, Project.find(1).identifier_frozen? - end def test_to_param_should_be_nil_for_new_records project = Project.new diff --git a/test/unit/repository_git_test.rb b/test/unit/repository_git_test.rb index 506c75055..34b4d8940 100644 --- a/test/unit/repository_git_test.rb +++ b/test/unit/repository_git_test.rb @@ -46,7 +46,7 @@ class RepositoryGitTest < ActiveSupport::TestCase end def test_nondefault_repo_with_blank_identifier_destruction - Repository.delete_all + Repository.destroy_all repo1 = Repository::Git.new( diff --git a/test/unit/repository_test.rb b/test/unit/repository_test.rb index 5bff54300..600d710c9 100644 --- a/test/unit/repository_test.rb +++ b/test/unit/repository_test.rb @@ -114,6 +114,23 @@ class RepositoryTest < ActiveSupport::TestCase assert !r.save end + def test_identifier_should_validate_used_identifiers + r = Repository::Subversion.new(:project_id => 3, :identifier => 'test', :url => 'file:///bar') + assert r.save + r.identifier = 'test2' + assert r.save + r.identifier = 'test' + assert !r.save + assert r.errors['identifier'].present? + end + + def test_identifier_should_allow_same_identifier_on_multiple_projects + r = Repository::Subversion.new(:project_id => 3, :identifier => 'test', :url => 'file:///bar') + assert r.save + r = Repository::Subversion.new(:project_id => 4, :identifier => 'test', :url => 'file:///bar') + assert r.save + end + def test_first_repository_should_be_set_as_default repository1 = Repository::Subversion. @@ -179,35 +196,6 @@ class RepositoryTest < ActiveSupport::TestCase assert r.save end - def test_identifier_should_not_be_frozen_for_a_new_repository - assert_equal false, Repository.new.identifier_frozen? - end - - def test_identifier_should_not_be_frozen_for_a_saved_repository_with_blank_identifier - Repository.where(:id => 10).update_all(["identifier = ''"]) - assert_equal false, Repository.find(10).identifier_frozen? - end - - def test_identifier_should_be_frozen_for_a_saved_repository_with_valid_identifier - Repository.where(:id => 10).update_all(["identifier = 'abc123'"]) - assert_equal true, Repository.find(10).identifier_frozen? - end - - def test_identifier_should_not_accept_change_if_frozen - r = Repository.new(:identifier => 'foo') - r.stubs(:identifier_frozen?).returns(true) - - r.identifier = 'bar' - assert_equal 'foo', r.identifier - end - - def test_identifier_should_accept_change_if_not_frozen - r = Repository.new(:identifier => 'foo') - r.stubs(:identifier_frozen?).returns(false) - - r.identifier = 'bar' - assert_equal 'bar', r.identifier - end def test_destroy repository = Repository.find(10)