diff --git a/app/presenters/README.md b/app/presenters/README.md index 62aec4fc8a201f7325d41989285fe95c828851a7..dfd1818f97dd7f956df1cb3428e7a7bac726dac9 100644 --- a/app/presenters/README.md +++ b/app/presenters/README.md @@ -66,14 +66,15 @@ we gain the following benefits: ### Presenter definition -Every presenter should inherit from `Gitlab::View::Presenter::Simple`, which -provides a `.presents` the method which allows you to define an accessor for the +If you need a presenter class that has only necessary interfaces for the view-related context, +inherit from `Gitlab::View::Presenter::Simple`. +It provides a `.presents` the method which allows you to define an accessor for the presented object. It also includes common helpers like `Gitlab::Routing` and `Gitlab::Allowable`. ```ruby class LabelPresenter < Gitlab::View::Presenter::Simple - presents :label + presents ::Label, as: :label def text_color label.color.to_s @@ -85,13 +86,14 @@ class LabelPresenter < Gitlab::View::Presenter::Simple end ``` -In some cases, it can be more practical to transparently delegate all missing -method calls to the presented object, in these cases, you can make your -presenter inherit from `Gitlab::View::Presenter::Delegated`: +If you need a presenter class that delegates missing method calls to the presented object, +inherit from `Gitlab::View::Presenter::Delegated`. +This is more like an "extension" in the sense that the produced object is going to have +all of interfaces of the presented object **AND** all of the interfaces in the presenter class: ```ruby class LabelPresenter < Gitlab::View::Presenter::Delegated - presents :label + presents ::Label, as: :label def text_color # color is delegated to label @@ -152,3 +154,82 @@ You can also present the model in the view: %div{ class: label.text_color } = render partial: label, label: label ``` + +### Validate accidental overrides + +We use presenters in many places, such as Controller, Haml, GraphQL/Rest API, +it's very handy to extend the core/backend logic of Active Record models, +however, there is a risk that it accidentally overrides important logic. + +For example, [this production incident](https://gitlab.com/gitlab-com/gl-infra/production/-/issues/5498) +was caused by [including `ActionView::Helpers::UrlHelper` in a presenter](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/69537/diffs#4b581cff00ef3cc9780efd23682af383de302e7d_3_3). +The `tag` accesor in `Ci::Build` was accidentally overridden by `ActionView::Helpers::TagHelper#tag`, +and as a conseuqence, a wrong `tag` value was persited into database. + +Starting from GitLab 14.4, we validate the presenters (specifically all of the subclasses of `Gitlab::View::Presenter::Delegated`) +that they do not accidentally override core/backend logic. In such case, a pipeline in merge requests fails with an error message, +here is an example: + +```plaintext +We've detected that a presetner is overriding a specific method(s) on a subject model. +There is a risk that it accidentally modifies the backend/core logic that leads to production incident. +Please follow https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/presenters/README.md#validate-accidental-overrides +to resolve this error with caution. + +Here are the conflict details. + +- Ci::PipelinePresenter#tag is overriding Ci::Pipeline#tag. delegator_location: /devkitkat/services/rails/cache/ruby/2.7.0/gems/actionview-6.1.3.2/lib/action_view/helpers/tag_helper.rb:271 original_location: /devkitkat/services/rails/cache/ruby/2.7.0/gems/activemodel-6.1.3.2/lib/active_model/attribute_methods.rb:254 +``` + +Here are the potential solutions: + +- If the conflict happens on an instance method in the presenter: + - If you intend to override the core/backend logic, define `delegator_override <method-name>` on top of the conflicted method. + This explicitly adds the method to an allowlist. + - If you do NOT intend to override the core/backend logic, rename the method name in the presenter. +- If the conflict happens on an included module in the presenter, remove the module from the presenter and find a workaround. + +### How to use the `Gitlab::Utils::DelegatorOverride` validator + +If a presenter class inhertis from `Gitlab::View::Presenter::Delegated`, +you should define what object class is presented: + +```ruby +class WebHookLogPresenter < Gitlab::View::Presenter::Delegated + presents ::WebHookLog, as: :web_hook_log # This defines that the presenter presents `WebHookLog` Active Record model. +``` + +These presenters are validated not to accidentaly override the methods in the presented object. +You can run the validation locally with: + +```shell +bundle exec rake lint:static_verification +``` + +To add a method to an allowlist, use `delegator_override`. For example: + +```ruby +class VulnerabilityPresenter < Gitlab::View::Presenter::Delegated + presents ::Vulnerability, as: :vulnerability + + delegator_override :description # This adds the `description` method to an allowlist that the override is intentional. + def description + vulnerability.description || finding.description + end +``` + +To add methods of a module to an allowlist, use `delegator_override_with`. For example: + +```ruby +module Ci + class PipelinePresenter < Gitlab::View::Presenter::Delegated + include Gitlab::Utils::StrongMemoize + include ActionView::Helpers::UrlHelper + + delegator_override_with Gitlab::Utils::StrongMemoize # TODO: Remove `Gitlab::Utils::StrongMemoize` inclusion as it's duplicate + delegator_override_with ActionView::Helpers::TagHelper # TODO: Remove `ActionView::Helpers::UrlHelper` inclusion as it overrides `Ci::Pipeline#tag` +``` + +Keep in mind that if you use `delegator_override_with`, +there is a high chance that you're doing **something wrong**. +Read the [Validate Accidental Overrides](#validate-accidental-overrides) for more information. diff --git a/app/presenters/alert_management/alert_presenter.rb b/app/presenters/alert_management/alert_presenter.rb index 93983c989c8957cfe1150122fe88162befc9743a..86fe985927169de2aeb0c4c574840f883f379c8c 100644 --- a/app/presenters/alert_management/alert_presenter.rb +++ b/app/presenters/alert_management/alert_presenter.rb @@ -5,6 +5,9 @@ class AlertPresenter < Gitlab::View::Presenter::Delegated include IncidentManagement::Settings include ActionView::Helpers::UrlHelper + presents ::AlertManagement::Alert + delegator_override_with Gitlab::Utils::StrongMemoize # TODO: Remove `Gitlab::Utils::StrongMemoize` inclusion as it's duplicate + MARKDOWN_LINE_BREAK = " \n" HORIZONTAL_LINE = "\n\n---\n\n" @@ -29,6 +32,7 @@ def start_time started_at&.strftime('%d %B %Y, %-l:%M%p (%Z)') end + delegator_override :details_url def details_url details_project_alert_management_url(project, alert.iid) end diff --git a/app/presenters/award_emoji_presenter.rb b/app/presenters/award_emoji_presenter.rb index 98713855d35ddc894c85f8ef570ff1a5c3bcdb2f..8a7b58e0aba20471930484c79f3b541d519826f0 100644 --- a/app/presenters/award_emoji_presenter.rb +++ b/app/presenters/award_emoji_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AwardEmojiPresenter < Gitlab::View::Presenter::Delegated - presents :award_emoji + presents ::AwardEmoji, as: :award_emoji def description as_emoji['description'] diff --git a/app/presenters/blob_presenter.rb b/app/presenters/blob_presenter.rb index ecc16e2840c37c4334fac8c5e9a98a5bed0eee8a..c198859aa4c8088b7990a133895bb5698aff9d8b 100644 --- a/app/presenters/blob_presenter.rb +++ b/app/presenters/blob_presenter.rb @@ -7,7 +7,7 @@ class BlobPresenter < Gitlab::View::Presenter::Delegated include TreeHelper include ChecksCollaboration - presents :blob + presents ::Blob, as: :blob def highlight(to: nil, plain: nil) load_all_blob_data diff --git a/app/presenters/blobs/unfold_presenter.rb b/app/presenters/blobs/unfold_presenter.rb index 487c6fe0757771778a9fc562197ba0d950ab966b..b921b5bf670a520ebc0be4eceba2cc11f6dd5f88 100644 --- a/app/presenters/blobs/unfold_presenter.rb +++ b/app/presenters/blobs/unfold_presenter.rb @@ -6,6 +6,8 @@ class UnfoldPresenter < BlobPresenter include ActiveModel::AttributeAssignment include Gitlab::Utils::StrongMemoize + presents ::Blob + attribute :full, :boolean, default: false attribute :since, :integer, default: 1 attribute :to, :integer, default: 1 diff --git a/app/presenters/board_presenter.rb b/app/presenters/board_presenter.rb index d7cecd44dd7f9c8e1a0ce51b2a36ce4d9c12569d..bb3e96b8fafc33651be5c53ba9c5af9ee734d9f9 100644 --- a/app/presenters/board_presenter.rb +++ b/app/presenters/board_presenter.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true class BoardPresenter < Gitlab::View::Presenter::Delegated - presents :board + presents ::Board, as: :board end diff --git a/app/presenters/ci/bridge_presenter.rb b/app/presenters/ci/bridge_presenter.rb index 724e10c26c33ab397687d832e82a20531f266034..a62d7cdbbd43d644db743a3947907f50e205c861 100644 --- a/app/presenters/ci/bridge_presenter.rb +++ b/app/presenters/ci/bridge_presenter.rb @@ -2,6 +2,9 @@ module Ci class BridgePresenter < ProcessablePresenter + presents ::Ci::Bridge + + delegator_override :detailed_status def detailed_status @detailed_status ||= subject.detailed_status(user) end diff --git a/app/presenters/ci/build_metadata_presenter.rb b/app/presenters/ci/build_metadata_presenter.rb index 4871bb3a91991d12872018f825e1cf4ec25c4a9e..2f559adf77dc11a66173090fb3b72f7f1c53baff 100644 --- a/app/presenters/ci/build_metadata_presenter.rb +++ b/app/presenters/ci/build_metadata_presenter.rb @@ -9,8 +9,9 @@ class BuildMetadataPresenter < Gitlab::View::Presenter::Delegated job_timeout_source: 'job' }.freeze - presents :metadata + presents ::Ci::BuildMetadata, as: :metadata + delegator_override :timeout_source def timeout_source return unless metadata.timeout_source? diff --git a/app/presenters/ci/build_presenter.rb b/app/presenters/ci/build_presenter.rb index 06ed6791bb745bfe62f7098eac9f7b46e6e48822..65e1c80085f531db0bed7c9ab2713ab9a8a234f0 100644 --- a/app/presenters/ci/build_presenter.rb +++ b/app/presenters/ci/build_presenter.rb @@ -2,6 +2,8 @@ module Ci class BuildPresenter < ProcessablePresenter + presents ::Ci::Build + def erased_by_user? # Build can be erased through API, therefore it does not have # `erased_by` user assigned in that case. diff --git a/app/presenters/ci/group_variable_presenter.rb b/app/presenters/ci/group_variable_presenter.rb index 99011150c84e0f9166e93db89aaf5534623ddf9f..dea9a42b62243f825b20c0487308068b466b09c8 100644 --- a/app/presenters/ci/group_variable_presenter.rb +++ b/app/presenters/ci/group_variable_presenter.rb @@ -2,7 +2,7 @@ module Ci class GroupVariablePresenter < Gitlab::View::Presenter::Delegated - presents :variable + presents ::Ci::GroupVariable, as: :variable def placeholder 'GROUP_VARIABLE' diff --git a/app/presenters/ci/legacy_stage_presenter.rb b/app/presenters/ci/legacy_stage_presenter.rb index d5c21baba287b74f3728637eafd2437d38369082..c803abfab6a1cf3a471ede5bec81f36511741492 100644 --- a/app/presenters/ci/legacy_stage_presenter.rb +++ b/app/presenters/ci/legacy_stage_presenter.rb @@ -2,7 +2,7 @@ module Ci class LegacyStagePresenter < Gitlab::View::Presenter::Delegated - presents :legacy_stage + presents ::Ci::LegacyStage, as: :legacy_stage def latest_ordered_statuses preload_statuses(legacy_stage.statuses.latest_ordered) diff --git a/app/presenters/ci/pipeline_presenter.rb b/app/presenters/ci/pipeline_presenter.rb index 82f00f746924ca7239ad2a45605e59af136ef6cc..e0cb899c9d398cf6053cf5880d24dea592e5f9cc 100644 --- a/app/presenters/ci/pipeline_presenter.rb +++ b/app/presenters/ci/pipeline_presenter.rb @@ -5,6 +5,9 @@ class PipelinePresenter < Gitlab::View::Presenter::Delegated include Gitlab::Utils::StrongMemoize include ActionView::Helpers::UrlHelper + delegator_override_with Gitlab::Utils::StrongMemoize # TODO: Remove `Gitlab::Utils::StrongMemoize` inclusion as it's duplicate + delegator_override_with ActionView::Helpers::TagHelper # TODO: Remove `ActionView::Helpers::UrlHelper` inclusion as it overrides `Ci::Pipeline#tag` + # We use a class method here instead of a constant, allowing EE to redefine # the returned `Hash` more easily. def self.failure_reasons @@ -20,8 +23,9 @@ def self.failure_reasons user_blocked: 'The user who created this pipeline is blocked.' } end - presents :pipeline + presents ::Ci::Pipeline, as: :pipeline + delegator_override :failed_builds def failed_builds return [] unless can?(current_user, :read_build, pipeline) @@ -30,6 +34,7 @@ def failed_builds end end + delegator_override :failure_reason def failure_reason return unless pipeline.failure_reason? diff --git a/app/presenters/ci/processable_presenter.rb b/app/presenters/ci/processable_presenter.rb index 5a8a6649071139cc330ec289eec440aa948d4039..9f3a83a00f3edcb36b3828dde755a9b7cb565b5e 100644 --- a/app/presenters/ci/processable_presenter.rb +++ b/app/presenters/ci/processable_presenter.rb @@ -2,5 +2,6 @@ module Ci class ProcessablePresenter < CommitStatusPresenter + presents ::Ci::Processable end end diff --git a/app/presenters/ci/runner_presenter.rb b/app/presenters/ci/runner_presenter.rb index 273328afc53ff14c1d9659ef2f1ae39172a3c662..ad889d444f86b944cbcbdb5008aae9135b4dcf2e 100644 --- a/app/presenters/ci/runner_presenter.rb +++ b/app/presenters/ci/runner_presenter.rb @@ -2,11 +2,14 @@ module Ci class RunnerPresenter < Gitlab::View::Presenter::Delegated - presents :runner + presents ::Ci::Runner, as: :runner + delegator_override :locked? def locked? read_attribute(:locked) && project_type? end + + delegator_override :locked alias_method :locked, :locked? end end diff --git a/app/presenters/ci/stage_presenter.rb b/app/presenters/ci/stage_presenter.rb index 21bda86cdedaa1e5874b2290bd5f27d85586b1b9..bd5bf08abbcad386d06c75eb990737cf26ffbbe2 100644 --- a/app/presenters/ci/stage_presenter.rb +++ b/app/presenters/ci/stage_presenter.rb @@ -2,7 +2,7 @@ module Ci class StagePresenter < Gitlab::View::Presenter::Delegated - presents :stage + presents ::Ci::Stage, as: :stage PRELOADED_RELATIONS = [:pipeline, :metadata, :tags, :job_artifacts_archive, :downstream_pipeline].freeze diff --git a/app/presenters/ci/trigger_presenter.rb b/app/presenters/ci/trigger_presenter.rb index 605c8f328a47a4f075314d0ceaca9c899974cbc8..5f0bd4b3a8bbf3c4f1c241d13203c656821f8729 100644 --- a/app/presenters/ci/trigger_presenter.rb +++ b/app/presenters/ci/trigger_presenter.rb @@ -2,12 +2,13 @@ module Ci class TriggerPresenter < Gitlab::View::Presenter::Delegated - presents :trigger + presents ::Ci::Trigger, as: :trigger def has_token_exposed? can?(current_user, :admin_trigger, trigger) end + delegator_override :token def token if has_token_exposed? trigger.token diff --git a/app/presenters/ci/variable_presenter.rb b/app/presenters/ci/variable_presenter.rb index f027f3aa5600fa992cd24aa58efe51c9431fcaf3..ec04dd5e9ff05455fd2bb6ee9527e044c7017a20 100644 --- a/app/presenters/ci/variable_presenter.rb +++ b/app/presenters/ci/variable_presenter.rb @@ -2,7 +2,7 @@ module Ci class VariablePresenter < Gitlab::View::Presenter::Delegated - presents :variable + presents ::Ci::Variable, as: :variable def placeholder 'PROJECT_VARIABLE' diff --git a/app/presenters/clusterable_presenter.rb b/app/presenters/clusterable_presenter.rb index 03e26b929229573c148c9eb1c0fe9be2c1726658..4b645510b515232472eef06bf5b8101bc7263837 100644 --- a/app/presenters/clusterable_presenter.rb +++ b/app/presenters/clusterable_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class ClusterablePresenter < Gitlab::View::Presenter::Delegated - presents :clusterable + presents ::Project, ::Group, ::Clusters::Instance, as: :clusterable def self.fabricate(clusterable, **attributes) presenter_class = "#{clusterable.class.name}ClusterablePresenter".constantize diff --git a/app/presenters/clusters/cluster_presenter.rb b/app/presenters/clusters/cluster_presenter.rb index eb4bd8532afe6fc6cce94fa2b859f6b891df74b6..892ea14267bc39051076e2300511e683aa226fe2 100644 --- a/app/presenters/clusters/cluster_presenter.rb +++ b/app/presenters/clusters/cluster_presenter.rb @@ -7,7 +7,9 @@ class ClusterPresenter < Gitlab::View::Presenter::Delegated include ActionView::Helpers::UrlHelper include IconsHelper - presents :cluster + delegator_override_with ::Gitlab::Utils::StrongMemoize # TODO: Remove `::Gitlab::Utils::StrongMemoize` inclusion as it's duplicate + + presents ::Clusters::Cluster, as: :cluster # We do not want to show the group path for clusters belonging to the # clusterable, only for the ancestor clusters. diff --git a/app/presenters/clusters/integration_presenter.rb b/app/presenters/clusters/integration_presenter.rb index 57608be29b157c654df86bd43c4758f0b0954b6a..f7be59f00f3c70d3e8d04373ca2303bde4121dde 100644 --- a/app/presenters/clusters/integration_presenter.rb +++ b/app/presenters/clusters/integration_presenter.rb @@ -2,7 +2,7 @@ module Clusters class IntegrationPresenter < Gitlab::View::Presenter::Delegated - presents :integration + presents ::Clusters::Integrations::Prometheus, ::Clusters::Integrations::ElasticStack, as: :integration def application_type integration.class.name.demodulize.underscore diff --git a/app/presenters/commit_presenter.rb b/app/presenters/commit_presenter.rb index c14dcab600071b6d91d04b4b07c6e4139eafdce3..7df45ca03bba331ed66a2d0944b2ac735b5db5d7 100644 --- a/app/presenters/commit_presenter.rb +++ b/app/presenters/commit_presenter.rb @@ -3,7 +3,7 @@ class CommitPresenter < Gitlab::View::Presenter::Delegated include GlobalID::Identification - presents :commit + presents ::Commit, as: :commit def status_for(ref) return unless can?(current_user, :read_commit_status, commit.project) diff --git a/app/presenters/commit_status_presenter.rb b/app/presenters/commit_status_presenter.rb index 3c39470b730c120ae3c4bf2542b494c73b686d3e..2d524fd611ecf1feddf97166da704d1a53c8aa97 100644 --- a/app/presenters/commit_status_presenter.rb +++ b/app/presenters/commit_status_presenter.rb @@ -33,7 +33,7 @@ class CommitStatusPresenter < Gitlab::View::Presenter::Delegated private_constant :CALLOUT_FAILURE_MESSAGES - presents :build + presents ::CommitStatus, as: :build def self.callout_failure_messages CALLOUT_FAILURE_MESSAGES diff --git a/app/presenters/dev_ops_report/metric_presenter.rb b/app/presenters/dev_ops_report/metric_presenter.rb index 4d7ac1cd3ec9d33fe1e173541d4a7d9c3b0ae0bd..55326f8f678f390c0e9289debfb6df43c12cda50 100644 --- a/app/presenters/dev_ops_report/metric_presenter.rb +++ b/app/presenters/dev_ops_report/metric_presenter.rb @@ -2,6 +2,8 @@ module DevOpsReport class MetricPresenter < Gitlab::View::Presenter::Simple + presents ::DevOpsReport::Metric + delegate :created_at, to: :subject def cards diff --git a/app/presenters/environment_presenter.rb b/app/presenters/environment_presenter.rb index 6f67bbe2a5a968148ef5095e3a55927e4b6ecb1f..6c8da86187c0a7eb0f3ecbfb4dbdd095a197d427 100644 --- a/app/presenters/environment_presenter.rb +++ b/app/presenters/environment_presenter.rb @@ -3,7 +3,7 @@ class EnvironmentPresenter < Gitlab::View::Presenter::Delegated include ActionView::Helpers::UrlHelper - presents :environment + presents ::Environment, as: :environment def path project_environment_path(project, self) diff --git a/app/presenters/event_presenter.rb b/app/presenters/event_presenter.rb index c37721f7213d762e3c26605ab797b6b63b1e0ae7..4c787b88e20faa8169604cfce7ba1cbae9c60038 100644 --- a/app/presenters/event_presenter.rb +++ b/app/presenters/event_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class EventPresenter < Gitlab::View::Presenter::Delegated - presents :event + presents ::Event, as: :event def initialize(subject, **attributes) super @@ -10,6 +10,7 @@ def initialize(subject, **attributes) end # Caching `visible_to_user?` method in the presenter beause it might be called multiple times. + delegator_override :visible_to_user? def visible_to_user?(user = nil) @visible_to_user_cache.fetch(user&.id) { super(user) } end diff --git a/app/presenters/gitlab/blame_presenter.rb b/app/presenters/gitlab/blame_presenter.rb index 1f2445b04a1fb33e0bf8ed0c4a4c8496515d5700..e9340a42e51f3a5c308b1c1ebd2399ec0f62979c 100644 --- a/app/presenters/gitlab/blame_presenter.rb +++ b/app/presenters/gitlab/blame_presenter.rb @@ -12,7 +12,7 @@ class BlamePresenter < Gitlab::View::Presenter::Simple include TreeHelper include IconsHelper - presents :blame + presents nil, as: :blame CommitData = Struct.new( :author_avatar, diff --git a/app/presenters/group_clusterable_presenter.rb b/app/presenters/group_clusterable_presenter.rb index 34e7084ab02976dbe4bdd98c79b0071d2d32f1da..207767af245ed0fe843128e925eeef3509577c3e 100644 --- a/app/presenters/group_clusterable_presenter.rb +++ b/app/presenters/group_clusterable_presenter.rb @@ -4,6 +4,8 @@ class GroupClusterablePresenter < ClusterablePresenter extend ::Gitlab::Utils::Override include ActionView::Helpers::UrlHelper + presents ::Group + override :cluster_status_cluster_path def cluster_status_cluster_path(cluster, params = {}) cluster_status_group_cluster_path(clusterable, cluster, params) diff --git a/app/presenters/group_member_presenter.rb b/app/presenters/group_member_presenter.rb index 5ab4b51f472b2c8fabda357100ddb98c29780df7..88facc3608de4bebfb30e86e5fad8109a85ee2cd 100644 --- a/app/presenters/group_member_presenter.rb +++ b/app/presenters/group_member_presenter.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class GroupMemberPresenter < MemberPresenter + presents ::GroupMember + private def admin_member_permission diff --git a/app/presenters/instance_clusterable_presenter.rb b/app/presenters/instance_clusterable_presenter.rb index 56d91f90b2ea4e611723de91919a2111692bcdb0..77ca171156d61445be7706777c8a10c1dc271ae4 100644 --- a/app/presenters/instance_clusterable_presenter.rb +++ b/app/presenters/instance_clusterable_presenter.rb @@ -4,6 +4,8 @@ class InstanceClusterablePresenter < ClusterablePresenter extend ::Gitlab::Utils::Override include ActionView::Helpers::UrlHelper + presents ::Clusters::Instance + def self.fabricate(clusterable, **attributes) attributes_with_presenter_class = attributes.merge(presenter_class: InstanceClusterablePresenter) diff --git a/app/presenters/invitation_presenter.rb b/app/presenters/invitation_presenter.rb index d8c07f327dd6beab9002e09e9a86afb94b542bf7..ada8227a477152ba3ac327cca3e5c2f15f781839 100644 --- a/app/presenters/invitation_presenter.rb +++ b/app/presenters/invitation_presenter.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true class InvitationPresenter < Gitlab::View::Presenter::Delegated - presents :invitation + presents nil, as: :invitation end diff --git a/app/presenters/issue_presenter.rb b/app/presenters/issue_presenter.rb index b7f4ac0555d4e9fcc6de1788f1f1a25a357bf376..72fe14d224de4579d3606c44d2413e1c329ef7f8 100644 --- a/app/presenters/issue_presenter.rb +++ b/app/presenters/issue_presenter.rb @@ -1,12 +1,13 @@ # frozen_string_literal: true class IssuePresenter < Gitlab::View::Presenter::Delegated - presents :issue + presents ::Issue, as: :issue def issue_path url_builder.build(issue, only_path: true) end + delegator_override :subscribed? def subscribed? issue.subscribed?(current_user, issue.project) end diff --git a/app/presenters/label_presenter.rb b/app/presenters/label_presenter.rb index 9e51e6fa4bae37a0acff26fa74e38f5a60f4e53c..fafade2828f0dbedaab3375ba96ff294e5704911 100644 --- a/app/presenters/label_presenter.rb +++ b/app/presenters/label_presenter.rb @@ -1,9 +1,11 @@ # frozen_string_literal: true class LabelPresenter < Gitlab::View::Presenter::Delegated - presents :label + presents ::Label, as: :label delegate :name, :full_name, to: :label_subject, prefix: :subject + delegator_override :subject # TODO: Fix `Gitlab::View::Presenter::Delegated#subject` not to override `Label#subject`. + def edit_path case label when GroupLabel then edit_group_label_path(label.group, label) diff --git a/app/presenters/member_presenter.rb b/app/presenters/member_presenter.rb index b37a43bf251ca8800101327fb1ee296e103f5ccd..67d044dd01c377fdbb5a0bc4646ee202c334a215 100644 --- a/app/presenters/member_presenter.rb +++ b/app/presenters/member_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class MemberPresenter < Gitlab::View::Presenter::Delegated - presents :member + presents ::Member, as: :member def access_level_roles member.class.access_level_roles diff --git a/app/presenters/members_presenter.rb b/app/presenters/members_presenter.rb index 03ebea36d49b4deb0c7fec606bc68a80a8945067..b572cf962358a107de523747cac26c331105138f 100644 --- a/app/presenters/members_presenter.rb +++ b/app/presenters/members_presenter.rb @@ -3,7 +3,7 @@ class MembersPresenter < Gitlab::View::Presenter::Delegated include Enumerable - presents :members + presents nil, as: :members def to_ary to_a diff --git a/app/presenters/merge_request_presenter.rb b/app/presenters/merge_request_presenter.rb index fc8a290f5f78dccae0c7bfb727b56636d5081fe7..d19d4964524e2836639510c934c7928ead9c43af 100644 --- a/app/presenters/merge_request_presenter.rb +++ b/app/presenters/merge_request_presenter.rb @@ -8,9 +8,11 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated include ChecksCollaboration include Gitlab::Utils::StrongMemoize + delegator_override_with Gitlab::Utils::StrongMemoize # TODO: Remove `Gitlab::Utils::StrongMemoize` inclusion as it's duplicate + APPROVALS_WIDGET_BASE_TYPE = 'base' - presents :merge_request + presents ::MergeRequest, as: :merge_request def ci_status if pipeline @@ -183,6 +185,7 @@ def can_push_to_source_branch? .can_push_to_branch?(source_branch) end + delegator_override :can_remove_source_branch? def can_remove_source_branch? source_branch_exists? && merge_request.can_remove_source_branch?(current_user) end @@ -202,6 +205,7 @@ def mergeable_discussions_state end end + delegator_override :subscribed? def subscribed? merge_request.subscribed?(current_user, merge_request.target_project) end diff --git a/app/presenters/milestone_presenter.rb b/app/presenters/milestone_presenter.rb index 6bf8203702fe10cc5f876f28edeb53659b3f3b74..4084c8740f0e9f739dbe375320dd2a289db17200 100644 --- a/app/presenters/milestone_presenter.rb +++ b/app/presenters/milestone_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class MilestonePresenter < Gitlab::View::Presenter::Delegated - presents :milestone + presents ::Milestone, as: :milestone def milestone_path url_builder.build(milestone, only_path: true) diff --git a/app/presenters/pages_domain_presenter.rb b/app/presenters/pages_domain_presenter.rb index 6ef89760bec1fb3d825e3d91bafad8427506021e..0523f702416131c94ce2f0b842cd33991d06f03c 100644 --- a/app/presenters/pages_domain_presenter.rb +++ b/app/presenters/pages_domain_presenter.rb @@ -1,7 +1,9 @@ # frozen_string_literal: true class PagesDomainPresenter < Gitlab::View::Presenter::Delegated - presents :pages_domain + presents ::PagesDomain, as: :pages_domain + + delegator_override :subject # TODO: Fix `Gitlab::View::Presenter::Delegated#subject` not to override `PagesDomain#subject`. def needs_verification? Gitlab::CurrentSettings.pages_domain_verification_enabled? && unverified? diff --git a/app/presenters/project_clusterable_presenter.rb b/app/presenters/project_clusterable_presenter.rb index 920304e743ed7ff001ce0578ba22ae8a7c093168..05344f2da965c4197b70806dd38b59ece700a3d9 100644 --- a/app/presenters/project_clusterable_presenter.rb +++ b/app/presenters/project_clusterable_presenter.rb @@ -4,6 +4,8 @@ class ProjectClusterablePresenter < ClusterablePresenter extend ::Gitlab::Utils::Override include ActionView::Helpers::UrlHelper + presents ::Project + override :cluster_status_cluster_path def cluster_status_cluster_path(cluster, params = {}) cluster_status_project_cluster_path(clusterable, cluster, params) diff --git a/app/presenters/project_hook_presenter.rb b/app/presenters/project_hook_presenter.rb index a65c7221b5a432cb9010de4fd0f4f325a8623b4f..a696e9fd0ec8db980d6dd6c4805348d2715c84de 100644 --- a/app/presenters/project_hook_presenter.rb +++ b/app/presenters/project_hook_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class ProjectHookPresenter < Gitlab::View::Presenter::Delegated - presents :project_hook + presents ::ProjectHook, as: :project_hook def logs_details_path(log) project_hook_hook_log_path(project, self, log) diff --git a/app/presenters/project_member_presenter.rb b/app/presenters/project_member_presenter.rb index 17947266ed79b29ad51313b9773d8ed2e5995834..91d3ae96877c22657cf96d5f345ea3fb3fa4dd75 100644 --- a/app/presenters/project_member_presenter.rb +++ b/app/presenters/project_member_presenter.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class ProjectMemberPresenter < MemberPresenter + presents ::ProjectMember + private def admin_member_permission diff --git a/app/presenters/project_presenter.rb b/app/presenters/project_presenter.rb index 066f4786cff5169a555ca39abaa17be8af035fd8..bbd8c715f5cb9292a344583c974ae72ee7cc4934 100644 --- a/app/presenters/project_presenter.rb +++ b/app/presenters/project_presenter.rb @@ -12,7 +12,10 @@ class ProjectPresenter < Gitlab::View::Presenter::Delegated include Gitlab::Utils::StrongMemoize include Gitlab::Experiment::Dsl - presents :project + delegator_override_with GitlabRoutingHelper # TODO: Remove `GitlabRoutingHelper` inclusion as it's duplicate + delegator_override_with Gitlab::Utils::StrongMemoize # TODO: Remove `Gitlab::Utils::StrongMemoize` inclusion as it's duplicate + + presents ::Project, as: :project AnchorData = Struct.new(:is_link, :label, :link, :class_modifier, :icon, :itemprop, :data) MAX_TOPICS_TO_SHOW = 3 diff --git a/app/presenters/projects/import_export/project_export_presenter.rb b/app/presenters/projects/import_export/project_export_presenter.rb index f56760b55df4c50d5c6fa9c7d1cedcdf80478db0..7b2ffb6d7557212807a987ca65da1926cb490d1e 100644 --- a/app/presenters/projects/import_export/project_export_presenter.rb +++ b/app/presenters/projects/import_export/project_export_presenter.rb @@ -5,16 +5,24 @@ module ImportExport class ProjectExportPresenter < Gitlab::View::Presenter::Delegated include ActiveModel::Serializers::JSON - presents :project + presents ::Project, as: :project + # TODO: Remove `ActiveModel::Serializers::JSON` inclusion as it's duplicate + delegator_override_with ActiveModel::Serializers::JSON + delegator_override_with ActiveModel::Naming + delegator_override :include_root_in_json, :include_root_in_json? + + delegator_override :project_members def project_members super + converted_group_members end + delegator_override :description def description self.respond_to?(:override_description) ? override_description : super end + delegator_override :protected_branches def protected_branches project.exported_protected_branches end diff --git a/app/presenters/projects/settings/deploy_keys_presenter.rb b/app/presenters/projects/settings/deploy_keys_presenter.rb index 13290a8e632ffe6ca65cd324b95d4494a38116f8..e3323b7518811b5f5256f250017964d41f04208f 100644 --- a/app/presenters/projects/settings/deploy_keys_presenter.rb +++ b/app/presenters/projects/settings/deploy_keys_presenter.rb @@ -5,7 +5,7 @@ module Settings class DeployKeysPresenter < Gitlab::View::Presenter::Simple include Gitlab::Utils::StrongMemoize - presents :project + presents ::Project, as: :project delegate :size, to: :enabled_keys, prefix: true delegate :size, to: :available_project_keys, prefix: true delegate :size, to: :available_public_keys, prefix: true diff --git a/app/presenters/prometheus_alert_presenter.rb b/app/presenters/prometheus_alert_presenter.rb index 99e24bdcdb9c283973564fcf53ec308535c2290a..714329ede71eaf01c32e16506c773af03a17cc21 100644 --- a/app/presenters/prometheus_alert_presenter.rb +++ b/app/presenters/prometheus_alert_presenter.rb @@ -3,7 +3,7 @@ class PrometheusAlertPresenter < Gitlab::View::Presenter::Delegated include ActionView::Helpers::UrlHelper - presents :prometheus_alert + presents ::PrometheusAlert, as: :prometheus_alert def humanized_text operator_text = diff --git a/app/presenters/release_presenter.rb b/app/presenters/release_presenter.rb index ac27e997b41c1cb777b4877bca662e8a9bd7bdfa..c919c7f4c60df735e10f097bfa91be2228e0e802 100644 --- a/app/presenters/release_presenter.rb +++ b/app/presenters/release_presenter.rb @@ -3,8 +3,10 @@ class ReleasePresenter < Gitlab::View::Presenter::Delegated include ActionView::Helpers::UrlHelper - presents :release + presents ::Release, as: :release + # TODO: Remove `delegate` as it's redundant due to SimpleDelegator. + delegator_override :tag, :project delegate :project, :tag, to: :release def commit_path @@ -51,6 +53,7 @@ def edit_url edit_project_release_url(project, release) end + delegator_override :assets_count def assets_count if can_download_code? release.assets_count @@ -59,6 +62,7 @@ def assets_count end end + delegator_override :name def name can_download_code? ? release.name : "Release-#{release.id}" end diff --git a/app/presenters/releases/evidence_presenter.rb b/app/presenters/releases/evidence_presenter.rb index a00cbacb7d836106d3d2919c90dca7e25d056da8..bdc053a303bee92504e05051db06edbcd5792206 100644 --- a/app/presenters/releases/evidence_presenter.rb +++ b/app/presenters/releases/evidence_presenter.rb @@ -4,7 +4,7 @@ module Releases class EvidencePresenter < Gitlab::View::Presenter::Delegated include ActionView::Helpers::UrlHelper - presents :evidence + presents ::Releases::Evidence, as: :evidence def filepath release = evidence.release diff --git a/app/presenters/search_service_presenter.rb b/app/presenters/search_service_presenter.rb index ab43800b9f236ab67bdc9b1185788a0b203a6e1c..72f967b8beb12883d4a7195f7077fee122795439 100644 --- a/app/presenters/search_service_presenter.rb +++ b/app/presenters/search_service_presenter.rb @@ -3,7 +3,7 @@ class SearchServicePresenter < Gitlab::View::Presenter::Delegated include RendersCommits - presents :search_service + presents ::SearchService, as: :search_service SCOPE_PRELOAD_METHOD = { projects: :with_web_entity_associations, @@ -18,6 +18,7 @@ class SearchServicePresenter < Gitlab::View::Presenter::Delegated SORT_ENABLED_SCOPES = %w(issues merge_requests epics).freeze + delegator_override :search_objects def search_objects @search_objects ||= begin objects = search_service.search_objects(SCOPE_PRELOAD_METHOD[scope.to_sym]) diff --git a/app/presenters/sentry_error_presenter.rb b/app/presenters/sentry_error_presenter.rb index 669bcb68b7cf6527b8f176aad28c7daeba7b4d93..5862e54dfc7cea1b2ce2e639af34592f1cf751a2 100644 --- a/app/presenters/sentry_error_presenter.rb +++ b/app/presenters/sentry_error_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class SentryErrorPresenter < Gitlab::View::Presenter::Delegated - presents :error + presents nil, as: :error FrequencyStruct = Struct.new(:time, :count, keyword_init: true) diff --git a/app/presenters/service_hook_presenter.rb b/app/presenters/service_hook_presenter.rb index 8f2ba1a905f1cefb5c20bb21705576b0a1af1390..91911eb3dff27882fe48c3f966ee6e3c6921d564 100644 --- a/app/presenters/service_hook_presenter.rb +++ b/app/presenters/service_hook_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class ServiceHookPresenter < Gitlab::View::Presenter::Delegated - presents :service_hook + presents ::ServiceHook, as: :service_hook def logs_details_path(log) project_service_hook_log_path(integration.project, integration, log) diff --git a/app/presenters/snippet_blob_presenter.rb b/app/presenters/snippet_blob_presenter.rb index ab8fc0f905b7fcae2a712abbd2d722fc98390622..4072696eb89027f0ec57d1ed1fe0c5db73094ff7 100644 --- a/app/presenters/snippet_blob_presenter.rb +++ b/app/presenters/snippet_blob_presenter.rb @@ -3,6 +3,8 @@ class SnippetBlobPresenter < BlobPresenter include GitlabRoutingHelper + presents ::SnippetBlob + def rich_data return unless blob.rich_viewer diff --git a/app/presenters/snippet_presenter.rb b/app/presenters/snippet_presenter.rb index 695aa266e2ca8a4b74ef132bd9292eb9c1ceb703..8badbe7f54acd47200790c47feb43c307278fb50 100644 --- a/app/presenters/snippet_presenter.rb +++ b/app/presenters/snippet_presenter.rb @@ -1,16 +1,18 @@ # frozen_string_literal: true class SnippetPresenter < Gitlab::View::Presenter::Delegated - presents :snippet + presents ::Snippet, as: :snippet def raw_url url_builder.build(snippet, raw: true) end + delegator_override :ssh_url_to_repo def ssh_url_to_repo snippet.ssh_url_to_repo if snippet.repository_exists? end + delegator_override :http_url_to_repo def http_url_to_repo snippet.http_url_to_repo if snippet.repository_exists? end @@ -31,6 +33,7 @@ def can_report_as_spam? snippet.submittable_as_spam_by?(current_user) end + delegator_override :blob def blob return snippet.blob if snippet.empty_repo? diff --git a/app/presenters/terraform/modules_presenter.rb b/app/presenters/terraform/modules_presenter.rb index 608f69e201975667b2caba2a606ed21d1b8c979e..9e9c6a5cd2b6d0ed2b4433f2eee35f7d4df79bb0 100644 --- a/app/presenters/terraform/modules_presenter.rb +++ b/app/presenters/terraform/modules_presenter.rb @@ -4,7 +4,7 @@ module Terraform class ModulesPresenter < Gitlab::View::Presenter::Simple attr_accessor :packages, :system - presents :modules + presents nil, as: :modules def initialize(packages, system) @packages = packages diff --git a/app/presenters/todo_presenter.rb b/app/presenters/todo_presenter.rb index 291be7848e25ac02e1f10c89fe3af761d92b4c91..cb8d37be22d8f2c491718809fb60941749ab0d37 100644 --- a/app/presenters/todo_presenter.rb +++ b/app/presenters/todo_presenter.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true class TodoPresenter < Gitlab::View::Presenter::Delegated - presents :todo + presents ::Todo, as: :todo end diff --git a/app/presenters/tree_entry_presenter.rb b/app/presenters/tree_entry_presenter.rb index 216b3b0d4c91bded9a4ed95a03e8d3ddd18c53c9..0b313d81360190194a53ee4822576d1947d1e2a0 100644 --- a/app/presenters/tree_entry_presenter.rb +++ b/app/presenters/tree_entry_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class TreeEntryPresenter < Gitlab::View::Presenter::Delegated - presents :tree + presents nil, as: :tree def web_url Gitlab::Routing.url_helpers.project_tree_url(tree.repository.project, File.join(tree.commit_id, tree.path)) diff --git a/app/presenters/user_presenter.rb b/app/presenters/user_presenter.rb index 7cd94082bacbb482dfe54fafd4a1edc497ad3bd1..5a99f10b6e74971f8095c30041ffd68993d05459 100644 --- a/app/presenters/user_presenter.rb +++ b/app/presenters/user_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class UserPresenter < Gitlab::View::Presenter::Delegated - presents :user + presents ::User, as: :user def group_memberships should_be_private? ? GroupMember.none : user.group_members diff --git a/app/presenters/web_hook_log_presenter.rb b/app/presenters/web_hook_log_presenter.rb index fca03ddb5d76aff5fa1325764b50a1db25764527..a5166589073795eb76d4ce58076da8b1fa1c8b31 100644 --- a/app/presenters/web_hook_log_presenter.rb +++ b/app/presenters/web_hook_log_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class WebHookLogPresenter < Gitlab::View::Presenter::Delegated - presents :web_hook_log + presents ::WebHookLog, as: :web_hook_log def details_path web_hook.present.logs_details_path(self) diff --git a/doc/development/reusing_abstractions.md b/doc/development/reusing_abstractions.md index ded6b0743248b66a461e7ac5ff229e74e89cb43f..568e8a9d1235ac41fc8406192da35dc87c769a56 100644 --- a/doc/development/reusing_abstractions.md +++ b/doc/development/reusing_abstractions.md @@ -183,6 +183,8 @@ queries they produce. Everything in `app/presenters`, used for exposing complex data to a Rails view, without having to create many instance variables. +See [the documentation](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/presenters/README.md) for more information. + ### Serializers Everything in `app/serializers`, used for presenting the response to a request, diff --git a/ee/app/presenters/audit_event_presenter.rb b/ee/app/presenters/audit_event_presenter.rb index 7c42cbb3a38cab4b13f6d4be44ce228eeaaca153..48170b89c1e7d5aae56a5c6386c279e3af4029cf 100644 --- a/ee/app/presenters/audit_event_presenter.rb +++ b/ee/app/presenters/audit_event_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AuditEventPresenter < Gitlab::View::Presenter::Simple - presents :audit_event + presents ::AuditEvent, as: :audit_event def author_name audit_event.author_name diff --git a/ee/app/presenters/boards/epic_board_presenter.rb b/ee/app/presenters/boards/epic_board_presenter.rb index 4b791a1e893061c863f0525be7b731ee33036ed3..a788a670993484b0be1974fdcfb8334deccc4399 100644 --- a/ee/app/presenters/boards/epic_board_presenter.rb +++ b/ee/app/presenters/boards/epic_board_presenter.rb @@ -2,6 +2,6 @@ module Boards class EpicBoardPresenter < Gitlab::View::Presenter::Delegated - presents :epic_board + presents ::Boards::EpicBoard, as: :epic_board end end diff --git a/ee/app/presenters/dast/site_profile_presenter.rb b/ee/app/presenters/dast/site_profile_presenter.rb index 7fe17745aaeb95332ea878d900bbfb1938468329..ca1f40873a9c43611f5754f884e60bbf2f721cfa 100644 --- a/ee/app/presenters/dast/site_profile_presenter.rb +++ b/ee/app/presenters/dast/site_profile_presenter.rb @@ -5,7 +5,7 @@ class SiteProfilePresenter < Gitlab::View::Presenter::Delegated REDACTED_PASSWORD = '••••••••' REDACTED_REQUEST_HEADERS = '••••••••' - presents :site_profile + presents ::DastSiteProfile, as: :site_profile def password return unless site_profile.secret_variables.any? { |variable| variable.key == ::Dast::SiteProfileSecretVariable::PASSWORD } diff --git a/ee/app/presenters/ee/ci/build_presenter.rb b/ee/app/presenters/ee/ci/build_presenter.rb index 679224e7a397cfe6a66c09d045badb05921c1087..1c739f66e599a9bed28c050bfb7ebbc884625fab 100644 --- a/ee/app/presenters/ee/ci/build_presenter.rb +++ b/ee/app/presenters/ee/ci/build_presenter.rb @@ -4,7 +4,9 @@ module EE module Ci module BuildPresenter extend ActiveSupport::Concern + extend ::Gitlab::Utils::DelegatorOverride + delegator_override :retryable? def retryable? !merge_train_pipeline? && super end diff --git a/ee/app/presenters/ee/ci/pipeline_presenter.rb b/ee/app/presenters/ee/ci/pipeline_presenter.rb index d31cb811239c3734bcbdbafc658e25625ab5ad2d..e1b23cdc446ed9987aae06ca8dbabf8548611d7b 100644 --- a/ee/app/presenters/ee/ci/pipeline_presenter.rb +++ b/ee/app/presenters/ee/ci/pipeline_presenter.rb @@ -4,6 +4,7 @@ module EE module Ci module PipelinePresenter extend ActiveSupport::Concern + extend ::Gitlab::Utils::DelegatorOverride def expose_security_dashboard? return false unless can?(current_user, :read_security_resource, pipeline.project) @@ -18,6 +19,7 @@ def degradation_threshold(file_type) end end + delegator_override :retryable? def retryable? !merge_train_pipeline? && super end diff --git a/ee/app/presenters/ee/label_presenter.rb b/ee/app/presenters/ee/label_presenter.rb index fd3a4572077fe667d1743d5d2101d9d49b58af4d..30180ca8e69e9885d64a692d720adf58cc2004c5 100644 --- a/ee/app/presenters/ee/label_presenter.rb +++ b/ee/app/presenters/ee/label_presenter.rb @@ -2,6 +2,9 @@ module EE module LabelPresenter + extend ::Gitlab::Utils::DelegatorOverride + + delegator_override :scoped_label? def scoped_label? label.scoped_label? && context_subject && context_subject.feature_available?(:scoped_labels) end diff --git a/ee/app/presenters/ee/merge_request_presenter.rb b/ee/app/presenters/ee/merge_request_presenter.rb index 3a9dad8fb5d2fc419cc6f6ed0d06c621a8e8bce0..2a15d8241943fa338c44ebb6a6cba9d0c4044aaf 100644 --- a/ee/app/presenters/ee/merge_request_presenter.rb +++ b/ee/app/presenters/ee/merge_request_presenter.rb @@ -4,6 +4,9 @@ module EE module MergeRequestPresenter include ::VisibleApprovable extend ::Gitlab::Utils::Override + extend ::Gitlab::Utils::DelegatorOverride + + delegator_override_with ::VisibleApprovable # TODO: Remove `::VisibleApprovable` inclusion as it's duplicate APPROVALS_WIDGET_FULL_TYPE = 'full' @@ -33,6 +36,7 @@ def merge_immediately_docs_path help_page_path('ci/pipelines/merge_trains.md', anchor: 'immediately-merge-a-merge-request-with-a-merge-train') end + delegator_override :target_project def target_project merge_request.target_project.present(current_user: current_user) end @@ -41,6 +45,7 @@ def code_owner_rules_with_users @code_owner_rules ||= merge_request.approval_rules.code_owner.with_users.to_a end + delegator_override :approver_groups def approver_groups ::ApproverGroup.filtered_approver_groups(merge_request.approver_groups, current_user) end @@ -54,6 +59,7 @@ def approvals_widget_type expose_mr_approval_path? ? APPROVALS_WIDGET_FULL_TYPE : super end + delegator_override :missing_security_scan_types def missing_security_scan_types merge_request.missing_security_scan_types if expose_missing_security_scan_types? end diff --git a/ee/app/presenters/ee/project_presenter.rb b/ee/app/presenters/ee/project_presenter.rb index d82df27e156f866846fcbbe18f8b419845b37637..2cea06454d7573b7ba6fdd56cbb4935fb03bf317 100644 --- a/ee/app/presenters/ee/project_presenter.rb +++ b/ee/app/presenters/ee/project_presenter.rb @@ -3,6 +3,7 @@ module EE module ProjectPresenter extend ::Gitlab::Utils::Override + extend ::Gitlab::Utils::DelegatorOverride override :statistics_buttons def statistics_buttons(show_auto_devops_callout:) @@ -13,6 +14,7 @@ def extra_statistics_buttons [sast_anchor_data.presence].compact end + delegator_override :approver_groups def approver_groups ::ApproverGroup.filtered_approver_groups(project.approver_groups, current_user) end diff --git a/ee/app/presenters/epic_issue_presenter.rb b/ee/app/presenters/epic_issue_presenter.rb index f54904196a5689cc10623f8b651989daf096c95b..a0338354556a6601d32481d8f8f78999e71a21de 100644 --- a/ee/app/presenters/epic_issue_presenter.rb +++ b/ee/app/presenters/epic_issue_presenter.rb @@ -1,7 +1,8 @@ # frozen_string_literal: true class EpicIssuePresenter < Gitlab::View::Presenter::Delegated - presents :issue + delegator_override :issue + presents ::EpicIssue, as: :issue def group_epic_issue_path(current_user) return unless can_admin_issue_link?(current_user) diff --git a/ee/app/presenters/epic_presenter.rb b/ee/app/presenters/epic_presenter.rb index 16e53e49134bf4cd883b14526f6877db88b8c785..ae6bed4ebbb74bd2812e24f8a310c27aaf1605ed 100644 --- a/ee/app/presenters/epic_presenter.rb +++ b/ee/app/presenters/epic_presenter.rb @@ -4,7 +4,7 @@ class EpicPresenter < Gitlab::View::Presenter::Delegated include GitlabRoutingHelper include EntityDateHelper - presents :epic + presents ::Epic, as: :epic def show_data(base_data: {}, author_icon: nil) { @@ -35,6 +35,7 @@ def epic_reference(full: false) end end + delegator_override :subscribed? def subscribed? epic.subscribed?(current_user) end diff --git a/ee/app/presenters/iteration_presenter.rb b/ee/app/presenters/iteration_presenter.rb index 4c2763b3345fb5403efb4aae7b1ddcadc6ded40e..63871c7cc432bf1d82cf2bd5301284ea5982b877 100644 --- a/ee/app/presenters/iteration_presenter.rb +++ b/ee/app/presenters/iteration_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class IterationPresenter < Gitlab::View::Presenter::Delegated - presents :iteration + presents ::Iteration, as: :iteration def iteration_path url_builder.build(iteration, only_path: true) diff --git a/ee/app/presenters/merge_request_approver_presenter.rb b/ee/app/presenters/merge_request_approver_presenter.rb index 6c0e5423d1bed84def0200b6c0477a9e551bb313..1efa1a71864a078e76f806a1df8d81084fcf5d99 100644 --- a/ee/app/presenters/merge_request_approver_presenter.rb +++ b/ee/app/presenters/merge_request_approver_presenter.rb @@ -12,7 +12,7 @@ class MergeRequestApproverPresenter < Gitlab::View::Presenter::Simple include ActionView::RecordIdentifier include Gitlab::Utils::StrongMemoize - presents :merge_request + presents ::MergeRequest, as: :merge_request attr_reader :skip_user diff --git a/ee/app/presenters/projects/security/configuration_presenter.rb b/ee/app/presenters/projects/security/configuration_presenter.rb index d32e3a706f6d28eb77a7bbde1e367662d64bf294..2ecb20bcf05b54f2d7fcb6bff17ac24b7f7cc4b8 100644 --- a/ee/app/presenters/projects/security/configuration_presenter.rb +++ b/ee/app/presenters/projects/security/configuration_presenter.rb @@ -7,7 +7,9 @@ class ConfigurationPresenter < Gitlab::View::Presenter::Delegated include AutoDevopsHelper include LatestPipelineInformation - presents :project + delegator_override_with Gitlab::Utils::StrongMemoize # TODO: Remove `Gitlab::Utils::StrongMemoize` inclusion as it's duplicate + + presents ::Project, as: :project def to_h { diff --git a/ee/app/presenters/security/scan_presenter.rb b/ee/app/presenters/security/scan_presenter.rb index 9f59738d144685788ac2b9cd847ae65f9cccf443..36871023b9803357e2e1a806fde5adaa33a6adea 100644 --- a/ee/app/presenters/security/scan_presenter.rb +++ b/ee/app/presenters/security/scan_presenter.rb @@ -4,8 +4,9 @@ module Security class ScanPresenter < Gitlab::View::Presenter::Delegated ERROR_MESSAGE_FORMAT = '[%<type>s] %<message>s' - presents :scan + presents ::Security::Scan, as: :scan + delegator_override :errors def errors info['errors'].to_a.map { |error| format(ERROR_MESSAGE_FORMAT, error.symbolize_keys) } end diff --git a/ee/app/presenters/subscription_presenter.rb b/ee/app/presenters/subscription_presenter.rb index b726876e4aa7e5a1fca6ceac3b700cf31c0bb35f..aa42356c72db2400311ca3a71b811196cb0062dd 100644 --- a/ee/app/presenters/subscription_presenter.rb +++ b/ee/app/presenters/subscription_presenter.rb @@ -3,7 +3,7 @@ class SubscriptionPresenter < Gitlab::View::Presenter::Delegated GRACE_PERIOD_EXTENSION_DAYS = 14.days - presents :subscription + presents ::Subscription, as: :subscription def block_changes? will_block_changes? && (block_changes_at < Date.today) diff --git a/ee/app/presenters/subscriptions/new_plan_presenter.rb b/ee/app/presenters/subscriptions/new_plan_presenter.rb index 90502c66c63eddfc1c6b0ee496c2a36632ffc7ad..858c22dec8489a768c1798aec1c949aa7b8cb1ab 100644 --- a/ee/app/presenters/subscriptions/new_plan_presenter.rb +++ b/ee/app/presenters/subscriptions/new_plan_presenter.rb @@ -2,11 +2,14 @@ module Subscriptions class NewPlanPresenter < Gitlab::View::Presenter::Delegated + presents ::Plan + NEW_PLAN_TITLES = { silver: 'Premium (Formerly Silver)', gold: 'Ultimate (Formerly Gold)' }.freeze + delegator_override :title def title NEW_PLAN_TITLES.fetch(plan_key, super) end diff --git a/ee/app/presenters/vulnerabilities/finding_presenter.rb b/ee/app/presenters/vulnerabilities/finding_presenter.rb index 9235f696b167b78e8a6b28553bc03b968a95f5b5..c3e094413aa4ada35ff3983aaf9ef4a64c7a136d 100644 --- a/ee/app/presenters/vulnerabilities/finding_presenter.rb +++ b/ee/app/presenters/vulnerabilities/finding_presenter.rb @@ -2,7 +2,7 @@ module Vulnerabilities class FindingPresenter < Gitlab::View::Presenter::Delegated - presents :finding + presents ::Vulnerabilities::Finding, as: :finding def title name diff --git a/ee/app/presenters/vulnerability_presenter.rb b/ee/app/presenters/vulnerability_presenter.rb index 295462cc4faa9aa0a6c9f83b4a8e4251f815011d..eadf6a7228f34e04766ff62bf94b6230bc88a63a 100644 --- a/ee/app/presenters/vulnerability_presenter.rb +++ b/ee/app/presenters/vulnerability_presenter.rb @@ -1,12 +1,14 @@ # frozen_string_literal: true class VulnerabilityPresenter < Gitlab::View::Presenter::Delegated - presents :vulnerability + presents ::Vulnerability, as: :vulnerability + delegator_override :links def links vulnerability.links.map(&:with_indifferent_access) end + delegator_override :remediations def remediations vulnerability.remediations.to_a.compact.map(&:with_indifferent_access) end @@ -31,6 +33,7 @@ def raw_path path_with_line_number(project_raw_path(vulnerability.project, File.join(pipeline_branch, file))) end + delegator_override :blob_path def blob_path return unless file @@ -52,6 +55,7 @@ def jira_issue_description ) end + delegator_override :description def description vulnerability.description || finding.description end diff --git a/lib/gitlab/utils/delegator_override.rb b/lib/gitlab/utils/delegator_override.rb new file mode 100644 index 0000000000000000000000000000000000000000..15ba29d39160529186f14d9c16023dcb10a4fad1 --- /dev/null +++ b/lib/gitlab/utils/delegator_override.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +module Gitlab + module Utils + # This module is to validate that delegator classes (`SimpleDelegator`) do not + # accidentally override important logic on the fabricated object. + module DelegatorOverride + def delegator_target(target_class) + return unless ENV['STATIC_VERIFICATION'] + + unless self < ::SimpleDelegator + raise ArgumentError, "'#{self}' is not a subclass of 'SimpleDelegator' class." + end + + DelegatorOverride.validator(self).add_target(target_class) + end + + def delegator_override(*names) + return unless ENV['STATIC_VERIFICATION'] + raise TypeError unless names.all? { |n| n.is_a?(Symbol) } + + DelegatorOverride.validator(self).add_allowlist(names) + end + + def delegator_override_with(mod) + return unless ENV['STATIC_VERIFICATION'] + raise TypeError unless mod.is_a?(Module) + + DelegatorOverride.validator(self).add_allowlist(mod.instance_methods) + end + + def self.validator(delegator_class) + validators[delegator_class] ||= Validator.new(delegator_class) + end + + def self.validators + @validators ||= {} + end + + def self.verify! + validators.each_value do |validator| + validator.expand_on_ancestors(validators) + validator.validate_overrides! + end + end + end + end +end diff --git a/lib/gitlab/utils/delegator_override/error.rb b/lib/gitlab/utils/delegator_override/error.rb new file mode 100644 index 0000000000000000000000000000000000000000..dfe8d5468b476ef1ec1ba8ad2f7c8515232a385e --- /dev/null +++ b/lib/gitlab/utils/delegator_override/error.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Gitlab + module Utils + module DelegatorOverride + class Error + attr_accessor :method_name, :target_class, :target_location, :delegator_class, :delegator_location + + def initialize(method_name, target_class, target_location, delegator_class, delegator_location) + @method_name = method_name + @target_class = target_class + @target_location = target_location + @delegator_class = delegator_class + @delegator_location = delegator_location + end + + def to_s + "#{delegator_class}##{method_name} is overriding #{target_class}##{method_name}. delegator_location: #{delegator_location} target_location: #{target_location}" + end + end + end + end +end diff --git a/lib/gitlab/utils/delegator_override/validator.rb b/lib/gitlab/utils/delegator_override/validator.rb new file mode 100644 index 0000000000000000000000000000000000000000..825b3efa20349cec9b65b91512629f0582577994 --- /dev/null +++ b/lib/gitlab/utils/delegator_override/validator.rb @@ -0,0 +1,99 @@ +# frozen_string_literal: true + +module Gitlab + module Utils + module DelegatorOverride + class Validator + UnexpectedDelegatorOverrideError = Class.new(StandardError) + + attr_reader :delegator_class, :target_classes + + OVERRIDE_ERROR_MESSAGE = <<~EOS + We've detected that the delegator is overriding a specific method(s) on the target class. + Please make sure if it's intentional and handle this error accordingly. + See https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/presenters/README.md#validate-accidental-overrides for more information. + EOS + + def initialize(delegator_class) + @delegator_class = delegator_class + @target_classes = [] + end + + def add_allowlist(names) + allowed_method_names.concat(names) + end + + def allowed_method_names + @allowed_method_names ||= [] + end + + def add_target(target_class) + @target_classes << target_class if target_class + end + + # This will make sure allowlist we put into ancestors are all included + def expand_on_ancestors(validators) + delegator_class.ancestors.each do |ancestor| + next if delegator_class == ancestor # ancestor includes itself + + validator_ancestor = validators[ancestor] + + next unless validator_ancestor + + add_allowlist(validator_ancestor.allowed_method_names) + end + end + + def validate_overrides! + return if target_classes.empty? + + errors = [] + + # Workaround to fully load the instance methods in the target class. + # See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/69823#note_678887402 + target_classes.map(&:new) rescue nil + + (delegator_class.instance_methods - allowlist).each do |method_name| + target_classes.each do |target_class| + next unless target_class.instance_methods.include?(method_name) + + errors << generate_error(method_name, target_class, delegator_class) + end + end + + return if errors.empty? + + details = errors.map { |error| "- #{error}" }.join("\n") + + raise UnexpectedDelegatorOverrideError, + <<~TEXT + #{OVERRIDE_ERROR_MESSAGE} + Here are the conflict details. + + #{details} + TEXT + end + + private + + def generate_error(method_name, target_class, delegator_class) + target_location = extract_location(target_class, method_name) + delegator_location = extract_location(delegator_class, method_name) + Error.new(method_name, target_class, target_location, delegator_class, delegator_location) + end + + def extract_location(klass, method_name) + klass.instance_method(method_name).source_location&.join(':') || 'unknown' + end + + def allowlist + [].tap do |allowed| + allowed.concat(allowed_method_names) + allowed.concat(Object.instance_methods) + allowed.concat(::Delegator.instance_methods) + end + end + end + end + end +end diff --git a/lib/gitlab/view/presenter/base.rb b/lib/gitlab/view/presenter/base.rb index 9dc687f7740f6d5fa4c71ccd2f39f688b6881b9f..3bacad720500d92a66eb0896b32b6181a464e763 100644 --- a/lib/gitlab/view/presenter/base.rb +++ b/lib/gitlab/view/presenter/base.rb @@ -47,8 +47,18 @@ def presenter? true end - def presents(name) - define_method(name) { subject } + def presents(*target_classes, as: nil) + if target_classes.any? { |k| k.is_a?(Symbol) } + raise ArgumentError, "Unsupported target class type: #{target_classes}." + end + + if self < ::Gitlab::View::Presenter::Delegated + target_classes.each { |k| delegator_target(k) } + elsif self < ::Gitlab::View::Presenter::Simple + # no-op + end + + define_method(as) { subject } if as end end end diff --git a/lib/gitlab/view/presenter/delegated.rb b/lib/gitlab/view/presenter/delegated.rb index d14f8cc4e5edbbfc0452a3cd585f34e9052f48b0..259cf0cf457f8fea785b37d829265152bfe00607 100644 --- a/lib/gitlab/view/presenter/delegated.rb +++ b/lib/gitlab/view/presenter/delegated.rb @@ -4,7 +4,18 @@ module Gitlab module View module Presenter class Delegated < SimpleDelegator + extend ::Gitlab::Utils::DelegatorOverride + + # TODO: Stop including auxiliary methods/modules in `Presenter::Base` as + # it overrides many methods in the Active Record models. + # See https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/presenters/README.md#validate-accidental-overrides + # for more information. include Gitlab::View::Presenter::Base + delegator_override_with Gitlab::Routing.url_helpers + delegator_override :can? + delegator_override :declarative_policy_delegate + delegator_override :present + delegator_override :web_url def initialize(subject, **attributes) @subject = subject diff --git a/lib/tasks/lint.rake b/lib/tasks/lint.rake index 976ec089011533382763b859f75d68632596a0b1..7d1721f1df8d3a8f222a0fbc93065319c97c8dd7 100644 --- a/lib/tasks/lint.rake +++ b/lib/tasks/lint.rake @@ -12,6 +12,7 @@ unless Rails.env.production? dev:load ] do Gitlab::Utils::Override.verify! + Gitlab::Utils::DelegatorOverride.verify! end desc "GitLab | Lint | Lint JavaScript files using ESLint" diff --git a/spec/lib/gitlab/utils/delegator_override/error_spec.rb b/spec/lib/gitlab/utils/delegator_override/error_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..59b67676eff4e19bb89c67a029418ae92dfb9565 --- /dev/null +++ b/spec/lib/gitlab/utils/delegator_override/error_spec.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +RSpec.describe Gitlab::Utils::DelegatorOverride::Error do + let(:error) { described_class.new('foo', 'Target', '/path/to/target', 'Delegator', '/path/to/delegator') } + + describe '#to_s' do + subject { error.to_s } + + it { is_expected.to eq("Delegator#foo is overriding Target#foo. delegator_location: /path/to/delegator target_location: /path/to/target") } + end +end diff --git a/spec/lib/gitlab/utils/delegator_override/validator_spec.rb b/spec/lib/gitlab/utils/delegator_override/validator_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..4cd1b18de82ba9922e953a4d4f3075c9856deeb9 --- /dev/null +++ b/spec/lib/gitlab/utils/delegator_override/validator_spec.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +RSpec.describe Gitlab::Utils::DelegatorOverride::Validator do + let(:delegator_class) do + Class.new(::SimpleDelegator) do + extend(::Gitlab::Utils::DelegatorOverride) + + def foo + end + end.prepend(ee_delegator_extension) + end + + let(:ee_delegator_extension) do + Module.new do + extend(::Gitlab::Utils::DelegatorOverride) + + def bar + end + end + end + + let(:target_class) do + Class.new do + def foo + end + + def bar + end + end + end + + let(:validator) { described_class.new(delegator_class) } + + describe '#add_allowlist' do + it 'adds a method name to the allowlist' do + validator.add_allowlist([:foo]) + + expect(validator.allowed_method_names).to contain_exactly(:foo) + end + end + + describe '#add_target' do + it 'adds the target class' do + validator.add_target(target_class) + + expect(validator.target_classes).to contain_exactly(target_class) + end + end + + describe '#expand_on_ancestors' do + it 'adds the allowlist in the ancestors' do + ancestor_validator = described_class.new(ee_delegator_extension) + ancestor_validator.add_allowlist([:bar]) + validator.expand_on_ancestors( { ee_delegator_extension => ancestor_validator }) + + expect(validator.allowed_method_names).to contain_exactly(:bar) + end + end + + describe '#validate_overrides!' do + before do + validator.add_target(target_class) + end + + it 'does not raise an error when the overrides are allowed' do + validator.add_allowlist([:foo]) + ancestor_validator = described_class.new(ee_delegator_extension) + ancestor_validator.add_allowlist([:bar]) + validator.expand_on_ancestors( { ee_delegator_extension => ancestor_validator }) + + expect { validator.validate_overrides! }.not_to raise_error + end + + it 'raises an error when there is an override' do + expect { validator.validate_overrides! } + .to raise_error(described_class::UnexpectedDelegatorOverrideError) + end + end +end diff --git a/spec/lib/gitlab/utils/delegator_override_spec.rb b/spec/lib/gitlab/utils/delegator_override_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..af4c7fa5d8ec9170a284d716222c9373bd378d02 --- /dev/null +++ b/spec/lib/gitlab/utils/delegator_override_spec.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +RSpec.describe Gitlab::Utils::DelegatorOverride do + let(:delegator_class) do + Class.new(::SimpleDelegator) do + extend(::Gitlab::Utils::DelegatorOverride) + + def foo + end + end + end + + let(:target_class) do + Class.new do + def foo + end + + def bar + end + end + end + + let(:dummy_module) do + Module.new do + def foobar + end + end + end + + before do + stub_env('STATIC_VERIFICATION', 'true') + end + + describe '.delegator_target' do + subject { delegator_class.delegator_target(target_class) } + + it 'sets the delegator target to the validator' do + expect(described_class.validator(delegator_class)) + .to receive(:add_target).with(target_class) + + subject + end + + context 'when the class does not inherit SimpleDelegator' do + let(:delegator_class) do + Class.new do + extend(::Gitlab::Utils::DelegatorOverride) + end + end + + it 'raises an error' do + expect { subject }.to raise_error(ArgumentError, /not a subclass of 'SimpleDelegator' class/) + end + end + end + + describe '.delegator_override' do + subject { delegator_class.delegator_override(:foo) } + + it 'adds the method name to the allowlist' do + expect(described_class.validator(delegator_class)) + .to receive(:add_allowlist).with([:foo]) + + subject + end + end + + describe '.delegator_override_with' do + subject { delegator_class.delegator_override_with(dummy_module) } + + it 'adds the method names of the module to the allowlist' do + expect(described_class.validator(delegator_class)) + .to receive(:add_allowlist).with([:foobar]) + + subject + end + end + + describe '.verify!' do + subject { described_class.verify! } + + it 'does not raise an error when an override is in allowlist' do + delegator_class.delegator_target(target_class) + delegator_class.delegator_override(:foo) + + expect { subject }.not_to raise_error + end + + it 'raises an error when there is an override' do + delegator_class.delegator_target(target_class) + + expect { subject }.to raise_error(Gitlab::Utils::DelegatorOverride::Validator::UnexpectedDelegatorOverrideError) + end + end +end diff --git a/spec/lib/gitlab/view/presenter/base_spec.rb b/spec/lib/gitlab/view/presenter/base_spec.rb index 97d5e2b280de4d6cb210d157192196edd5d69d2b..a7083bd272231955c8d2a752814d55dea6ee8fb9 100644 --- a/spec/lib/gitlab/view/presenter/base_spec.rb +++ b/spec/lib/gitlab/view/presenter/base_spec.rb @@ -18,11 +18,43 @@ describe '.presents' do it 'exposes #subject with the given keyword' do - presenter_class.presents(:foo) + presenter_class.presents(Object, as: :foo) presenter = presenter_class.new(project) expect(presenter.foo).to eq(project) end + + it 'raises an error when symbol is passed' do + expect { presenter_class.presents(:foo) }.to raise_error(ArgumentError) + end + + context 'when the presenter class inherits Presenter::Delegated' do + let(:presenter_class) do + Class.new(::Gitlab::View::Presenter::Delegated) do + include(::Gitlab::View::Presenter::Base) + end + end + + it 'sets the delegator target' do + expect(presenter_class).to receive(:delegator_target).with(Object) + + presenter_class.presents(Object, as: :foo) + end + end + + context 'when the presenter class inherits Presenter::Simple' do + let(:presenter_class) do + Class.new(::Gitlab::View::Presenter::Simple) do + include(::Gitlab::View::Presenter::Base) + end + end + + it 'does not set the delegator target' do + expect(presenter_class).not_to receive(:delegator_target).with(Object) + + presenter_class.presents(Object, as: :foo) + end + end end describe '#can?' do