Maven VReg: Move the cache_validity_period column to the upstreams table

app/models/virtual_registries/packages/maven/cached_response.rb

@@ -80,14 +80,11 @@ def filename
           File.basename(relative_path)
         end
 
-        # The registry parameter is there to counter a bug with has_one :through records that will fire an extra
-        # database query.
-        # See https://github.com/rails/rails/issues/51817.
-        def stale?(registry:)
-          return true unless registry
-          return false if registry.cache_validity_hours == 0
-
-          (upstream_checked_at + registry.cache_validity_hours.hours).past?
+        def stale?
+          return true unless upstream
+          return false if upstream.cache_validity_hours == 0
+
+          (upstream_checked_at + upstream.cache_validity_hours.hours).past?
         end
 
         def bump_statistics(include_upstream_checked_at: false)

app/models/virtual_registries/packages/maven/registry.rb

@@ -4,6 +4,8 @@ module VirtualRegistries
   module Packages
     module Maven
       class Registry < ApplicationRecord
+        ignore_column :cache_validity_hours, remove_with: '17.8', remove_after: '2024-12-23'
+
         belongs_to :group
         has_one :registry_upstream,
           class_name: 'VirtualRegistries::Packages::Maven::RegistryUpstream',
@@ -11,7 +13,6 @@ class Registry < ApplicationRecord
         has_one :upstream, class_name: 'VirtualRegistries::Packages::Maven::Upstream', through: :registry_upstream
 
         validates :group, top_level_group: true, presence: true, uniqueness: true
-        validates :cache_validity_hours, numericality: { greater_than_or_equal_to: 0, only_integer: true }
 
         scope :for_group, ->(group) { where(group: group) }
 

app/models/virtual_registries/packages/maven/upstream.rb

@@ -29,6 +29,7 @@ class Upstream < ApplicationRecord
         validates :username, presence: true, if: :password?
         validates :password, presence: true, if: :username?
         validates :url, :username, :password, length: { maximum: 255 }
+        validates :cache_validity_hours, numericality: { greater_than_or_equal_to: 0, only_integer: true }
 
         after_initialize :read_credentials
         after_validation :reset_credentials, if: -> { persisted? && url_changed? }

app/services/virtual_registries/packages/maven/handle_file_request_service.rb

@@ -64,7 +64,7 @@ def cached_response
         def cache_response_still_valid?
           return false unless cached_response
 
-          unless cached_response.stale?(registry: registry)
+          unless cached_response.stale?
             cached_response.bump_statistics
             return true
           end

db/migrate/20241024095104_add_cache_validity_hours_to_virtual_registries_packages_maven_upstreams.rb

+# frozen_string_literal: true
+
+class AddCacheValidityHoursToVirtualRegistriesPackagesMavenUpstreams < Gitlab::Database::Migration[2.2]
+  milestone '17.6'
+  disable_ddl_transaction!
+
+  TABLE_NAME = :virtual_registries_packages_maven_upstreams
+
+  def up
+    with_lock_retries do
+      add_column TABLE_NAME, :cache_validity_hours, :smallint, default: 24, null: false, if_not_exists: true
+    end
+
+    constraint = check_constraint_name(TABLE_NAME.to_s, 'cache_validity_hours', 'zero_or_positive')
+    add_check_constraint(TABLE_NAME, 'cache_validity_hours >= 0', constraint)
+  end
+
+  def down
+    remove_column TABLE_NAME, :cache_validity_hours, if_exists: true
+  end
+end

db/schema_migrations/20241024095104

+b69d8e3e59a4bbb1f79869e153e8a05412facf59406bd8378281942d6d7e963c
\ No newline at end of file

db/structure.sql

@@ -20304,8 +20304,10 @@ CREATE TABLE virtual_registries_packages_maven_upstreams (
     url text NOT NULL,
     encrypted_credentials bytea,
     encrypted_credentials_iv bytea,
+    cache_validity_hours smallint DEFAULT 24 NOT NULL,
     CONSTRAINT check_26c0572777 CHECK ((char_length(url) <= 255)),
     CONSTRAINT check_4af2999ab8 CHECK ((octet_length(encrypted_credentials_iv) <= 1020)),
+    CONSTRAINT check_a3593dca3a CHECK ((cache_validity_hours >= 0)),
     CONSTRAINT check_b9e3bfa31a CHECK ((octet_length(encrypted_credentials) <= 1020))
 );

