Skip to content

Commit cb72354

Browse files
committed
Add case-insensitive project identifier storage
Introduce case-insensitive handling for project identifiers: - Add `normalizes :identifier` to automatically upcase (alphanumeric mode) or downcase (numeric mode) identifiers on assignment - Add `parse_friendly_id` to normalize FriendlyId lookups for case-insensitive finder queries - Switch uniqueness validation to `case_sensitive: false` - Replace inline `exclusion:` validator with explicit `identifier_not_reserved` that checks case-insensitively - Consolidate alphanumeric format validators into a single `identifier_alphanumeric_format` method with early return to prevent cascading error messages - Use case-insensitive LOWER() comparison in historical identifier reservation check - Add `post_process` support to the OpActiveRecord acts_as_url adapter with an allowlist of safe transforms (upcase/downcase) - Add migration to replace the unique index on projects.identifier with a case-insensitive LOWER(identifier) index - Update table definition to match the new index Includes corresponding test additions for normalization, case- insensitive uniqueness, reserved identifier rejection, and the create_spec fixture fix for alphanumeric mode.
1 parent 1babb4b commit cb72354

File tree

7 files changed

+178
-17
lines changed

7 files changed

+178
-17
lines changed

app/models/projects/identifier.rb

Lines changed: 43 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -39,37 +39,42 @@ module Projects::Identifier
3939
extend FriendlyId
4040

4141
normalizes :identifier, with: OpenProject::RemoveAsciiControlCharacters
42+
normalizes :identifier, with: ->(value) {
43+
stripped = value.to_s.strip
44+
Setting::WorkPackageIdentifier.alphanumeric? ? stripped.upcase : stripped.downcase
45+
}
4246

4347
acts_as_url :name,
4448
url_attribute: :identifier,
4549
sync_url: false, # Don't update identifier when name changes
4650
only_when_blank: true, # Only generate when identifier not set
4751
limit: IDENTIFIER_MAX_LENGTH,
52+
force_downcase: false,
53+
post_process: ->(_instance) {
54+
Setting::WorkPackageIdentifier.alphanumeric? ? :upcase : :downcase
55+
},
4856
blacklist: RESERVED_IDENTIFIERS,
4957
adapter: OpenProject::ActsAsUrl::Adapter::OpActiveRecord # use a custom adapter able to handle edge cases
5058

51-
### Validators for the legacy underscored identifier format (e.g. "project_one")
5259
validates :identifier,
5360
presence: true,
54-
uniqueness: { case_sensitive: true },
61+
uniqueness: { case_sensitive: false },
5562
length: { maximum: IDENTIFIER_MAX_LENGTH },
56-
exclusion: RESERVED_IDENTIFIERS,
5763
if: ->(p) { p.persisted? || p.identifier.present? }
64+
65+
# Validators for the numeric identifier format (e.g. "project_one")
5866
# Contains only a-z, 0-9, dashes and underscores but cannot consist of numbers only as it would clash with the id.
5967
validates :identifier,
6068
format: { with: /\A(?!^\d+\z)[a-z0-9\-_]+\z/ },
6169
if: ->(p) {
6270
p.identifier_changed? && p.identifier.present? && Setting::WorkPackageIdentifier.numeric?
6371
}
6472

65-
### Validators for the uppercase identifier format (e.g. "PROJ1")
66-
validates :identifier,
67-
format: { with: /\A[A-Z]/, message: :must_start_with_letter },
68-
if: ->(p) { p.identifier_changed? && p.identifier.present? && Setting::WorkPackageIdentifier.alphanumeric? }
69-
validates :identifier,
70-
format: { with: /\A[A-Z][A-Z0-9_]*\z/, message: :no_special_characters },
71-
length: { maximum: SEMANTIC_IDENTIFIER_MAX_LENGTH },
72-
if: ->(p) { p.identifier_changed? && p.identifier.present? && Setting::WorkPackageIdentifier.alphanumeric? }
73+
# Validators for the alphanumeric identifier format (e.g. "PROJ1")
74+
validate :identifier_alphanumeric_format,
75+
if: ->(p) { p.identifier_changed? && p.identifier.present? && Setting::WorkPackageIdentifier.alphanumeric? }
76+
77+
validate :identifier_not_reserved, if: -> { identifier.present? }
7378

7479
# Complements the uniqueness validation above: once an identifier has been used by a
7580
# project, it remains reserved for that project even after the project moves to a new
@@ -88,6 +93,12 @@ def unset_slug_if_invalid; end
8893
end
8994

9095
class_methods do
96+
# Normalize the input to FriendlyID finders so that lookups are case-insensitive.
97+
# FriendlyID's default parse_friendly_id returns the value unchanged.
98+
def parse_friendly_id(value)
99+
normalize_value_for(:identifier, value)
100+
end
101+
91102
def suggest_identifier(name)
92103
if Setting::WorkPackageIdentifier.alphanumeric?
93104
WorkPackages::IdentifierAutofix::ProjectIdentifierSuggestionGenerator.suggest_identifier(name)
@@ -117,14 +128,33 @@ def validation_context
117128

