## Summary In https://gitlab.com/gitlab-org/gitlab/-/merge_requests/224189+s , `Groups::VirtualRegistriesController` reimplemented availability checks that already exist in `VirtualRegistries::Packages::Maven` and `VirtualRegistries::Container` models [noticing that](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/224189#note_3103946877) the check `Maven.virtual_registry_available? || Container.virtual_registry_available?` calls multiple methods twice in [packages_registries_menu](https://gitlab.com/gitlab-org/gitlab/-/blob/f58bb003eeb80e0137be6755eec591ce7b07a049/ee/lib/ee/sidebars/groups/menus/packages_registries_menu.rb#L37-40) ## Current state ### Model implementations (nearly identical) Both `VirtualRegistries::Packages::Maven` ([`ee/app/models/virtual_registries/packages/maven.rb`]) and `VirtualRegistries::Container` ([`ee/app/models/virtual_registries/container.rb`]) define the same three class methods: - `feature_enabled?(group)` — checks `dependency_proxy_feature_available?`, a type-specific feature flag, a type-specific licensed feature, and `VirtualRegistries::Setting.find_for_group(group).enabled` - `user_has_access?(group, current_user, permission)` — checks `Ability.allowed?` - `virtual_registry_available?(group, current_user, permission)` — combines both The **only differences** are the feature flag name and licensed feature name: | Module | Feature flag | Licensed feature | |---|---|---| | `Packages::Maven` | `:maven_virtual_registry` | `:packages_virtual_registry` | | `Container` | `:container_virtual_registries` | `:container_virtual_registry` | ## Proposal ### Option A: Controller delegates to model methods Replace the 5 reimplemented controller methods with direct delegation similar to `packages_registries_menu` ```ruby # TODO: Once :ui_for_container_virtual_registries feature flag is removed, # simplify to: ::VirtualRegistries::Packages::Maven.virtual_registry_available?(group, current_user) || # ::VirtualRegistries::Container.virtual_registry_available?(group, current_user) def feature_available? ::VirtualRegistries::Packages::Maven.virtual_registry_available?(group, current_user) || (::Feature.enabled?(:ui_for_container_virtual_registries, group) && ::VirtualRegistries::Container.virtual_registry_available?(group, current_user)) end ``` ### Option B: Moderate — Option A + shared `Availability` concern Extract the identical model methods into `ee/app/models/concerns/virtual_registries/availability.rb`: ```ruby module VirtualRegistries module Availability def feature_enabled?(group) group.dependency_proxy_feature_available? && ::Feature.enabled?(feature_flag_name, group) && group.licensed_feature_available?(licensed_feature_name) && ::VirtualRegistries::Setting.find_for_group(group).enabled end def user_has_access?(group, current_user, permission = :read_virtual_registry) Ability.allowed?(current_user, permission, group.virtual_registry_policy_subject) end def virtual_registry_available?(group, current_user, permission = :read_virtual_registry) feature_enabled?(group) && user_has_access?(group, current_user, permission) end end end ``` Each module would `extend VirtualRegistries::Availability` and define `feature_flag_name` / `licensed_feature_name`. ### Option C: Full — Option B + `VirtualRegistries.any_registry_available?` facade Add a facade method to `ee/app/models/virtual_registries.rb`: ```ruby def self.any_registry_available?(group, current_user, permission = :read_virtual_registry) Packages::Maven.virtual_registry_available?(group, current_user, permission) || Container.virtual_registry_available?(group, current_user, permission) end ``` Update sidebar to use the facade. Controller would use it after `:ui_for_container_virtual_registries` flag removal. **Risk:** Low — but the facade delegates to module methods, so shared checks (dependency_proxy, Setting, Ability) run twice when Maven check fails and Container is tried. See performance section below. ### Option D: Facade checks shared prerequisites once Build on Option B (shared concern), but split `feature_enabled?` into shared and type-specific parts. The facade checks shared prerequisites once, then only evaluates type-specific flags: ```ruby # In VirtualRegistries::Availability concern module VirtualRegistries module Availability def feature_enabled?(group) shared_feature_enabled?(group) && type_specific_feature_enabled?(group) end def shared_feature_enabled?(group) group.dependency_proxy_feature_available? && ::VirtualRegistries::Setting.find_for_group(group).enabled end def type_specific_feature_enabled?(group) ::Feature.enabled?(feature_flag_name, group) && group.licensed_feature_available?(licensed_feature_name) end def user_has_access?(group, current_user, permission = :read_virtual_registry) Ability.allowed?(current_user, permission, group.virtual_registry_policy_subject) end def virtual_registry_available?(group, current_user, permission = :read_virtual_registry) feature_enabled?(group) && user_has_access?(group, current_user, permission) end end end ``` ```ruby # In VirtualRegistries module (facade) def self.any_registry_available?(group, current_user, permission = :read_virtual_registry) # Check shared prerequisites once return false unless Packages::Maven.shared_feature_enabled?(group) return false unless Packages::Maven.user_has_access?(group, current_user, permission) # Only check type-specific flags Packages::Maven.type_specific_feature_enabled?(group) || Container.type_specific_feature_enabled?(group) end ``` **Tradeoff:** 1 `Setting.find_for_group` call instead of 2) at the cost of exposing `shared_feature_enabled?` / `type_specific_feature_enabled?` in the concern's public API. ## Open questions ### 1. Request-level caching for `VirtualRegistries::Setting.find_for_group` `Setting.find_for_group(group)` ([`ee/app/models/virtual_registries/setting.rb`]) calls `find_or_initialize_by(group: group)` — a DB query with **no explicit memoization**. When multiple `virtual_registry_available?` checks run in the same request (e.g., sidebar checking both Maven and Container), this produces duplicate queries. Rails query cache mitigates this per-request, but explicit `SafeRequestStore` caching would eliminate even that: ```ruby def self.find_for_group(group) Gitlab::SafeRequestStore.fetch("virtual_registries:setting:#{group.id}") do find_or_initialize_by(group: group) end end ``` **Should this be included in scope or tracked separately?** ### 2. Performance of duplicate shared checks in the facade (Option C) When using `Maven.virtual_registry_available? || Container.virtual_registry_available?`, shared prerequisites are checked twice if Maven fails: | Check | Cost | Caching mechanism | |---|---|---| | `dependency_proxy_feature_available?` | Negligible | Config read from memory | | `Feature.enabled?` | Low | Memory → Redis → DB (multi-level) | | `licensed_feature_available?` | Low | `strong_memoize` on group instance | | `Ability.allowed?` | Medium (first call) | `SafeRequestStore` (request-scoped) | | `Setting.find_for_group` | **DB query** | **Rails query cache only** | Most checks are cached and the duplication is negligible in practice. The `Setting.find_for_group` call is the only concern, addressed by open question #1. **Is this acceptable, or should the facade optimize by checking shared prerequisites once like Option D suggests?**