lib/api/concerns/virtual_registries/packages/maven/registry_endpoints.rb

@@ -52,7 +52,6 @@ module RegistryEndpoints
               params do
                 requires :group_id, type: Integer, desc: 'The ID of the group. Must be a top-level group',
                   allow_blank: false
-                optional :cache_validity_hours, type: Integer, desc: 'The validity of the cache in hours. Defaults to 1'
               end
               post do
                 group = find_group!(declared_params[:group_id])
@@ -86,31 +85,6 @@ module RegistryEndpoints
                   present registry, with: ::API::Entities::VirtualRegistries::Packages::Maven::Registry
                 end
 
-                desc 'Update a specific maven virtual registry' do
-                  detail 'This feature was introduced in GitLab 17.4. \
-                    This feature is currently in an experimental state. \
-                    This feature is behind the `virtual_registry_maven` feature flag.'
-                  success ::API::Entities::VirtualRegistries::Packages::Maven::Registry
-                  failure [
-                    { code: 400, message: 'Bad request' },
-                    { code: 401, message: 'Unauthorized' },
-                    { code: 403, message: 'Forbidden' },
-                    { code: 404, message: 'Not found' }
-                  ]
-                  tags %w[maven_virtual_registries]
-                  hidden true
-                end
-                params do
-                  requires :cache_validity_hours, type: Integer, desc: 'The validity of the cache in hours'
-                end
-                patch do
-                  authorize! :update_virtual_registry, registry
-
-                  render_validation_error!(registry) unless registry.update(declared_params)
-
-                  present registry, with: ::API::Entities::VirtualRegistries::Packages::Maven::Registry
-                end
-
                 desc 'Delete a specific maven virtual registry' do
                   detail 'This feature was introduced in GitLab 17.4. \
                     This feature is currently in an experimental state. \

lib/api/concerns/virtual_registries/packages/maven/upstream_endpoints.rb

@@ -48,6 +48,7 @@ module UpstreamEndpoints
                 requires :url, type: String, desc: 'The URL of the maven virtual registry upstream', allow_blank: false
                 optional :username, type: String, desc: 'The username of the maven virtual registry upstream'
                 optional :password, type: String, desc: 'The password of the maven virtual registry upstream'
+                optional :cache_validity_hours, type: Integer, desc: 'The cache validity in hours. Defaults to 24'
                 all_or_none_of :username, :password
               end
               post do
@@ -55,7 +56,7 @@ module UpstreamEndpoints
 
                 conflict!(_('Upstream already exists')) if upstream
 
-                registry.build_upstream(declared_params.merge(group: group))
+                registry.build_upstream(declared_params(include_missing: false).merge(group: group))
                 registry_upstream.group = group
 
                 ApplicationRecord.transaction do
@@ -106,13 +107,14 @@ module UpstreamEndpoints
                   hidden true
                 end
                 params do
-                  optional :url, type: String, desc: 'The URL of the maven virtual registry upstream',
-                    allow_blank: false
-                  optional :username, type: String, desc: 'The username of the maven virtual registry upstream',
-                    allow_blank: false
-                  optional :password, type: String, desc: 'The password of the maven virtual registry upstream',
-                    allow_blank: false
-                  at_least_one_of :url, :username, :password
+                  with(allow_blank: false) do
+                    optional :url, type: String, desc: 'The URL of the maven virtual registry upstream'
+                    optional :username, type: String, desc: 'The username of the maven virtual registry upstream'
+                    optional :password, type: String, desc: 'The password of the maven virtual registry upstream'
+                    optional :cache_validity_hours, type: Integer, desc: 'The validity of the cache in hours'
+                  end
+
+                  at_least_one_of :url, :username, :password, :cache_validity_hours
                 end
                 patch do
                   authorize! :update_virtual_registry, registry

lib/api/entities/virtual_registries/packages/maven/registry.rb

@@ -6,7 +6,7 @@ module VirtualRegistries
       module Packages
         module Maven
           class Registry < Grape::Entity
