Merge branch '9903-geo-selective-sync-by-namespace-is-broken' into 'master'

Resolves "Geo - Selective sync by namespace is broken"

Closes #10041 and #9903

See merge request gitlab-org/gitlab-ee!9732

ee/changelogs/unreleased/9903-geo-selective-sync-by-namespace-is-broken.yml

+---
+title: Geo - Fix selective sync by namespace
+merge_request: 9732
+author:
+type: fixed

ee/spec/workers/geo/file_download_dispatch_worker_spec.rb

@@ -2,14 +2,14 @@ require 'spec_helper'
 
 describe Geo::FileDownloadDispatchWorker, :geo do
   include ::EE::GeoHelpers
+  include ExclusiveLeaseHelpers
 
   let(:primary)   { create(:geo_node, :primary, host: 'primary-geo-node') }
   let(:secondary) { create(:geo_node) }
 
   before do
     stub_current_geo_node(secondary)
-    allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(true)
-    allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:renew).and_return(true)
+    stub_exclusive_lease(renew: true)
     allow_any_instance_of(described_class).to receive(:over_time?).and_return(false)
     WebMock.stub_request(:get, /primary-geo-node/).to_return(status: 200, body: "", headers: {})
   end
@@ -347,15 +347,18 @@ describe Geo::FileDownloadDispatchWorker, :geo do
       end
     end
 
-    context 'when node has namespace restrictions' do
+    context 'when node has namespace restrictions', :request_store do
       let(:synced_group) { create(:group) }
       let(:project_in_synced_group) { create(:project, group: synced_group) }
       let(:unsynced_project) { create(:project) }
 
       before do
-        allow(ProjectCacheWorker).to receive(:perform_async).and_return(true)
-
         secondary.update!(selective_sync_type: 'namespaces', namespaces: [synced_group])
+
+        allow(ProjectCacheWorker).to receive(:perform_async).and_return(true)
+        allow(::Gitlab::Geo).to receive(:current_node).and_call_original
+        Rails.cache.write(:current_node, secondary.to_json)
+        allow(::GeoNode).to receive(:current_node).and_return(secondary)
       end
 
       it 'does not perform Geo::FileDownloadWorker for LFS object that does not belong to selected namespaces to replicate' do

ee/spec/workers/geo/repository_shard_sync_worker_spec.rb

@@ -115,9 +115,13 @@ describe Geo::RepositoryShardSyncWorker, :geo, :delete, :clean_gitlab_redis_cach
       end
     end
 
-    context 'when node has namespace restrictions' do
+    context 'when node has namespace restrictions', :request_store do
       before do
         secondary.update!(selective_sync_type: 'namespaces', namespaces: [restricted_group])
+
+        allow(::Gitlab::Geo).to receive(:current_node).and_call_original
+        Rails.cache.write(:current_node, secondary.to_json)
+        allow(::GeoNode).to receive(:current_node).and_return(secondary)
       end
 
       it 'does not perform Geo::ProjectSyncWorker for projects that do not belong to selected namespaces to replicate' do

lib/gitlab/json_cache.rb

@@ -71,7 +71,21 @@ module Gitlab
     end
 
     def parse_entry(raw, klass)
-      klass.new(raw) if valid_entry?(raw, klass)
+      return unless valid_entry?(raw, klass)
+      return klass.new(raw) unless klass.ancestors.include?(ActiveRecord::Base)
+
+      # When the cached value is a persisted instance of ActiveRecord::Base in
+      # some cases a relation can return an empty collection becauses scope.none!
+      # is being applied on ActiveRecord::Associations::CollectionAssociation#scope
+      # when the new_record? method incorrectly returns false.
+      #
+      # See https://gitlab.com/gitlab-org/gitlab-ee/issues/9903#note_145329964
+      attributes = klass.attributes_builder.build_from_database(raw, {})
+      klass.allocate.init_with("attributes" => attributes, "new_record" => new_record?(raw, klass))
+    end
+
+    def new_record?(raw, klass)
+      raw.fetch(klass.primary_key, nil).blank?
     end
 
     def valid_entry?(raw, klass)

spec/lib/gitlab/json_cache_spec.rb

@@ -297,13 +297,39 @@ describe Gitlab::JsonCache do
           expect(result).to eq(broadcast_message)
         end
 
+        context 'when the cached value is an instance of ActiveRecord::Base' do
+          it 'returns a persisted record when id is set' do
+            backend.write(expanded_key, broadcast_message.to_json)
+
+            result = cache.fetch(key, as: BroadcastMessage) { 'block result' }
+
+            expect(result).to be_persisted
+          end
+
+          it 'returns a new record when id is nil' do
+            backend.write(expanded_key, build(:broadcast_message).to_json)
+
+            result = cache.fetch(key, as: BroadcastMessage) { 'block result' }
+
+            expect(result).to be_new_record
+          end
+
+          it 'returns a new record when id is missing' do
+            backend.write(expanded_key, build(:broadcast_message).attributes.except('id').to_json)
+
+            result = cache.fetch(key, as: BroadcastMessage) { 'block result' }
+
+            expect(result).to be_new_record
+          end
+        end
+
         it "returns the result of the block when 'as' option is nil" do
           result = cache.fetch(key, as: nil) { 'block result' }
 
           expect(result).to eq('block result')
         end
 
-        it "returns the result of the block when 'as' option is not informed" do
+        it "returns the result of the block when 'as' option is missing" do
           result = cache.fetch(key) { 'block result' }
 
           expect(result).to eq('block result')