Help us learn about your current experience with the documentation. Take the survey.

常见陷阱

本指南旨在记录贡献者在开发 GitLab CE 和 EE 时可能遇到的或应避免的潜在"陷阱"。

不要从 app/assets 目录读取文件

Omnibus GitLab 在资源编译后已经移除了 app/assets 目录。 ee/app/assets、vendor/assets 目录也同样被移除。

这意味着在通过 Omnibus 安装的 GitLab 实例中，从该目录读取文件会失败：

file = Rails.root.join('app/assets/images/logo.svg')

# This file does not exist, read will fail with:
# Errno::ENOENT: No such file or directory @ rb_sysopen
File.read(file)

不要对序列生成属性的绝对值进行断言

考虑以下 factory：

FactoryBot.define do
  factory :label do
    sequence(:title) { |n| "label#{n}" }
  end
end

考虑以下 API spec：

require 'spec_helper'

RSpec.describe API::Labels do
  it 'creates a first label' do
    create(:label)

    get api("/projects/#{project.id}/labels", user)

    expect(response).to have_gitlab_http_status(:ok)
    expect(json_response.first['name']).to eq('label1')
  end

  it 'creates a second label' do
    create(:label)

    get api("/projects/#{project.id}/labels", user)

    expect(response).to have_gitlab_http_status(:ok)
    expect(json_response.first['name']).to eq('label1')
  end
end

运行时，这个 spec 并没有达到我们预期的效果：

1) API::API reproduce sequence issue creates a second label
   Failure/Error: expect(json_response.first['name']).to eq('label1')

     expected: "label1"
          got: "label2"

     (compared using ==)

这是因为 FactoryBot 序列不会为每个示例重置。

请记住，序列生成的值仅用于在使用 factory 时避免必须显式设置具有唯一性约束的属性。

解决方案

如果你要对序列生成属性的值进行断言，应该显式设置它。此外，你设置的值不应与序列模式匹配。

例如，使用我们的 :label factory，编写 create(:label, title: 'foo') 是可以的，但 create(:label, title: 'label1') 不行。

以下是修复后的 API spec：

require 'spec_helper'

RSpec.describe API::Labels do
  it 'creates a first label' do
    create(:label, title: 'foo')

    get api("/projects/#{project.id}/labels", user)

    expect(response).to have_gitlab_http_status(:ok)
    expect(json_response.first['name']).to eq('foo')
  end

  it 'creates a second label' do
    create(:label, title: 'bar')

    get api("/projects/#{project.id}/labels", user)

    expect(response).to have_gitlab_http_status(:ok)
    expect(json_response.first['name']).to eq('bar')
  end
end

避免在 RSpec 中使用 expect_any_instance_of 或 allow_any_instance_of

为什么

  • 因为它不是隔离的，所以有时可能会出错。
  • 因为当我们想要 stub 的方法是在 prepended 模块中定义时，它不起作用，这在 EE 中很可能发生。我们可能会看到如下错误：
1.1) Failure/Error: expect_any_instance_of(ApplicationSetting).to receive_messages(messages)
       Using `any_instance` to stub a method (elasticsearch_indexing) that has been defined on a prepended module (EE::ApplicationSetting) is not supported.

替代方案

改用以下任一方法：

  • expect_next_instance_of
  • allow_next_instance_of
  • expect_next_found_instance_of
  • allow_next_found_instance_of

例如：

# Don't do this:
expect_any_instance_of(Project).to receive(:add_import_job)

# Don't do this:
allow_any_instance_of(Project).to receive(:add_import_job)

我们可以这样写：

# Do this:
expect_next_instance_of(Project) do |project|
  expect(project).to receive(:add_import_job)
end

# Do this:
allow_next_instance_of(Project) do |project|
  allow(project).to receive(:add_import_job)
end

# Do this:
expect_next_found_instance_of(Project) do |project|
  expect(project).to receive(:add_import_job)
end

# Do this:
allow_next_found_instance_of(Project) do |project|
  allow(project).to receive(:add_import_job)
end

由于 Active Record 不是在模型类上调用 .new 方法来实例化对象，你应该使用 expect_next_found_instance_of 或 allow_next_found_instance_of mock helpers 来设置 Active Record 查询和查找方法返回的对象的 mock。