-            expose :id, :group_id, :cache_validity_hours, :created_at, :updated_at
+            expose :id, :group_id, :created_at, :updated_at
           end
         end
       end

lib/api/entities/virtual_registries/packages/maven/upstream.rb

@@ -6,7 +6,7 @@ module VirtualRegistries
       module Packages
         module Maven
           class Upstream < Grape::Entity
-            expose :id, :group_id, :url, :created_at, :updated_at
+            expose :id, :group_id, :url, :cache_validity_hours, :created_at, :updated_at
           end
         end
       end

spec/factories/virtual_registries/packages/maven/registries.rb

@@ -3,7 +3,6 @@
 FactoryBot.define do
   factory :virtual_registries_packages_maven_registry, class: 'VirtualRegistries::Packages::Maven::Registry' do
     group
-    cache_validity_hours { 1 }
 
     trait :with_upstream do
       upstream { association(:virtual_registries_packages_maven_upstream, group: group) }

spec/factories/virtual_registries/packages/maven/upstreams.rb

@@ -7,6 +7,7 @@
     password { 'password' }
     registry { association(:virtual_registries_packages_maven_registry) }
     group { registry.group }
+    cache_validity_hours { 24 }
 
     after(:build) do |entry, _|
       entry.registry_upstream.group = entry.group if entry.registry_upstream

spec/lib/api/entities/virtual_registries/packages/maven/registry_spec.rb

@@ -7,5 +7,5 @@
 
   subject { described_class.new(registry).as_json }
 
-  it { is_expected.to include(:id, :group_id, :cache_validity_hours, :created_at, :updated_at) }
+  it { is_expected.to include(:id, :group_id, :created_at, :updated_at) }
 end

spec/lib/api/entities/virtual_registries/packages/maven/upstream_spec.rb

@@ -7,5 +7,5 @@
 
   subject { described_class.new(upstream).as_json }
 
-  it { is_expected.to include(:id, :group_id, :url, :created_at, :updated_at) }
+  it { is_expected.to include(:id, :group_id, :url, :cache_validity_hours, :created_at, :updated_at) }
 end

spec/models/virtual_registries/packages/maven/cached_response_spec.rb

@@ -235,10 +235,10 @@
     end
 
     let(:threshold) do
-      cached_response.upstream_checked_at + cached_response.upstream.registry.cache_validity_hours.hours
+      cached_response.upstream_checked_at + cached_response.upstream.cache_validity_hours.hours
     end
 
-    subject { cached_response.stale?(registry: cached_response.upstream.registry) }
+    subject { cached_response.stale? }
 
     context 'when before the threshold' do
       before do
@@ -264,9 +264,9 @@
       it { is_expected.to eq(true) }
     end
 
-    context 'with no registry' do
+    context 'with no upstream' do
       before do
-        cached_response.upstream.registry = nil
+        cached_response.upstream = nil
       end
 
       it { is_expected.to eq(true) }
@@ -274,7 +274,7 @@
 
     context 'with 0 cache validity hours' do
       before do
-        cached_response.upstream.registry.cache_validity_hours = 0
+        cached_response.upstream.cache_validity_hours = 0
       end
 
       it { is_expected.to eq(false) }

spec/models/virtual_registries/packages/maven/registry_spec.rb

@@ -24,7 +24,6 @@
   describe 'validations' do
     it { is_expected.to validate_uniqueness_of(:group) }
     it { is_expected.to validate_presence_of(:group) }
-    it { is_expected.to validate_numericality_of(:cache_validity_hours).only_integer.is_greater_than_or_equal_to(0) }
   end
 
   describe '.for_group' do

spec/models/virtual_registries/packages/maven/upstream_spec.rb

@@ -39,6 +39,7 @@
     it { is_expected.to validate_length_of(:url).is_at_most(255) }
     it { is_expected.to validate_length_of(:username).is_at_most(255) }
     it { is_expected.to validate_length_of(:password).is_at_most(255) }
+    it { is_expected.to validate_numericality_of(:cache_validity_hours).only_integer.is_greater_than_or_equal_to(0) }
 
     context 'for url' do
       where(:url, :valid, :error_messages) do

spec/requests/api/virtual_registries/packages/maven_spec.rb

