DEV Community

Pavan Kumar Sadashiv
Pavan Kumar Sadashiv

Posted on Edited on

Part 2: Decoupling EE::Group URL Routing into Ai::DuoSettings::Paths Across 22 Files

#gitlab #ruby #rails #architecture

Series Context: Part 2 of 10 in GitLab Core Engineering: Architecture, MCP & CI/CD Deep Dives.

Featured MR: gitlab-org/gitlab!269059 · Work Item: #636678

Models Should Know Identity, Not URL Routing Anchors

ActiveRecord models in Rails should not construct Rails URL helper paths with UI DOM anchors hardcoded into them. Yet inside ee/app/models/ee/group.rb, duo_settings_path had quietly lived for years:

# Before (ee/app/models/ee/group.rb)
def duo_settings_path
  if ::GitlabSubscriptions::SubscriptionHelper.gitlab_com_subscription? && root?
    ::Gitlab::Routing.url_helpers.group_settings_gitlab_duo_configuration_index_path(self)
  else
    ::Gitlab::Routing.url_helpers.edit_group_path(self, anchor: 'js-gitlab-duo-settings')
  end
end
Enter fullscreen mode Exit fullscreen mode

That 7-line method violates multiple architectural boundaries at once:

  1. It couples the core EE::Group persistence model to ::GitlabSubscriptions::SubscriptionHelper, ::Gitlab::Routing.url_helpers, and the DOM anchor #js-gitlab-duo-settings.
  2. It bloats EE::Group's Gitlab/OmniscientClassMethods baseline (ee_group/legacy_methods.yml).

In !269059, we moved this routing responsibility to a dedicated bounded-context module Ai::DuoSettings::Paths across 22 files.

Creating Ai::DuoSettings::Paths.group_path

We extracted the logic into a pure module_function in ee/lib/ai/duo_settings/paths.rb:

# frozen_string_literal: true

module Ai
  module DuoSettings
    module Paths
      module_function

      def group_path(group)
        return unless group

        if ::GitlabSubscriptions::SubscriptionHelper.gitlab_com_subscription? && group.root?
          ::Gitlab::Routing.url_helpers.group_settings_gitlab_duo_configuration_index_path(group)
        else
          ::Gitlab::Routing.url_helpers.edit_group_path(group, anchor: 'js-gitlab-duo-settings')
        end
      end
    end
  end
end
Enter fullscreen mode Exit fullscreen mode

Look closely at line 9: return unless group.

Why does that single guard matter so much? Compare the call sites before and after in ee/app/helpers/ee/integrations_helper.rb:

# Before: caller had to guard nil via Ruby safe navigation
group_settings_path: duo_group&.duo_settings_path

# After: nil safety lives once inside group_path(group)
group_settings_path: ::Ai::DuoSettings::Paths.group_path(duo_group)
Enter fullscreen mode Exit fullscreen mode

Moving the nil guard into group_path prevents every caller (especially integration helpers and Slack bots where duo_group can legitimately be nil) from repeating if duo_group ternaries.

Mapping All Call Surfaces Across the Stack

Removing a method from EE::Group touches every presentation layer in GitLab. In this refactor, callers spanned four distinct architectural layers:

Layer File Change
ViewComponent ee/app/components/duo_chat_panel/container.rb Replaced record.duo_settings_path with ::Ai::DuoSettings::Paths.group_path(record)
GraphQL Type ee/app/graphql/ee/types/group_type.rb Added explicit def duo_settings_path; ::Ai::DuoSettings::Paths.group_path(object); end field resolver
Rails Helpers (5) ee/app/helpers/ee/groups/settings_helper.rb
ee/app/helpers/ee/integrations_helper.rb (2 callers)
ee/app/helpers/ee/projects/duo_agents_platform_helper.rb
ee/app/helpers/ee/projects/security/configuration_helper.rb
ee/app/helpers/groups/security_features_helper.rb 		Switched group.duo_settings_path, root_ancestor.duo_settings_path, and root_group.duo_settings_path to ::Ai::DuoSettings::Paths.group_path(...)
Integration Services ee/app/services/ee/integrations/slack_events/app_mentioned_service.rb Passed ::Ai::DuoSettings::Paths.group_path(namespace) into Gitlab::Utils.append_path
RuboCop Baseline rubocop/cop/gitlab/config/omniscient_classes/ee_group/legacy_methods.yml Deleted - duo_settings_path entry

Pay Special Attention to GraphQL Types (EE::Types::GroupType)

Why did we have to add def duo_settings_path inside ee/app/graphql/ee/types/group_type.rb?
Because GraphQL field declarations (field :duo_settings_path, ...) default to calling the matching method name on object (the underlying Group model)! Once EE::Group#duo_settings_path is removed, GraphQL queries selecting group { duoSettingsPath } would raise NoMethodError unless GroupType defines an explicit field resolver delegating to ::Ai::DuoSettings::Paths.group_path(object).

We backed this contract directly in ee/spec/graphql/ee/types/group_type_spec.rb:

describe '#duo_settings_path' do
  include GraphqlHelpers

  let_it_be(:user) { create(:user) }
  let_it_be(:group) { create(:group, owners: user) }

  it 'delegates to Ai::DuoSettings::Paths.group_path' do
    expect(::Ai::DuoSettings::Paths).to receive(:group_path).with(group).and_return('/group/duo-settings')

    expect(resolve_field(:duo_settings_path, group, current_user: user)).to eq('/group/duo-settings')
  end
end
Enter fullscreen mode Exit fullscreen mode

Surviving a Live Race With upstream/master

If you have ever refactored a heavily called method in gitlab-org/gitlab, you know the classic hazard: between the moment you cut your branch and the moment a reviewer approves it, someone merges a new caller onto master.

During review on !269059, maintainer @rliu-gl spotted two critical updates on master:

  1. ee/app/helpers/ee/integrations_helper.rb gained a brand-new caller (renamed from slack_duo_instance_data to slack_duo_admin_data(organization)) invoking duo_group&.duo_settings_path—which was NOT in our initial branch cut!
  2. ee/spec/helpers/groups/security/configuration_helper_spec.rb had a merge conflict from concurrent work.

Had !269059 merged without rebasing and re-grepping master, production slack_duo_admin_data would have broken with undefined method 'duo_settings_path' for #<Group> immediately after merge!

The fix in commit 931ece1724:

  1. Rebased onto fresh upstream/master and resolved the helper spec conflict.
  2. Re-ran repo-wide git grep -n "duo_settings_path" after rebasing, catching slack_duo_admin_data (ee/app/helpers/ee/integrations_helper.rb:135) and updating it to ::Ai::DuoSettings::Paths.group_path(duo_group).
  3. Verified all 22 touched files and got a 100% green pipeline (#3053240087).

Takeaways for GitLab Engineers

  1. Always check GraphQL type classes when deleting model methods: GraphQL resolvers implicitly delegate to object.<field_name> unless overridden on the type (EE::Types::GroupType).
  2. Always re-grep the entire repo after every rebase on master: Removing a method from a core model (Group, Project, User) makes every new caller merged while your MR is in review a latent NoMethodError in production.
  3. Put nil guards inside the extracted path helper (return unless group), not scattered &.method at call sites.

Series Navigation

Top comments (0)