118129
private
119130

131+
def identifier_alphanumeric_format
132+
unless identifier.match?(/\A[A-Z]/)
133+
errors.add(:identifier, :must_start_with_letter)
134+
return
135+
end
136+
137+
errors.add(:identifier, :no_special_characters) unless identifier.match?(/\A[A-Z][A-Z0-9_]*\z/)
138+
if identifier.length > SEMANTIC_IDENTIFIER_MAX_LENGTH
139+
errors.add(:identifier, :too_long, count: SEMANTIC_IDENTIFIER_MAX_LENGTH)
140+
end
141+
end
142+
143+
def identifier_not_reserved
144+
if RESERVED_IDENTIFIERS.include?(identifier&.downcase)
145+
errors.add(:identifier, :exclusion)
146+
end
147+
end
148+
120149
# Checks friendly_id_slugs for any project that previously used this identifier and
121150
# has since changed it. It allows to switch back to an identifier the project itself
122-
# has used before.
151+
# has used before. Uses case-insensitive comparison to prevent cross-case collisions.
123152
def identifier_not_historically_reserved
124153
return if errors.any? { |error| error.attribute == :identifier && error.type == :taken }
125154

126155
already_existing = FriendlyId::Slug
127-
.where(slug: identifier, sluggable_type: self.class.to_s)
156+
.where("LOWER(slug) = LOWER(?)", identifier)
157+
.where(sluggable_type: self.class.to_s)
128158
.where.not(sluggable_id: id)
129159
.exists?
130160

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
# frozen_string_literal: true
2+
3+
#-- copyright
4+
# OpenProject is an open source project management software.
5+
# Copyright (C) the OpenProject GmbH
6+
#
7+
# This program is free software; you can redistribute it and/or
8+
# modify it under the terms of the GNU General Public License version 3.
9+
#
10+
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
11+
# Copyright (C) 2006-2013 Jean-Philippe Lang
12+
# Copyright (C) 2010-2013 the ChiliProject Team
13+
#
14+
# This program is free software; you can redistribute it and/or
15+
# modify it under the terms of the GNU General Public License
16+
# as published by the Free Software Foundation; either version 2
17+
# of the License, or (at your option) any later version.
18+
#
19+
# This program is distributed in the hope that it will be useful,
20+
# but WITHOUT ANY WARRANTY; without even the implied warranty of
21+
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
22+
# GNU General Public License for more details.
23+
#
24+
# You should have received a copy of the GNU General Public License
25+
# along with this program; if not, write to the Free Software
26+
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
27+
#
28+
# See COPYRIGHT and LICENSE files for more details.
29+
#++
30+
31+
class AddCaseInsensitiveUniquenessForProjectIdentifiers < ActiveRecord::Migration[8.0]
32+
disable_ddl_transaction!
33+
34+
def up
35+
remove_index :projects, :identifier, unique: true, algorithm: :concurrently, if_exists: true
36+
add_index :projects, "LOWER(identifier)",
37+
unique: true,
38+
name: "index_projects_on_lower_identifier",
39+
algorithm: :concurrently,
40+
if_not_exists: true
41+
end
42+
43+
def down
44+
remove_index :projects, name: "index_projects_on_lower_identifier", algorithm: :concurrently, if_exists: true
45+
add_index :projects, :identifier, unique: true, algorithm: :concurrently
46+
end
47+
end

db/migrate/tables/projects.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ def self.table(migration) # rubocop:disable Metrics/AbcSize
4949

5050
t.index :lft, name: "index_projects_on_lft"
5151
t.index :rgt, name: "index_projects_on_rgt"
52-
t.index :identifier, unique: true
52+
t.index "LOWER(identifier)", unique: true, name: "index_projects_on_lower_identifier"
5353
t.index %i[lft rgt]
5454
end
5555
end

lib/open_project/acts_as_url/adapter/op_active_record.rb

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,8 @@ def url_attribute(instance)
5050
read_attribute instance, settings.url_attribute
5151
end
5252

53+
ALLOWED_POST_TRANSFORMS = %i[upcase downcase].freeze
54+
5355
private
5456

5557
def modify_base_url
@@ -58,6 +60,15 @@ def modify_base_url
5860
self.base_url = root.to_localized_slug(locale:, **configuration.string_extensions_settings)
5961

6062
modify_base_url_custom_rules if base_url.empty?
63+
apply_post_process
64+
end
65+
66+
def apply_post_process
67+
post_process = configuration.settings.post_process
68+
return unless post_process
69+
70+
method = post_process.respond_to?(:call) ? post_process.call(instance) : post_process
71+
self.base_url = base_url.public_send(method) if method && ALLOWED_POST_TRANSFORMS.include?(method)
6172
end
6273

6374
def modify_base_url_custom_rules