@@ -172,14 +172,11 @@
         expect { api_request }.to change { registry_class.count }.by(1)
 
         expect(registry_class.last.group_id).to eq(params[:group_id])
-        expect(registry_class.last.cache_validity_hours).to eq(
-          params[:cache_validity_hours] || registry_class.new.cache_validity_hours
-        )
       end
     end
 
     context 'with valid params' do
-      let(:params) { { group_id: group.id, cache_validity_hours: 24 } }
+      let(:params) { { group_id: group.id } }
 
       it { is_expected.to have_request_urgency(:low) }
 
@@ -208,17 +205,6 @@
         end
       end
 
-      context 'without cache_validity_hours param' do
-        let(:params) { { group_id: group.id } }
-
-        before_all do
-          registry_class.for_group(group).delete_all
-          group.add_maintainer(user)
-        end
-
-        it_behaves_like 'successful response'
-      end
-
       context 'with existing registry' do
         before_all do
           group.add_maintainer(user)
@@ -266,22 +252,18 @@
     end
 
     context 'with invalid params' do
-      let(:valid_group_id) { group.id }
-
       before_all do
         group.add_maintainer(user)
       end
 
-      where(:group_id, :cache_validity_hours, :status) do
-        non_existing_record_id | 1   | :not_found
-        'foo'                  | 1   | :bad_request
-        ''                     | 1   | :bad_request
-        ref(:valid_group_id)   | 'a' | :bad_request
-        ref(:valid_group_id)   | -1  | :bad_request
+      where(:group_id, :status) do
+        non_existing_record_id  | :not_found
+        'foo'                   | :bad_request
+        ''                      | :bad_request
       end
 
       with_them do
-        let(:params) { { group_id: group_id, cache_validity_hours: cache_validity_hours } }
+        let(:params) { { group_id: group_id } }
 
         it_behaves_like 'returning response status', params[:status]
       end
@@ -290,7 +272,7 @@
     context 'with subgroup' do
       let(:subgroup) { create(:group, parent: group, visibility_level: group.visibility_level) }
 
-      let(:params) { { group_id: subgroup.id, cache_validity_hours: 1 } }
+      let(:params) { { group_id: subgroup.id } }
 
       before_all do
         group.add_maintainer(user)
@@ -384,104 +366,6 @@
     end
   end
 
-  describe 'PATCH /api/v4/virtual_registries/packages/maven/registries/:id' do
-    let(:registry_id) { registry.id }
-    let(:url) { "/virtual_registries/packages/maven/registries/#{registry_id}" }
-
-    subject(:api_request) { patch api(url), headers: headers, params: params }
-
-    shared_examples 'successful response' do
-      it 'returns a successful response' do
-        api_request
-
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(json_response['cache_validity_hours']).to eq(registry.reset.cache_validity_hours)
-      end
-    end
-
-    context 'with valid params' do
-      let(:params) { { cache_validity_hours: 2 } }
-
-      it { is_expected.to have_request_urgency(:low) }
-
-      it_behaves_like 'disabled feature flag'
-      it_behaves_like 'disabled dependency proxy'
-      it_behaves_like 'not authenticated user'
-
-      where(:user_role, :status) do
-        :owner      | :ok
-        :maintainer | :ok
-        :developer  | :forbidden
-        :reporter   | :forbidden
-        :guest      | :forbidden
-      end
-
-      with_them do
-        before do
-          group.send(:"add_#{user_role}", user)
-        end
-
-        if params[:status] == :ok
-          it_behaves_like 'successful response'
-        else
-          it_behaves_like 'returning response status', params[:status]
-        end
-      end
-
-      context 'for authentication' do
-        before_all do
-          group.add_maintainer(user)
-        end
-
-        where(:token, :sent_as, :status) do
-          :personal_access_token | :header     | :ok
-          :personal_access_token | :basic_auth | :ok
-          :deploy_token          | :header     | :forbidden
-          :deploy_token          | :basic_auth | :forbidden
-          :job_token             | :header     | :ok
-          :job_token             | :basic_auth | :ok
-        end
-
-        with_them do
-          let(:headers) do
-            case sent_as
-            when :header
-              token_header(token)
-            when :basic_auth
-              token_basic_auth(token)
-            end
-          end
-
-          it_behaves_like 'returning response status', params[:status]
-        end
-      end
-    end
-
-    context 'with invalid params' do
-      let(:valid_registry_id) { registry.id }
-
-      before_all do
-        group.add_maintainer(user)
-      end
-
-      where(:registry_id, :cache_validity_hours, :status) do
-        non_existing_record_id  | 1   | :not_found
-        'foo'                   | 1   | :bad_request
-        ''                      | 1   | :not_found
-        ref(:valid_registry_id) | 'a' | :bad_request
-        ref(:valid_registry_id) | ''  | :bad_request
-        ref(:valid_registry_id) | -1  | :bad_request
-        ref(:valid_registry_id) | nil | :bad_request
-      end
-
-      with_them do
-        let(:params) { { cache_validity_hours: cache_validity_hours } }
-
-        it_behaves_like 'returning response status', params[:status]
-      end
-    end
-  end
-
   describe 'DELETE /api/v4/virtual_registries/packages/maven/registries/:id' do
     let(:registry_id) { registry.id }
     let(:url) { "/virtual_registries/packages/maven/registries/#{registry_id}" }