也可以使用 expect_next_found_(number)_instances_of 和 allow_next_found_(number)_instances_of helpers 来为同一 Active Record 模型的多个实例设置 mock 和期望，如下所示：

expect_next_found_2_instances_of(Project) do |project|
  expect(project).to receive(:add_import_job)
end

allow_next_found_2_instances_of(Project) do |project|
  allow(project).to receive(:add_import_job)
end

如果我们还想用一些特定的参数初始化实例，也可以这样传递：

# Do this:
expect_next_instance_of(MergeRequests::RefreshService, project, user) do |refresh_service|
  expect(refresh_service).to receive(:execute).with(oldrev, newrev, ref)
end

这将期望以下内容：

# Above expects:
refresh_service = MergeRequests::RefreshService.new(project, user)
refresh_service.execute(oldrev, newrev, ref)

不要 rescue Exception

请参阅“为什么在 Ruby 中 rescue Exception => e 是不好的风格？”。

这个规则由 RuboCop 自动强制执行。

不要在视图中使用内联 JavaScript

使用内联 :javascript Haml filters 会带来性能开销。使用内联 JavaScript 不是构建代码的好方法，应该避免。

我们在一个 initializer 中移除了这两个 filters。

进一步阅读

存储不需要预编译的资源

需要提供给用户的资源存储在 app/assets 目录下，之后会被预编译并放置在 public/ 目录中。

但是，你无法从应用程序代码中访问 app/assets 内任何文件的内容，因为我们没有在生产安装中包含该文件夹，这是一种节省空间的措施。

support_bot = Users::Internal.support_bot

# accessing a file from the `app/assets` folder
support_bot.avatar = Rails.root.join('app', 'assets', 'images', 'bot_avatars', 'support_bot.png').open

support_bot.save!

虽然上面的代码在本地环境中可以工作，但在生产安装中会出错，因为 app/assets 文件夹没有被包含。

解决方案

替代方案是 lib/assets 文件夹。如果你需要向仓库添加满足以下条件的资源（如图像），请使用它：

  • 资源不需要直接提供给用户（因此不需要预编译）。
  • 资源确实需要通过应用程序代码访问。

简而言之：

  • 使用 app/assets 存储任何需要预编译并提供给最终用户的资源。
  • 使用 lib/assets 存储任何不需要直接提供给最终用户但仍需要被应用程序代码访问的资源。

MR for reference: !37671

不要覆盖 has_many through: 或 has_one through: 关联

带有 :through 选项的关联不应该被覆盖，因为我们可能会意外销毁错误的对象。

这是因为 destroy() 方法在作用于 has_many through: 和 has_one through: 关联时的行为不同。

group.users.destroy(id)

上面的代码示例看起来像是我们在销毁一个 User 记录，但在幕后，它实际上是在销毁一个 Member 记录。这是因为 users 关联在 Group 上被定义为 has_many through: 关联：

class Group < Namespace
  has_many :group_members, -> { where(requested_at: nil).where.not(members: { access_level: Gitlab::Access::MINIMAL_ACCESS }) }, dependent: :destroy, as: :source

  has_many :users, through: :group_members
end

并且 Rails 在对这类关联使用 destroy() 时有以下行为：

如果使用了 :through 选项，则销毁的是连接记录，而不是对象本身。

这就是为什么正在销毁的是连接 User 和 Group 的连接记录 Member。

现在，如果我们覆盖 users 关联，就像这样：

class Group < Namespace
  has_many :group_members, -> { where(requested_at: nil).where.not(members: { access_level: Gitlab::Access::MINIMAL_ACCESS }) }, dependent: :destroy, as: :source

  has_many :users, through: :group_members

  def users
    super.where(admin: false)
  end
end

被覆盖的方法现在改变了上述 destroy() 的行为，因此如果我们执行

group.users.destroy(id)

将会删除一个 User 记录，这可能导致数据丢失。

简而言之，覆盖 has_many through: 或 has_one through: 关联可能很危险。为了防止这种情况发生，我们在 !131455 中引入了一个自动化检查。

For more information, see issue 424536.