极狐 GitLab

陷阱

本指南旨在记录贡献者在开发 极狐GitLab CE 和 EE 过程中可能遇到或应避免的潜在“陷阱”。

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

Omnibus 极狐GitLab 已在资源编译后删除了 app/assets 目录。ee/app/assets、vendor/assets 目录也已删除。

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

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

# 该文件不存在，读取将失败并显示：
# Errno::ENOENT：没有这样的文件或目录 @ rb_sysopen
File.read(file)

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

思考以下工厂：

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

考虑以下 API spec：

ruby
1require 'spec_helper'
2
3RSpec.describe API::Labels do
4  it '创建第一个标签' do
5    create(:label)
6
7    get api("/projects/#{project.id}/labels", user)
8
9    expect(response).to have_gitlab_http_status(:ok)
10    expect(json_response.first['name']).to eq('标签1')
11  end
12
13  it '创建第二个标签' do
14    create(:label)
15
16    get api("/projects/#{project.id}/labels", user)
17
18    expect(response).to have_gitlab_http_status(:ok)
19    expect(json_response.first['name']).to eq('标签1')
20  end
21end

当运行时，这个 spec 并不会产生我们期望的结果：

shell
11) API::Labels 创建第二个标签
2   Failure/Error: expect(json_response.first['name']).to eq('标签1')
3
4     expected: "标签1"
5          got: "标签2"
6
7     (compared using ==)

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

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

解决方案#

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

例如，使用我们的 :label 工厂，编写 create(:label, title: 'foo') 是可以的，但 create(:label, title: '标签1') 不可以。

以下是修复后的 API spec：

ruby
1require 'spec_helper'
2
3RSpec.describe API::Labels do
4  it '创建第一个标签' do
5    create(:label, title: 'foo')
6
7    get api("/projects/#{project.id}/labels", user)
8
9    expect(response).to have_gitlab_http_status(:ok)
10    expect(json_response.first['name']).to eq('foo')
11  end
12
13  it '创建第二个标签' do
14    create(:label, title: 'bar')
15
16    get api("/projects/#{project.id}/labels", user)
17
18    expect(response).to have_gitlab_http_status(:ok)
19    expect(json_response.first['name']).to eq('bar')
20  end
21end

避免在 RSpec 中使用 expect_any_instance_of 或 allow_any_instance_of#

为什么#

  • 因为它未隔离，因此有时可能会被破坏。

  • 因为它在我们希望 stub 的方法定义在 prepended 模块中时不起作用，而这在 EE 中是非常可能的情况。我们可能会看到类似以下错误：

    plaintext
    1.1) Failure/Error: expect_any_instance_of(ApplicationSetting).to receive_messages(messages)
     使用 `any_instance` 去 stub 定义在 prepended 模块 (EE::ApplicationSetting) 上的方法 (elasticsearch_indexing) 是不受支持的。

替代方案#

相反，请使用以下任一方法：

  • expect_next_instance_of
  • allow_next_instance_of
  • expect_next_found_instance_of
  • allow_next_found_instance_of

例如：

ruby
# 不要这么做：
expect_any_instance_of(Project).to receive(:add_import_job)

# 不要这么做：
allow_any_instance_of(Project).to receive(:add_import_job)

我们可以这样写：

ruby
1# 可以这么做：
2expect_next_instance_of(Project) do |project|
3  expect(project).to receive(:add_import_job)
4end
5
6# 可以这么做：
7allow_next_instance_of(Project) do |project|
8  allow(project).to receive(:add_import_job)
9end
10
11# 可以这么做：
12expect_next_found_instance_of(Project) do |project|
13  expect(project).to receive(:add_import_job)
14end
15
16# 可以这么做：
17allow_next_found_instance_of(Project) do |project|
18  allow(project).to receive(:add_import_job)
19end

由于 Active Record 不会调用模型类的 .new 方法来实例化对象，因此应使用 expect_next_found_instance_of 或 allow_next_found_instance_of mock 辅助方法在 Active Record 查询和 finder 方法返回的对象上设置 mock。

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

ruby
1expect_next_found_2_instances_of(Project) do |project|
2  expect(project).to receive(:add_import_job)
3end
4
5allow_next_found_2_instances_of(Project) do |project|
6  allow(project).to receive(:add_import_job)
7end

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

ruby
# 可以这么做：
expect_next_instance_of(MergeRequests::RefreshService, project, user) do |refresh_service|
  expect(refresh_service).to receive(:execute).with(oldrev, newrev, ref)
end

这将期望如下：

ruby
# 上面的期望：
refresh_service = MergeRequests::RefreshService.new(project, user)
refresh_service.execute(oldrev, newrev, ref)

不要 rescue Exception#

请参阅“Why is it bad style to rescue Exception => e in Ruby?”。

此规则由 RuboCop 自动强制执行。

不要在视图中使用内嵌 JavaScript#

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

我们已在一个初始化器中移除了这两个过滤器。

延伸阅读#

存储不需要预编译的资源#

需要向用户提供的资源存储在 app/assets 目录下，该目录稍后会进行预编译并放置在 public/ 目录中。

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

ruby
1support_bot = Users::Internal.in_organization(organization).support_bot
2
3# 从 `app/assets` 文件夹访问文件
4support_bot.avatar = Rails.root.join('app', 'assets', 'images', 'bot_avatars', 'support_bot.png').open
5
6support_bot.save!

尽管上述代码在本地环境中有效，但在生产安装中会出错，因为 app/assets 文件夹未被包含。

解决方案#

替代方案是 lib/assets 文件夹。如果你需要将符合以下条件的资源（如图片）添加到仓库中，则使用它：

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

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

参考 MR：!37671

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

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

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

ruby
group.users.destroy(id)

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

ruby
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 关联，例如：

ruby
1class Group < Namespace
2  has_many :group_members, -> { where(requested_at: nil).where.not(members: { access_level: 极狐Gitlab::Access::MINIMAL_ACCESS }) }, dependent: :destroy, as: :source
3
4  has_many :users, through: :group_members
5
6  def users
7    super.where(admin: false)
8  end
9end

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

ruby
group.users.destroy(id)

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

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

更多信息，请参阅议题 424536。