Skip to content

G
gitlab-ce
Commit b79f8931 authored by Douglas Barbosa Alexandre's avatar Douglas Barbosa Alexandre
Browse files
Options

Merge branch 'pl-revert-feature-category-label-in-api' into 'master' 

Revert "Add a feature category to requests handled by rails controllers"

Closes gitlab-development-kit#993

See merge request gitlab-org/gitlab!36097
parents f9fb61ef f867298f
Hide whitespace changes
Inline Side-by-side
Showing with 52 additions and 431 deletions
app/controllers/application_controller.rb
View file @ b79f8931
......@@ -22,7 +22,6 @@ class ApplicationController < ActionController::Base
include Impersonation
include Gitlab::Logging::CloudflareHelper
include Gitlab::Utils::StrongMemoize
include ControllerWithFeatureCategory
before_action :authenticate_user!, except: [:route_not_found]
before_action :enforce_terms!, if: :should_enforce_terms?
......
app/controllers/concerns/controller_with_feature_category.rb deleted 100644 → 0
View file @ f9fb61ef
# frozen_string_literal: true
module ControllerWithFeatureCategory
extend ActiveSupport::Concern
include Gitlab::ClassAttributes
class_methods do
def feature_category(category, config = {})
validate_config!(config)
category_config = Config.new(category, config[:only], config[:except], config[:if], config[:unless])
# Add the config to the beginning. That way, the last defined one takes precedence.
feature_category_configuration.unshift(category_config)
end
def feature_category_for_action(action)
category_config = feature_category_configuration.find { |config| config.matches?(action) }
category_config&.category || superclass_feature_category_for_action(action)
end
private
def validate_config!(config)
invalid_keys = config.keys - [:only, :except, :if, :unless]
if invalid_keys.any?
raise ArgumentError, "unknown arguments: #{invalid_keys} "
end
if config.key?(:only) && config.key?(:except)
raise ArgumentError, "cannot configure both `only` and `except`"
end
end
def feature_category_configuration
class_attributes[:feature_category_config] ||= []
end
def superclass_feature_category_for_action(action)
return unless superclass.respond_to?(:feature_category_for_action)
superclass.feature_category_for_action(action)
end
end
end
app/controllers/concerns/controller_with_feature_category/config.rb deleted 100644 → 0
View file @ f9fb61ef
# frozen_string_literal: true
module ControllerWithFeatureCategory
class Config
attr_reader :category
def initialize(category, only, except, if_proc, unless_proc)
@category = category.to_sym
@only, @except = only&.map(&:to_s), except&.map(&:to_s)
@if_proc, @unless_proc = if_proc, unless_proc
end
def matches?(action)
included?(action) && !excluded?(action) &&
if_proc?(action) && !unless_proc?(action)
end
private
attr_reader :only, :except, :if_proc, :unless_proc
def if_proc?(action)
if_proc.nil? || if_proc.call(action)
end
def unless_proc?(action)
unless_proc.present? && unless_proc.call(action)
end
def included?(action)
only.nil? || only.include?(action)
end
def excluded?(action)
except.present? && except.include?(action)
end
end
end
app/controllers/projects/merge_requests_controller.rb
View file @ b79f8931
......@@ -45,13 +45,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
around_action :allow_gitaly_ref_name_caching, only: [:index, :show, :discussions]
feature_category :source_code_management,
unless: -> (action) { action.ends_with?("_reports") }
feature_category :code_testing,
only: [:test_reports, :coverage_reports, :terraform_reports]
feature_category :accessibility_testing,
only: [:accessibility_reports]
def index
@merge_requests = @issuables
......
app/workers/concerns/worker_attributes.rb
View file @ b79f8931
......@@ -2,7 +2,6 @@
module WorkerAttributes
extend ActiveSupport::Concern
include Gitlab::ClassAttributes
# Resource boundaries that workers can declare through the
# `resource_boundary` attribute
......@@ -31,24 +30,24 @@ module WorkerAttributes
}.stringify_keys.freeze
class_methods do
def feature_category(value, *extras)
def feature_category(value)
raise "Invalid category. Use `feature_category_not_owned!` to mark a worker as not owned" if value == :not_owned
class_attributes[:feature_category] = value
worker_attributes[:feature_category] = value
end
# Special case: mark this work as not associated with a feature category
# this should be used for cross-cutting concerns, such as mailer workers.
def feature_category_not_owned!
class_attributes[:feature_category] = :not_owned
worker_attributes[:feature_category] = :not_owned
end
def get_feature_category
get_class_attribute(:feature_category)
get_worker_attribute(:feature_category)
end
def feature_category_not_owned?
get_feature_category == :not_owned
get_worker_attribute(:feature_category) == :not_owned
end
# This should be set to :high for jobs that need to be run
......@@ -62,11 +61,11 @@ module WorkerAttributes
def urgency(urgency)
raise "Invalid urgency: #{urgency}" unless VALID_URGENCIES.include?(urgency)
class_attributes[:urgency] = urgency
worker_attributes[:urgency] = urgency
end
def get_urgency
class_attributes[:urgency] || :low
worker_attributes[:urgency] || :low
end
# Set this attribute on a job when it will call to services outside of the
......@@ -74,64 +73,85 @@ module WorkerAttributes
# doc/development/sidekiq_style_guide.md#Jobs-with-External-Dependencies for
# details
def worker_has_external_dependencies!
class_attributes[:external_dependencies] = true
worker_attributes[:external_dependencies] = true
end
# Returns a truthy value if the worker has external dependencies.
# See doc/development/sidekiq_style_guide.md#Jobs-with-External-Dependencies
# for details
def worker_has_external_dependencies?
class_attributes[:external_dependencies]
worker_attributes[:external_dependencies]
end
def worker_resource_boundary(boundary)
raise "Invalid boundary" unless VALID_RESOURCE_BOUNDARIES.include? boundary
class_attributes[:resource_boundary] = boundary
worker_attributes[:resource_boundary] = boundary
end
def get_worker_resource_boundary
class_attributes[:resource_boundary] || :unknown
worker_attributes[:resource_boundary] || :unknown
end
def idempotent!
class_attributes[:idempotent] = true
worker_attributes[:idempotent] = true
end
def idempotent?
class_attributes[:idempotent]
worker_attributes[:idempotent]
end
def weight(value)
class_attributes[:weight] = value
worker_attributes[:weight] = value
end
def get_weight
class_attributes[:weight] ||
worker_attributes[:weight] ||
NAMESPACE_WEIGHTS[queue_namespace] ||
1
end
def tags(*values)
class_attributes[:tags] = values
worker_attributes[:tags] = values
end
def get_tags
Array(class_attributes[:tags])
Array(worker_attributes[:tags])
end
def deduplicate(strategy, options = {})
class_attributes[:deduplication_strategy] = strategy
class_attributes[:deduplication_options] = options
worker_attributes[:deduplication_strategy] = strategy
worker_attributes[:deduplication_options] = options
end
def get_deduplicate_strategy
class_attributes[:deduplication_strategy] ||
worker_attributes[:deduplication_strategy] ||
Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob::DEFAULT_STRATEGY
end
def get_deduplication_options
class_attributes[:deduplication_options] || {}
worker_attributes[:deduplication_options] || {}
end
protected
# Returns a worker attribute declared on this class or its parent class.
# This approach allows declared attributes to be inherited by
# child classes.
def get_worker_attribute(name)
worker_attributes[name] || superclass_worker_attributes(name)
end
private
def worker_attributes
@attributes ||= {}
end
def superclass_worker_attributes(name)
return unless superclass.include? WorkerAttributes
superclass.get_worker_attribute(name)
end
end
end
ee/app/controllers/ee/projects/merge_requests_controller.rb
View file @ b79f8931
......@@ -19,14 +19,6 @@ module EE
before_action :authorize_read_pipeline!, only: [:container_scanning_reports, :dependency_scanning_reports,
:license_scanning_reports,
:sast_reports, :secret_detection_reports, :dast_reports, :metrics_reports]
feature_category :container_scanning, only: [:container_scanning_reports]
feature_category :dependency_scanning, only: [:dependency_scanning_reports]
feature_category :license_compliance, only: [:license_scanning_reports]
feature_category :static_application_security_testing, only: [:sast_reports]
feature_category :secret_detection, only: [:secret_detection_reports]
feature_category :dynamic_application_security_testing, only: [:dast_reports]
feature_category :metrics, only: [:metrics_reports]
end
def approve
......
lib/gitlab/class_attributes.rb deleted 100644 → 0
View file @ f9fb61ef
# frozen_string_literal: true
module Gitlab
module ClassAttributes
extend ActiveSupport::Concern
class_methods do
protected
# Returns an attribute declared on this class or its parent class.
# This approach allows declared attributes to be inherited by
# child classes.
def get_class_attribute(name)
class_attributes[name] || superclass_attributes(name)
end
private
def class_attributes
@class_attributes ||= {}
end
def superclass_attributes(name)
return unless superclass.include? Gitlab::ClassAttributes
superclass.get_class_attribute(name)
end
end
end
end
lib/gitlab/metrics/web_transaction.rb
View file @ b79f8931
......@@ -32,10 +32,6 @@ module Gitlab
action = "#{controller.action_name}"
# Try to get the feature category, but don't fail when the controller is
# not an ApplicationController.
feature_category = controller.class.try(:feature_category_for_action, action).to_s
# Devise exposes a method called "request_format" that does the below.
# However, this method is not available to all controllers (e.g. certain
# Doorkeeper controllers). As such we use the underlying code directly.
......@@ -49,7 +45,7 @@ module Gitlab
action = "#{action}.#{suffix}"
end
{ controller: controller.class.name, action: action, feature_category: feature_category }
{ controller: controller.class.name, action: action }
end
def labels_from_endpoint
......@@ -65,10 +61,7 @@ module Gitlab
if route
path = endpoint_paths_cache[route.request_method][route.path]
# Feature categories will be added for grape endpoints in
# https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/462
{ controller: 'Grape', action: "#{route.request_method} #{path}", feature_category: '' }
{ controller: 'Grape', action: "#{route.request_method} #{path}" }
end
end
......
spec/controllers/concerns/controller_with_feature_category/config_spec.rb deleted 100644 → 0
View file @ f9fb61ef
# frozen_string_literal: true
require "fast_spec_helper"
require "rspec-parameterized"
require_relative "../../../../app/controllers/concerns/controller_with_feature_category/config"
RSpec.describe ControllerWithFeatureCategory::Config do
describe "#matches?" do
using RSpec::Parameterized::TableSyntax
where(:only_actions, :except_actions, :if_proc, :unless_proc, :test_action, :expected) do
nil | nil | nil | nil | "action" | true
[:included] | nil | nil | nil | "action" | false
[:included] | nil | nil | nil | "included" | true
nil | [:excluded] | nil | nil | "excluded" | false
nil | nil | true | nil | "action" | true
[:included] | nil | true | nil | "action" | false
[:included] | nil | true | nil | "included" | true
nil | [:excluded] | true | nil | "excluded" | false
nil | nil | false | nil | "action" | false
[:included] | nil | false | nil | "action" | false
[:included] | nil | false | nil | "included" | false
nil | [:excluded] | false | nil | "excluded" | false
nil | nil | nil | true | "action" | false
[:included] | nil | nil | true | "action" | false
[:included] | nil | nil | true | "included" | false
nil | [:excluded] | nil | true | "excluded" | false
nil | nil | nil | false | "action" | true
[:included] | nil | nil | false | "action" | false
[:included] | nil | nil | false | "included" | true
nil | [:excluded] | nil | false | "excluded" | false
nil | nil | true | false | "action" | true
[:included] | nil | true | false | "action" | false
[:included] | nil | true | false | "included" | true
nil | [:excluded] | true | false | "excluded" | false
nil | nil | false | true | "action" | false
[:included] | nil | false | true | "action" | false
[:included] | nil | false | true | "included" | false
nil | [:excluded] | false | true | "excluded" | false
end
with_them do
let(:config) do
if_to_proc = if_proc.nil? ? nil : -> (_) { if_proc }
unless_to_proc = unless_proc.nil? ? nil : -> (_) { unless_proc }
described_class.new(:category, only_actions, except_actions, if_to_proc, unless_to_proc)
end
specify { expect(config.matches?(test_action)).to be(expected) }
end
end
end
spec/controllers/concerns/controller_with_feature_category_spec.rb deleted 100644 → 0
View file @ f9fb61ef
# frozen_string_literal: true
require 'fast_spec_helper'
require_relative "../../../app/controllers/concerns/controller_with_feature_category"
require_relative "../../../app/controllers/concerns/controller_with_feature_category/config"
RSpec.describe ControllerWithFeatureCategory do
describe ".feature_category_for_action" do
let(:base_controller) do
Class.new do
include ControllerWithFeatureCategory
end
end
let(:controller) do
Class.new(base_controller) do
feature_category :baz
feature_category :foo, except: %w(update edit)
feature_category :bar, only: %w(index show)
feature_category :quux, only: %w(destroy)
feature_category :quuz, only: %w(destroy)
end
end
let(:subclass) do
Class.new(controller) do
feature_category :qux, only: %w(index)
end
end
it "is nil when nothing was defined" do
expect(base_controller.feature_category_for_action("hello")).to be_nil
end
it "returns the expected category", :aggregate_failures do
expect(controller.feature_category_for_action("update")).to eq(:baz)
expect(controller.feature_category_for_action("hello")).to eq(:foo)
expect(controller.feature_category_for_action("index")).to eq(:bar)
end
it "returns the closest match for categories defined in subclasses" do
expect(subclass.feature_category_for_action("index")).to eq(:qux)
expect(subclass.feature_category_for_action("show")).to eq(:bar)
end
it "returns the last defined feature category when multiple match" do
expect(controller.feature_category_for_action("destroy")).to eq(:quuz)
end
it "raises an error when using including and excluding the same action" do
expect do
Class.new(base_controller) do
feature_category :hello, only: [:world], except: [:world]
end
end.to raise_error(%r(cannot configure both `only` and `except`))
end
it "raises an error when using unknown arguments" do
expect do
Class.new(base_controller) do
feature_category :hello, hello: :world
end
end.to raise_error(%r(unknown arguments))
end
end
end
spec/controllers/every_controller_spec.rb deleted 100644 → 0
View file @ f9fb61ef
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe "Every controller" do
context "feature categories" do
let_it_be(:feature_categories) do
YAML.load_file(Rails.root.join('config', 'feature_categories.yml')).map(&:to_sym).to_set
end
let_it_be(:controller_actions) do
# This will return tuples of all controller actions defined in the routes
# Only for controllers inheriting ApplicationController
# Excluding controllers from gems (OAuth, Sidekiq)
Rails.application.routes.routes
.map { |route| route.required_defaults.presence }
.compact
.select { |route| route[:controller].present? && route[:action].present? }
.map { |route| [constantize_controller(route[:controller]), route[:action]] }
.reject { |route| route.first.nil? || !route.first.include?(ControllerWithFeatureCategory) }
end
let_it_be(:routes_without_category) do
controller_actions.map do |controller, action|
"#{controller}##{action}" unless controller.feature_category_for_action(action)
end.compact
end
it "has feature categories" do
pending("We'll work on defining categories for all controllers: "\
"https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/463")
expect(routes_without_category).to be_empty, "#{routes_without_category.first(10)} did not have a category"
end
it "completed controllers don't get new routes without categories" do
completed_controllers = [Projects::MergeRequestsController].map(&:to_s)
newly_introduced_missing_category = routes_without_category.select do |route|
completed_controllers.any? { |controller| route.start_with?(controller) }
end
expect(newly_introduced_missing_category).to be_empty
end
it "recognizes the feature categories" do
routes_unknown_category = controller_actions.map do |controller, action|
used_category = controller.feature_category_for_action(action)
next unless used_category
next if used_category == :not_owned
["#{controller}##{action}", used_category] unless feature_categories.include?(used_category)
end.compact
expect(routes_unknown_category).to be_empty, "#{routes_unknown_category.first(10)} had an unknown category"
end
it "doesn't define or exclude categories on removed actions", :aggregate_failures do
controller_actions.group_by(&:first).each do |controller, controller_action|
existing_actions = controller_action.map(&:last)
used_actions = actions_defined_in_feature_category_config(controller)
non_existing_used_actions = used_actions - existing_actions
expect(non_existing_used_actions).to be_empty,
"#{controller} used #{non_existing_used_actions} to define feature category, but the route does not exist"
end
end
end
def constantize_controller(name)
"#{name.camelize}Controller".constantize
rescue NameError
nil # some controllers, like the omniauth ones are dynamic
end
def actions_defined_in_feature_category_config(controller)
feature_category_configs = controller.send(:class_attributes)[:feature_category_config]
feature_category_configs.map do |config|
Array(config.send(:only)) + Array(config.send(:except))
end.flatten.uniq.map(&:to_s)
end
end
spec/lib/gitlab/class_attributes_spec.rb deleted 100644 → 0
View file @ f9fb61ef
# frozen_string_literal: true
require 'fast_spec_helper'
RSpec.describe Gitlab::ClassAttributes do
let(:klass) do
Class.new do
include Gitlab::ClassAttributes
def self.get_attribute(name)
get_class_attribute(name)
end
def self.set_attribute(name, value)
class_attributes[name] = value
end
end
end
let(:subclass) { Class.new(klass) }
describe ".get_class_attribute" do
it "returns values set on the class" do
klass.set_attribute(:foo, :bar)
expect(klass.get_attribute(:foo)).to eq(:bar)
end
it "returns values set on a superclass" do
klass.set_attribute(:foo, :bar)
expect(subclass.get_attribute(:foo)).to eq(:bar)
end
it "returns values from the subclass over attributes from a superclass" do
klass.set_attribute(:foo, :baz)
subclass.set_attribute(:foo, :bar)
expect(subclass.get_attribute(:foo)).to eq(:bar)
end
end
end
spec/lib/gitlab/metrics/web_transaction_spec.rb
View file @ b79f8931
......@@ -70,9 +70,6 @@ RSpec.describe Gitlab::Metrics::WebTransaction do
end
describe '#labels' do
let(:request) { double(:request, format: double(:format, ref: :html)) }
let(:controller_class) { double(:controller_class, name: 'TestController') }
context 'when request goes to Grape endpoint' do
before do
route = double(:route, request_method: 'GET', path: '/:version/projects/:id/archive(.:format)')
......@@ -80,9 +77,8 @@ RSpec.describe Gitlab::Metrics::WebTransaction do
env['api.endpoint'] = endpoint
end
it 'provides labels with the method and path of the route in the grape endpoint' do
expect(transaction.labels).to eq({ controller: 'Grape', action: 'GET /projects/:id/archive', feature_category: '' })
expect(transaction.labels).to eq({ controller: 'Grape', action: 'GET /projects/:id/archive' })
end
it 'does not provide labels if route infos are missing' do
......@@ -96,21 +92,24 @@ RSpec.describe Gitlab::Metrics::WebTransaction do
end
context 'when request goes to ActionController' do
let(:request) { double(:request, format: double(:format, ref: :html)) }
before do
controller = double(:controller, class: controller_class, action_name: 'show', request: request)
klass = double(:klass, name: 'TestController')
controller = double(:controller, class: klass, action_name: 'show', request: request)
env['action_controller.instance'] = controller
end
it 'tags a transaction with the name and action of a controller' do
expect(transaction.labels).to eq({ controller: 'TestController', action: 'show', feature_category: '' })
expect(transaction.labels).to eq({ controller: 'TestController', action: 'show' })
end
context 'when the request content type is not :html' do
let(:request) { double(:request, format: double(:format, ref: :json)) }
it 'appends the mime type to the transaction action' do
expect(transaction.labels).to eq({ controller: 'TestController', action: 'show.json', feature_category: '' })
expect(transaction.labels).to eq({ controller: 'TestController', action: 'show.json' })
end
end
......@@ -118,31 +117,11 @@ RSpec.describe Gitlab::Metrics::WebTransaction do
let(:request) { double(:request, format: double(:format, ref: 'http://example.com')) }
it 'does not append the MIME type to the transaction action' do
expect(transaction.labels).to eq({ controller: 'TestController', action: 'show', feature_category: '' })
end
end
context 'when the feature category is known' do
it 'includes it in the feature category label' do
expect(controller_class).to receive(:feature_category_for_action).with('show').and_return(:source_code_management)
expect(transaction.labels).to eq({ controller: 'TestController', action: 'show', feature_category: "source_code_management" })
expect(transaction.labels).to eq({ controller: 'TestController', action: 'show' })
end
end
end
it 'returns the same labels for API and controller requests' do
route = double(:route, request_method: 'GET', path: '/:version/projects/:id/archive(.:format)')
endpoint = double(:endpoint, route: route)
api_env = { 'api.endpoint' => endpoint }
api_labels = described_class.new(api_env).labels
controller = double(:controller, class: controller_class, action_name: 'show', request: request)
controller_env = { 'action_controller.instance' => controller }
controller_labels = described_class.new(controller_env).labels
expect(api_labels.keys).to contain_exactly(*controller_labels.keys)
end
it 'returns no labels when no route information is present in env' do
expect(transaction.labels).to eq({})
end
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7