spec/features/projects/create_spec.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -592,6 +592,7 @@
592592

593593
fill_in "Name", with: "Flight Planning Algorithm"
594594
find("body").click
595+
expect(page).to have_field "Identifier", with: "FPA"
595596

596597
fill_in "Identifier", with: "3INVALID"
597598
click_on "Complete"

spec/models/project_spec.rb

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -606,5 +606,4 @@
606606
end
607607
end
608608
end
609-
610609
end

spec/models/projects/identifier_spec.rb

Lines changed: 75 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828
# See COPYRIGHT and LICENSE files for more details.
2929
#++
3030

31-
require "spec_helper"
31+
require "rails_helper"
3232

3333
RSpec.describe Projects::Identifier do
3434
describe "identifier normalization" do
@@ -37,7 +37,57 @@
3737
it { is_expected.to normalize(:identifier).from("my\n\x00project\t").to("myproject") }
3838
end
3939

40-
describe "url identifier" do
40+
describe ".normalize_value_for" do
41+
context "when in numeric mode (default)" do
42+
before { allow(Setting::WorkPackageIdentifier).to receive(:alphanumeric?).and_return(false) }
43+
44+
it "downcases the value" do
45+
expect(Project.normalize_value_for(:identifier, "PROJ")).to eq("proj")
46+
end
47+
48+
it "strips whitespace" do
49+
expect(Project.normalize_value_for(:identifier, " proj ")).to eq("proj")
50+
end
51+
52+
it "returns nil unchanged" do
53+
expect(Project.normalize_value_for(:identifier, nil)).to be_nil
54+
end
55+
end
56+
57+
context "when in alphanumeric mode" do
58+
before { allow(Setting::WorkPackageIdentifier).to receive(:alphanumeric?).and_return(true) }
59+
60+
it "upcases the value" do
61+
expect(Project.normalize_value_for(:identifier, "proj")).to eq("PROJ")
62+
end
63+
64+
it "strips whitespace" do
65+
expect(Project.normalize_value_for(:identifier, " proj ")).to eq("PROJ")
66+
end
67+
end
68+
end
69+
70+
describe "normalizes :identifier on Project" do
71+
context "when in numeric mode (default)" do
72+
before { allow(Setting::WorkPackageIdentifier).to receive(:alphanumeric?).and_return(false) }
73+
74+
it "downcases identifier on assignment" do
75+
project = Project.new(identifier: "MyProject")
76+
expect(project.identifier).to eq("myproject")
77+
end
78+
end
79+
80+
context "when in alphanumeric mode" do
81+
before { allow(Setting::WorkPackageIdentifier).to receive(:alphanumeric?).and_return(true) }
82+
83+
it "upcases identifier on assignment" do
84+
project = Project.new(identifier: "myproject")
85+
expect(project.identifier).to eq("MYPROJECT")
86+
end
87+
end
88+
end
89+
90+
describe "url identifier", with_settings: { work_packages_identifier: "numeric" } do
4191
let(:reserved) do
4292
Rails.application.routes.routes
4393
.map { |route| route.path.spec.to_s }
@@ -73,6 +123,14 @@
73123
expect(project.errors[:identifier]).to include("has already been taken.")
74124
end
75125

126+
it "is not allowed to clash with another project case-insensitively" do
127+
create(:project, identifier: "existing")
128+
129+
project = build(:project, identifier: "EXISTING")
130+
expect(project).not_to be_valid
131+
expect(project.errors[:identifier]).to include("has already been taken.")
132+
end
133+
76134
it "is not allowed to clash with a former identifier of another project" do
77135
other_project = create(:project, identifier: "former-id")
78136
other_project.update!(identifier: "new-id")
@@ -82,6 +140,15 @@
82140
expect(project.errors[:identifier]).to include("has already been taken.")
83141
end
84142

143+
it "is not allowed to clash with a former identifier of another project case-insensitively" do
144+
other_project = create(:project, identifier: "former-id")
145+
other_project.update!(identifier: "new-id")
146+
147+
project = build(:project, identifier: "FORMER-ID")
148+
expect(project).not_to be_valid
149+
expect(project.errors[:identifier]).to include("has already been taken.")
150+
end
151+
85152
it "is allowed to be the same as its own former identifier" do
86153
project = create(:project, identifier: "old-id")
87154
project.update!(identifier: "new-id")
@@ -90,6 +157,12 @@
90157
expect(project).to be_valid
91158
end
92159

160+
it "rejects reserved identifiers case-insensitively" do
161+
project = build(:project, identifier: "NEW")
162+
expect(project).not_to be_valid
163+
expect(project.errors[:identifier]).to include("is reserved.")
164+
end
165+
93166
# The acts_as_url plugin defines validation callbacks on :create and it is not automatically
94167
# called when calling a custom context. However we need the acts_as_url callback to set the
95168
# identifier when the validations are called with the :saving_custom_fields context.

0 commit comments

Comments
 (0)