Browse Source

Fixes #5461 - Creating a Ticket Object called 'article' will lead to all kinds of errors.

Co-authored-by: Dominik Klein <dk@zammad.com>
Florian Liebe 1 month ago
parent
commit
7377bbd6ff

+ 46 - 3
app/models/object_manager/attribute.rb

@@ -29,6 +29,45 @@ class ObjectManager::Attribute < ApplicationModel
     active
   ].freeze
 
+  RESERVED_NAMES = %w[
+    destroy
+    true
+    false
+    integer
+    select
+    drop
+    create
+    alter
+    index
+    table
+    varchar
+    blob
+    date
+    datetime
+    timestamp
+    url
+    icon
+    initials
+    avatar
+    permission
+    validate
+    subscribe
+    unsubscribe
+    translate
+    search
+    _type
+    _doc
+    _id
+    id
+    action
+    scope
+    constructor
+  ].freeze
+
+  RESERVED_NAMES_PER_MODEL = {
+    'Ticket' => %w[article],
+  }.freeze
+
   self.table_name = 'object_manager_attributes'
 
   belongs_to :object_lookup, optional: true
@@ -909,8 +948,12 @@ is certain attribute used by triggers, overviews or schedulers
     end
 
     # do not allow model method names as attributes
-    reserved_words = %w[destroy true false integer select drop create alter index table varchar blob date datetime timestamp url icon initials avatar permission validate subscribe unsubscribe translate search _type _doc _id id action scope constructor]
-    if name.match?(%r{^(#{reserved_words.join('|')})$})
+    if name.match?(%r{^(#{RESERVED_NAMES.join('|')})$})
+      errors.add(:name, __('%{name} is a reserved word'), name: name)
+    end
+
+    model = object_lookup.name
+    if RESERVED_NAMES_PER_MODEL.key?(model) && name.match?(%r{^(#{RESERVED_NAMES_PER_MODEL[model].join('|')})$})
       errors.add(:name, __('%{name} is a reserved word'), name: name)
     end
 
@@ -921,7 +964,7 @@ is certain attribute used by triggers, overviews or schedulers
       errors.add(:name, __('%{name} is a reserved word'), name: name)
     end
 
-    record = object_lookup.name.constantize.new
+    record = model.constantize.new
     if new_record? && (record.respond_to?(name.to_sym) || record.attributes.key?(name))
       errors.add(:name, __('%{name} already exists'), name: name)
     end

+ 26 - 0
db/migrate/20250124112935_reserved_words_per_model.rb

@@ -0,0 +1,26 @@
+# Copyright (C) 2012-2025 Zammad Foundation, https://zammad-foundation.org/
+
+class ReservedWordsPerModel < ActiveRecord::Migration[7.2]
+  def change
+    # return if it's a new setup
+    return if !Setting.exists?(name: 'system_init_done')
+
+    reserved_name = 'article'
+    sanitized_name = '_article'
+
+    # Rename column in database.
+    if ActiveRecord::Base.connection.columns(Ticket.table_name).map(&:name).include?(reserved_name)
+      ActiveRecord::Migration.rename_column(:tickets, reserved_name.to_sym, sanitized_name.to_sym)
+      Ticket.reset_column_information
+    end
+
+    # Rename the attribute itself.
+    attribute = ObjectManager::Attribute.get(
+      object: Ticket.to_app_model,
+      name:   reserved_name,
+    )
+    return if !attribute
+
+    attribute.update!(name: sanitized_name)
+  end
+end

+ 9 - 9
i18n/zammad.pot

@@ -422,11 +422,11 @@ msgstr ""
 msgid "%{model} can have full or granular access to group"
 msgstr ""
 
-#: app/models/object_manager/attribute.rb:567
+#: app/models/object_manager/attribute.rb:610
 msgid "%{name} already exists"
 msgstr ""
 
-#: app/models/object_manager/attribute.rb:556
+#: app/models/object_manager/attribute.rb:594
 msgid "%{name} is a reserved word"
 msgstr ""
 
@@ -18518,7 +18518,7 @@ msgstr ""
 msgid "at"
 msgstr ""
 
-#: app/models/object_manager/attribute.rb:551
+#: app/models/object_manager/attribute.rb:590
 msgid "at least one letter is required"
 msgstr ""
 
@@ -18527,7 +18527,7 @@ msgstr ""
 msgid "attachment,attached,enclosed,enclosure"
 msgstr ""
 
-#: app/models/object_manager/attribute.rb:580
+#: app/models/object_manager/attribute.rb:623
 msgid "attribute is not editable"
 msgstr ""
 
@@ -18561,11 +18561,11 @@ msgstr ""
 msgid "camera,image,photo,picture"
 msgstr ""
 
-#: app/models/object_manager/attribute.rb:629
+#: app/models/object_manager/attribute.rb:672
 msgid "can only be created on postgresql databases"
 msgstr ""
 
-#: app/models/object_manager/attribute.rb:622
+#: app/models/object_manager/attribute.rb:665
 msgid "can't be altered after creation (you can delete the attribute and create another with the desired value)"
 msgstr ""
 
@@ -18575,7 +18575,7 @@ msgstr ""
 msgid "can't be empty"
 msgstr ""
 
-#: app/models/object_manager/attribute.rb:542
+#: app/models/object_manager/attribute.rb:581
 msgid "can't be used because *_id and *_ids are not allowed"
 msgstr ""
 
@@ -19416,7 +19416,7 @@ msgstr ""
 msgid "now"
 msgstr ""
 
-#: app/models/object_manager/attribute.rb:548
+#: app/models/object_manager/attribute.rb:587
 msgid "only lowercase letters, numbers, and '_' are allowed"
 msgstr ""
 
@@ -19677,7 +19677,7 @@ msgstr ""
 msgid "sms"
 msgstr ""
 
-#: app/models/object_manager/attribute.rb:545
+#: app/models/object_manager/attribute.rb:584
 msgid "spaces are not allowed"
 msgstr ""
 

+ 36 - 0
spec/db/migrate/reserved_words_per_model_spec.rb

@@ -0,0 +1,36 @@
+# Copyright (C) 2012-2025 Zammad Foundation, https://zammad-foundation.org/
+
+require 'rails_helper'
+
+RSpec.describe ReservedWordsPerModel, db_strategy: :reset, type: :db_migration do
+  let(:attribute) do
+    ObjectManager::Attribute.add(
+      attributes_for(
+        :object_manager_attribute_text,
+        object_name:,
+        name:        'article'
+      ).merge(force: true)
+    )
+    ObjectManager::Attribute.migration_execute(false)
+
+    ObjectManager::Attribute.get(object: object_name.constantize.to_app_model, name: 'article')
+  end
+
+  before { attribute }
+
+  context 'when an attribute called "article" exists in the Ticket model' do
+    let(:object_name) { 'Ticket' }
+
+    it 'renames the attribute to "_article"' do
+      expect { migrate }.to change { attribute.reload.name }.from('article').to('_article')
+    end
+  end
+
+  context 'when an attribute called "article" exists in the User model' do
+    let(:object_name) { 'User' }
+
+    it 'does not rename the attribute' do
+      expect { migrate }.not_to change { attribute.reload.name }
+    end
+  end
+end

+ 10 - 4
spec/models/object_manager/attribute_spec.rb

@@ -60,11 +60,17 @@ RSpec.describe ObjectManager::Attribute, type: :model do
       end.to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: Name attribute is a reserved word')
     end
 
-    %w[destroy true false integer select drop create alter index table varchar blob date datetime timestamp url icon initials avatar permission validate subscribe unsubscribe translate search _type _doc _id id action].each do |reserved_word|
+    ObjectManager::Attribute::RESERVED_NAMES.each do |reserved_word|
       it "rejects Zammad reserved word '#{reserved_word}'" do
-        expect do
-          described_class.add attributes_for :object_manager_attribute_text, name: reserved_word
-        end.to raise_error(ActiveRecord::RecordInvalid, %r{is a reserved word})
+        expect { described_class.add attributes_for :object_manager_attribute_text, name: reserved_word }.to raise_error(ActiveRecord::RecordInvalid, %r{is a reserved word})
+      end
+    end
+
+    ObjectManager::Attribute::RESERVED_NAMES_PER_MODEL.each do |object, reserved_names|
+      reserved_names.each do |reserved_word|
+        it "rejects Zammad reserved word '#{reserved_word}' for model '#{object}'" do
+          expect { described_class.add attributes_for :object_manager_attribute_text, name: reserved_word, object_name: object }.to raise_error(ActiveRecord::RecordInvalid, %r{is a reserved word})
+        end
       end
     end