@@ -654,6 +538,10 @@
       it 'returns a successful response' do
         expect { api_request }.to change { ::VirtualRegistries::Packages::Maven::Upstream.count }.by(1)
           .and change { ::VirtualRegistries::Packages::Maven::RegistryUpstream.count }.by(1)
+
+        expect(::VirtualRegistries::Packages::Maven::Upstream.last.cache_validity_hours).to eq(
+          params[:cache_validity_hours] || ::VirtualRegistries::Packages::Maven::Upstream.new.cache_validity_hours
+        )
       end
     end
 
@@ -700,10 +588,11 @@
 
     context 'for params' do
       where(:params, :status) do
-        { url: 'http://example.com', username: 'test', password: 'test' } | :created
-        { url: '', username: 'test', password: 'test' }                   | :bad_request
-        { url: 'http://example.com', username: 'test' }                   | :bad_request
-        {}                                                                | :bad_request
+        { url: 'http://example.com', username: 'test', password: 'test', cache_validity_hours: 3 } | :created
+        { url: 'http://example.com', username: 'test', password: 'test' }                          | :created
+        { url: '', username: 'test', password: 'test' }                                            | :bad_request
+        { url: 'http://example.com', username: 'test' }                                            | :bad_request
+        {}                                                                                         | :bad_request
       end
 
       before do
@@ -900,19 +789,25 @@
         group.add_maintainer(user)
       end
 
-      where(:param_url, :username, :password, :status) do
-        nil                  | 'test' | 'test' | :ok
-        'http://example.com' | nil    | 'test' | :ok
-        'http://example.com' | 'test' | nil    | :ok
-        ''                   | 'test' | 'test' | :bad_request
-        'http://example.com' | ''     | 'test' | :bad_request
-        'http://example.com' | 'test' | ''     | :bad_request
-        nil                  | nil    | nil    | :bad_request
+      let(:params) do
+        { url: param_url, username: username, password: password, cache_validity_hours: cache_validity_hours }.compact
       end
 
-      with_them do
-        let(:params) { { url: param_url, username: username, password: password }.compact }
+      where(:param_url, :username, :password, :cache_validity_hours, :status) do
+        nil                  | 'test' | 'test' | 3   | :ok
+        'http://example.com' | nil    | 'test' | 3   | :ok
+        'http://example.com' | 'test' | nil    | 3   | :ok
+        'http://example.com' | 'test' | 'test' | nil | :ok
+        nil                  | nil    | nil    | 3   | :ok
+        'http://example.com' | 'test' | 'test' | 3   | :ok
+        ''                   | 'test' | 'test' | 3   | :bad_request
+        'http://example.com' | ''     | 'test' | 3   | :bad_request
+        'http://example.com' | 'test' | ''     | 3   | :bad_request
+        'http://example.com' | 'test' | 'test' | -1  | :bad_request
+        nil                  | nil    | nil    | nil | :bad_request
+      end
 
+      with_them do
         it_behaves_like 'returning response status', params[:status]
       end
     end