Merge pull request #1574 from Freika/chore/n-minus-ones

Eliminate some n-plus-ones.

.gitignore

@@ -65,7 +65,7 @@
 .dotnet/
 .cursorrules
 .cursormemory.md
-.serena/project.yml
+.serena/**/*
 
 /config/credentials/production.key
 /config/credentials/production.yml.enc

CHANGELOG.md

@@ -4,6 +4,15 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/)
 and this project adheres to [Semantic Versioning](http://semver.org/).
 
+# [0.30.6] - 2025-07-27
+
+## Changed
+
+- Put all jobs in their own queues.
+- Visits page should load faster now.
+- Reverse geocoding jobs now make less database queries.
+
+
 # [0.30.5] - 2025-07-26
 
 ## Fixed

app/controllers/stats_controller.rb

@@ -57,8 +57,15 @@ class StatsController < ApplicationController
   def precompute_year_distances
     year_distances = {}
 
-    @stats.each do |year, _stats|
-      year_distances[year] = Stat.year_distance(year, current_user)
+    @stats.each do |year, stats|
+      stats_by_month = stats.index_by(&:month)
+      
+      year_distances[year] = (1..12).map do |month|
+        month_name = Date::MONTHNAMES[month]
+        distance = stats_by_month[month]&.distance || 0
+        
+        [month_name, distance]
+      end
     end
 
     year_distances

app/controllers/visits_controller.rb

@@ -11,7 +11,7 @@ class VisitsController < ApplicationController
     visits = current_user
              .visits
              .where(status:)
-             .includes(%i[suggested_places area points])
+             .includes(%i[suggested_places area points place])
              .order(started_at: order_by)
 
     @suggested_visits_count = current_user.visits.suggested.count

app/javascript/application.js

@@ -1,5 +1,6 @@
 // Configure your import map in config/importmap.rb. Read more: https://github.com/rails/importmap-rails
 
+import "@rails/ujs"
 import "@rails/actioncable"
 import "controllers"
 import "@hotwired/turbo-rails"
@@ -13,5 +14,4 @@ import "./channels"
 import "trix"
 import "@rails/actiontext"
 
-import "@rails/ujs"
 Rails.start()

app/jobs/app_version_checking_job.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 class AppVersionCheckingJob < ApplicationJob
-  queue_as :default
+  queue_as :app_version_checking
   sidekiq_options retry: false
 
   def perform

app/jobs/cache/cleaning_job.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 class Cache::CleaningJob < ApplicationJob
-  queue_as :default
+  queue_as :cache
 
   def perform
     Cache::Clean.call

app/jobs/cache/preheating_job.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 class Cache::PreheatingJob < ApplicationJob
-  queue_as :default
+  queue_as :cache
 
   def perform
     User.find_each do |user|

app/jobs/data_migrations/migrate_places_lonlat_job.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 class DataMigrations::MigratePlacesLonlatJob < ApplicationJob
-  queue_as :default
+  queue_as :data_migrations
 
   def perform(user_id)
     user = User.find(user_id)

app/jobs/data_migrations/migrate_points_latlon_job.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 class DataMigrations::MigratePointsLatlonJob < ApplicationJob
-  queue_as :default
+  queue_as :data_migrations
 
   def perform(user_id)
     user = User.find(user_id)

app/jobs/data_migrations/set_points_country_ids_job.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 class DataMigrations::SetPointsCountryIdsJob < ApplicationJob
-  queue_as :default
+  queue_as :data_migrations
 
   def perform(point_id)
     point = Point.find(point_id)

app/jobs/data_migrations/set_reverse_geocoded_at_for_points_job.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 class DataMigrations::SetReverseGeocodedAtForPointsJob < ApplicationJob
-  queue_as :default
+  queue_as :data_migrations
 
   def perform
     timestamp = Time.current

app/jobs/data_migrations/start_settings_points_country_ids_job.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 class DataMigrations::StartSettingsPointsCountryIdsJob < ApplicationJob
-  queue_as :default
+  queue_as :data_migrations
 
   def perform
     Point.where(country_id: nil).find_each do |point|

app/services/reverse_geocoding/places/fetch_data.rb

@@ -19,18 +19,15 @@ class ReverseGeocoding::Places::FetchData
     first_place = places.shift
     update_place(first_place)
 
-    osm_ids = places.map { |place| place.data['properties']['osm_id'].to_s }
+    osm_ids = extract_osm_ids(places)
 
     return if osm_ids.empty?
 
-    existing_places =
-      Place.where("geodata->'properties'->>'osm_id' IN (?)", osm_ids)
-        .index_by { |p| p.geodata.dig('properties', 'osm_id').to_s }
-        .compact
+    existing_places = find_existing_places(osm_ids)
 
-    places.each do |reverse_geocoded_place|
-      fetch_and_create_place(reverse_geocoded_place, existing_places)
-    end
+    places_to_create, places_to_update = prepare_places_for_bulk_operations(places, existing_places)
+
+    save_places(places_to_create, places_to_update)
   end
 
   private
@@ -42,7 +39,7 @@ class ReverseGeocoding::Places::FetchData
 
     place.update!(
       name:       place_name(data),
-      lonlat:     "POINT(#{data['geometry']['coordinates'][0]} #{data['geometry']['coordinates'][1]})",
+      lonlat:     build_point_coordinates(data['geometry']['coordinates']),
       city:       data['properties']['city'],
       country:    data['properties']['country'],
       geodata:    data,
@@ -51,33 +48,20 @@ class ReverseGeocoding::Places::FetchData
     )
   end
 
-  def fetch_and_create_place(reverse_geocoded_place, existing_places)
-    data = reverse_geocoded_place.data
-    new_place = find_place(data, existing_places)
-
-    new_place.name = place_name(data)
-    new_place.city = data['properties']['city']
-    new_place.country = data['properties']['country'] # TODO: Use country id
-    new_place.geodata = data
-    new_place.source = :photon
-    if new_place.lonlat.blank?
-      new_place.lonlat = "POINT(#{data['geometry']['coordinates'][0]} #{data['geometry']['coordinates'][1]})"
-    end
-
-    new_place.save!
-  end
 
   def find_place(place_data, existing_places)
     osm_id = place_data['properties']['osm_id'].to_s
 
     existing_place = existing_places[osm_id]
+
     return existing_place if existing_place.present?
 
-    # If not found in existing places, initialize a new one
+    coordinates = place_data['geometry']['coordinates']
+
     Place.new(
-      lonlat: "POINT(#{place_data['geometry']['coordinates'][0].to_f.round(5)} #{place_data['geometry']['coordinates'][1].to_f.round(5)})",
-      latitude: place_data['geometry']['coordinates'][1].to_f.round(5),
-      longitude: place_data['geometry']['coordinates'][0].to_f.round(5)
+      lonlat: build_point_coordinates(coordinates),
+      latitude: coordinates[1].to_f.round(5),
+      longitude: coordinates[0].to_f.round(5)
     )
   end
 
@@ -92,6 +76,75 @@ class ReverseGeocoding::Places::FetchData
     "#{name} (#{type})"
   end
 
+  def extract_osm_ids(places)
+    places.map { |place| place.data['properties']['osm_id'].to_s }
+  end
+
+  def find_existing_places(osm_ids)
+    Place.where("geodata->'properties'->>'osm_id' IN (?)", osm_ids)
+      .index_by { |p| p.geodata.dig('properties', 'osm_id').to_s }
+      .compact
+  end
+
+  def prepare_places_for_bulk_operations(places, existing_places)
+    places_to_create = []
+    places_to_update = []
+
+    places.each do |reverse_geocoded_place|
+      data = reverse_geocoded_place.data
+      new_place = find_place(data, existing_places)
+
+      populate_place_attributes(new_place, data)
+
+      if new_place.persisted?
+        places_to_update << new_place
+      else
+        places_to_create << new_place
+      end
+    end
+
+    [places_to_create, places_to_update]
+  end
+
+  def populate_place_attributes(place, data)
+    place.name = place_name(data)
+    place.city = data['properties']['city']
+    place.country = data['properties']['country']
+    place.geodata = data
+    place.source = :photon
+
+    if place.lonlat.blank?
+      place.lonlat = build_point_coordinates(data['geometry']['coordinates'])
+    end
+  end
+
+  def save_places(places_to_create, places_to_update)
+    if places_to_create.any?
+      place_attributes = places_to_create.map do |place|
+        {
+          name: place.name,
+          latitude: place.latitude,
+          longitude: place.longitude,
+          lonlat: place.lonlat,
+          city: place.city,
+          country: place.country,
+          geodata: place.geodata,
+          source: place.source,
+          created_at: Time.current,
+          updated_at: Time.current
+        }
+      end
+      Place.insert_all(place_attributes)
+    end
+
+    # Individual updates for existing places
+    places_to_update.each(&:save!) if places_to_update.any?
+  end
+
+  def build_point_coordinates(coordinates)
+    "POINT(#{coordinates[0]} #{coordinates[1]})"
+  end
+
   def reverse_geocoded_places
     data = Geocoder.search(
       [place.lat, place.lon],

app/views/stats/_year.html.erb

@@ -4,7 +4,9 @@
 </h2>
 <div class='my-10'>
   <%= column_chart(
-    @year_distances[year],
+    @year_distances[year].map { |month_name, distance_meters|
+      [month_name, Stat.convert_distance(distance_meters, current_user.safe_settings.distance_unit).round]
+    },
     height: '200px',
     suffix: " #{current_user.safe_settings.distance_unit}",
     xtitle: 'Days',

config/sidekiq.yml

@@ -1,6 +1,7 @@
 ---
 :concurrency: <%= ENV.fetch("BACKGROUND_PROCESSING_CONCURRENCY", 10) %>
 :queues:
+  - data_migrations
   - points
   - default
   - imports
@@ -11,3 +12,5 @@
   - reverse_geocoding
   - visit_suggesting
   - places
+  - app_version_checking
+  - cache

spec/jobs/data_migrations/migrate_places_lonlat_job_spec.rb

@@ -66,8 +66,8 @@ RSpec.describe DataMigrations::MigratePlacesLonlatJob, type: :job do
   end
 
   describe 'queue' do
-    it 'uses the default queue' do
-      expect(described_class.queue_name).to eq('default')
+    it 'uses the data_migrations queue' do
+      expect(described_class.queue_name).to eq('data_migrations')
     end
   end
 end

spec/jobs/data_migrations/set_points_country_ids_job_spec.rb

@@ -21,8 +21,8 @@ RSpec.describe DataMigrations::SetPointsCountryIdsJob, type: :job do
   end
 
   describe 'queue' do
-    it 'uses the default queue' do
-      expect(described_class.queue_name).to eq('default')
+    it 'uses the data_migrations queue' do
+      expect(described_class.queue_name).to eq('data_migrations')
     end
   end
 end

spec/jobs/data_migrations/start_settings_points_country_ids_job_spec.rb

@@ -26,8 +26,8 @@ RSpec.describe DataMigrations::StartSettingsPointsCountryIdsJob, type: :job do
   end
 
   describe 'queue' do
-    it 'uses the default queue' do
-      expect(described_class.queue_name).to eq('default')
+    it 'uses the data_migrations queue' do
+      expect(described_class.queue_name).to eq('data_migrations')
     end
   end
 end

spec/services/reverse_geocoding/places/fetch_data_spec.rb

@@ -3,6 +3,617 @@
 require 'rails_helper'
 
 RSpec.describe ReverseGeocoding::Places::FetchData do
+  subject(:service) { described_class.new(place.id) }
+
+  let(:place) { create(:place) }
+  let(:mock_geocoded_place) do
+    double(
+      data: {
+        'geometry' => {
+          'coordinates' => [13.0948638, 54.2905245]
+        },
+        'properties' => {
+          'osm_id' => 12345,
+          'name' => 'Test Place',
+          'osm_value' => 'restaurant',
+          'city' => 'Berlin',
+          'country' => 'Germany',
+          'postcode' => '10115',
+          'street' => 'Test Street',
+          'housenumber' => '1'
+        }
+      }
+    )
+  end
+
   describe '#call' do
+    context 'when reverse geocoding is enabled' do
+      before do
+        allow(DawarichSettings).to receive(:reverse_geocoding_enabled?).and_return(true)
+        allow(Geocoder).to receive(:search).and_return([mock_geocoded_place])
+      end
+
+      it 'fetches geocoded places' do
+        service.call
+
+        expect(Geocoder).to have_received(:search).with(
+          [place.lat, place.lon],
+          limit: 10,
+          distance_sort: true,
+          radius: 1,
+          units: :km
+        )
+      end
+
+      it 'updates the original place with geocoded data' do
+        expect { service.call }.to change { place.reload.name }
+          .and change { place.reload.city }.to('Berlin')
+          .and change { place.reload.country }.to('Germany')
+      end
+
+      it 'sets reverse_geocoded_at timestamp' do
+        expect { service.call }.to change { place.reload.reverse_geocoded_at }
+          .from(nil)
+
+        expect(place.reload.reverse_geocoded_at).to be_present
+      end
+
+      it 'sets the source to photon' do
+        expect { service.call }.to change { place.reload.source }
+          .to('photon')
+      end
+
+      context 'with multiple geocoded places' do
+        let(:second_mock_place) do
+          double(
+            data: {
+              'geometry' => {
+                'coordinates' => [13.1, 54.3]
+              },
+              'properties' => {
+                'osm_id' => 67890,
+                'name' => 'Second Place',
+                'osm_value' => 'cafe',
+                'city' => 'Hamburg',
+                'country' => 'Germany'
+              }
+            }
+          )
+        end
+
+        before do
+          allow(Geocoder).to receive(:search).and_return([mock_geocoded_place, second_mock_place])
+        end
+
+        it 'creates new places for additional geocoded results' do
+          # Force place creation before counting
+          place # This triggers the let(:place) lazy loading
+          initial_count = Place.count
+          service.call
+          final_count = Place.count
+
+          expect(final_count - initial_count).to eq(1)
+        end
+
+        it 'updates the original place and creates others' do
+          service.call
+
+          created_place = Place.where.not(id: place.id).first
+          expect(created_place.name).to include('Second Place')
+          expect(created_place.city).to eq('Hamburg')
+        end
+      end
+
+      context 'with existing places in database' do
+        let!(:existing_place) { create(:place, :with_geodata) }
+
+        before do
+          # Mock geocoded place with same OSM ID as existing place
+          existing_osm_id = existing_place.geodata.dig('properties', 'osm_id')
+          mock_with_existing_osm = double(
+            data: {
+              'geometry' => { 'coordinates' => [13.0948638, 54.2905245] },
+              'properties' => {
+                'osm_id' => existing_osm_id,
+                'name' => 'Updated Name',
+                'osm_value' => 'restaurant',
+                'city' => 'Updated City',
+                'country' => 'Updated Country'
+              }
+            }
+          )
+
+          allow(Geocoder).to receive(:search).and_return([mock_geocoded_place, mock_with_existing_osm])
+        end
+
+        it 'updates existing places instead of creating duplicates' do
+          place # Force place creation
+          expect { service.call }.not_to change { Place.count }
+        end
+
+        it 'updates the existing place attributes' do
+          service.call
+
+          existing_place.reload
+          expect(existing_place.name).to include('Updated Name')
+          expect(existing_place.city).to eq('Updated City')
+        end
+      end
+
+      context 'when first geocoded place is nil' do
+        before do
+          allow(Geocoder).to receive(:search).and_return([nil, mock_geocoded_place])
+        end
+
+        it 'does not update the original place' do
+          place # Force place creation
+          expect { service.call }.not_to change { place.reload.updated_at }
+        end
+
+        it 'still processes other places' do
+          place # Force place creation
+          expect { service.call }.to change { Place.count }.by(1)
+        end
+      end
+
+      context 'when no additional places are returned' do
+        before do
+          allow(Geocoder).to receive(:search).and_return([mock_geocoded_place])
+        end
+
+        it 'only updates the original place' do
+          place # Force place creation
+          expect { service.call }.not_to change { Place.count }
+        end
+
+        it 'returns early when osm_ids is empty' do
+          # This tests the early return when osm_ids.empty?
+          service.call
+
+          expect(Geocoder).to have_received(:search).once
+        end
+      end
+    end
+
+    context 'when reverse geocoding is disabled' do
+      before do
+        allow(DawarichSettings).to receive(:reverse_geocoding_enabled?).and_return(false)
+        allow(Rails.logger).to receive(:warn)
+      end
+
+      it 'logs a warning and returns early' do
+        service.call
+
+        expect(Rails.logger).to have_received(:warn).with('Reverse geocoding is not enabled')
+      end
+
+      it 'does not call Geocoder' do
+        allow(Geocoder).to receive(:search)
+        service.call
+
+        expect(Geocoder).not_to have_received(:search)
+      end
+
+      it 'does not update the place' do
+        expect { service.call }.not_to change { place.reload.updated_at }
+      end
+    end
+  end
+
+  describe 'private methods' do
+    before do
+      allow(DawarichSettings).to receive(:reverse_geocoding_enabled?).and_return(true)
+    end
+
+    describe '#place_name' do
+      it 'builds place name from properties' do
+        data = {
+          'properties' => {
+            'name' => 'Test Restaurant',
+            'osm_value' => 'restaurant',
+            'postcode' => '10115',
+            'street' => 'Main Street',
+            'housenumber' => '42'
+          }
+        }
+
+        result = service.send(:place_name, data)
+        expect(result).to eq('Test Restaurant (Restaurant)')
+      end
+
+      it 'uses address when name is missing' do
+        data = {
+          'properties' => {
+            'osm_value' => 'cafe',
+            'postcode' => '10115',
+            'street' => 'Oak Street',
+            'housenumber' => '123'
+          }
+        }
+
+        result = service.send(:place_name, data)
+        expect(result).to eq('10115 Oak Street 123 (Cafe)')
+      end
+
+      it 'handles missing housenumber' do
+        data = {
+          'properties' => {
+            'name' => 'Test Place',
+            'osm_value' => 'shop',
+            'postcode' => '10115',
+            'street' => 'Pine Street'
+          }
+        }
+
+        result = service.send(:place_name, data)
+        expect(result).to eq('Test Place (Shop)')
+      end
+
+      it 'formats osm_value correctly' do
+        data = {
+          'properties' => {
+            'name' => 'Test',
+            'osm_value' => 'fast_food_restaurant'
+          }
+        }
+
+        result = service.send(:place_name, data)
+        expect(result).to eq('Test (Fast food restaurant)')
+      end
+    end
+
+    describe '#extract_osm_ids' do
+      it 'extracts OSM IDs from places' do
+        places = [
+          double(data: { 'properties' => { 'osm_id' => 123 } }),
+          double(data: { 'properties' => { 'osm_id' => 456 } })
+        ]
+
+        result = service.send(:extract_osm_ids, places)
+        expect(result).to eq(['123', '456'])
+      end
+    end
+
+    describe '#build_point_coordinates' do
+      it 'builds POINT geometry string' do
+        coordinates = [13.0948638, 54.2905245]
+        result = service.send(:build_point_coordinates, coordinates)
+        expect(result).to eq('POINT(13.0948638 54.2905245)')
+      end
+    end
+
+    describe '#find_existing_places' do
+      let!(:existing_place1) { create(:place, :with_geodata) }
+      let!(:existing_place2) do
+        create(:place, geodata: {
+          'properties' => { 'osm_id' => 999 }
+        })
+      end
+
+      it 'finds existing places by OSM IDs' do
+        osm_id1 = existing_place1.geodata.dig('properties', 'osm_id').to_s
+        osm_ids = [osm_id1, '999']
+
+        result = service.send(:find_existing_places, osm_ids)
+
+        expect(result.keys).to contain_exactly(osm_id1, '999')
+        expect(result[osm_id1]).to eq(existing_place1)
+        expect(result['999']).to eq(existing_place2)
+      end
+
+      it 'returns empty hash when no matches found' do
+        result = service.send(:find_existing_places, ['nonexistent'])
+        expect(result).to eq({})
+      end
+    end
+
+    describe '#find_place' do
+      let(:existing_places) { { '123' => create(:place) } }
+      let(:place_data) do
+        {
+          'properties' => { 'osm_id' => 123 },
+          'geometry' => { 'coordinates' => [13.1, 54.3] }
+        }
+      end
+
+      context 'when place exists' do
+        it 'returns existing place' do
+          result = service.send(:find_place, place_data, existing_places)
+          expect(result).to eq(existing_places['123'])
+        end
+      end
+
+      context 'when place does not exist' do
+        let(:place_data) do
+          {
+            'properties' => { 'osm_id' => 999 },
+            'geometry' => { 'coordinates' => [13.1, 54.3] }
+          }
+        end
+
+        it 'creates new place with coordinates' do
+          result = service.send(:find_place, place_data, existing_places)
+
+          expect(result).to be_a(Place)
+          expect(result.latitude).to eq(54.3)
+          expect(result.longitude).to eq(13.1)
+          expect(result.lonlat.to_s).to eq('POINT (13.1 54.3)')
+        end
+      end
+    end
+
+    describe '#populate_place_attributes' do
+      let(:test_place) { Place.new }
+      let(:data) do
+        {
+          'properties' => {
+            'name' => 'Test Place',
+            'osm_value' => 'restaurant',
+            'city' => 'Berlin',
+            'country' => 'Germany'
+          },
+          'geometry' => { 'coordinates' => [13.1, 54.3] }
+        }
+      end
+
+      it 'populates all place attributes' do
+        place # Ensure place exists
+        service.send(:populate_place_attributes, test_place, data)
+
+        expect(test_place.name).to include('Test Place')
+        expect(test_place.city).to eq('Berlin')
+        expect(test_place.country).to eq('Germany')
+        expect(test_place.geodata).to eq(data)
+        expect(test_place.source).to eq('photon')
+      end
+
+      it 'sets lonlat when nil' do
+        place # Ensure place exists
+        service.send(:populate_place_attributes, test_place, data)
+        expect(test_place.lonlat.to_s).to eq('POINT (13.1 54.3)')
+      end
+
+      it 'does not override existing lonlat' do
+        place # Ensure place exists
+        test_place.lonlat = 'POINT(10.0 50.0)'
+        service.send(:populate_place_attributes, test_place, data)
+        expect(test_place.lonlat.to_s).to eq('POINT (10.0 50.0)')
+      end
+    end
+
+    describe '#prepare_places_for_bulk_operations' do
+      let(:new_place_data) do
+        double(
+          data: {
+            'properties' => { 'osm_id' => 999 },
+            'geometry' => { 'coordinates' => [13.1, 54.3] }
+          }
+        )
+      end
+      let(:existing_place) { create(:place, :with_geodata) }
+      let(:existing_place_data) do
+        double(
+          data: {
+            'properties' => { 'osm_id' => existing_place.geodata.dig('properties', 'osm_id') },
+            'geometry' => { 'coordinates' => [13.2, 54.4] }
+          }
+        )
+      end
+
+      it 'separates places into create and update arrays' do
+        existing_places = { existing_place.geodata.dig('properties', 'osm_id').to_s => existing_place }
+        places = [new_place_data, existing_place_data]
+
+        places_to_create, places_to_update = service.send(:prepare_places_for_bulk_operations, places, existing_places)
+
+        expect(places_to_create.length).to eq(1)
+        expect(places_to_update.length).to eq(1)
+        expect(places_to_update.first).to eq(existing_place)
+        expect(places_to_create.first).to be_a(Place)
+        expect(places_to_create.first.persisted?).to be(false)
+      end
+    end
+
+    describe '#save_places' do
+      it 'saves new places when places_to_create is present' do
+        place # Ensure place exists
+        new_place = build(:place)
+        places_to_create = [new_place]
+        places_to_update = []
+
+        expect { service.send(:save_places, places_to_create, places_to_update) }
+          .to change { Place.count }.by(1)
+      end
+
+      it 'saves updated places when places_to_update is present' do
+        existing_place = create(:place, name: 'Old Name')
+        existing_place.name = 'New Name'
+        places_to_create = []
+        places_to_update = [existing_place]
+
+        service.send(:save_places, places_to_create, places_to_update)
+
+        expect(existing_place.reload.name).to eq('New Name')
+      end
+
+      it 'handles empty arrays gracefully' do
+        expect { service.send(:save_places, [], []) }.not_to raise_error
+      end
+    end
+  end
+
+  describe 'edge cases and error scenarios' do
+    before do
+      allow(DawarichSettings).to receive(:reverse_geocoding_enabled?).and_return(true)
+    end
+
+    context 'when Geocoder returns empty results' do
+      before do
+        allow(Geocoder).to receive(:search).and_return([])
+      end
+
+      it 'handles empty geocoder results gracefully' do
+        expect { service.call }.not_to raise_error
+      end
+
+      it 'does not update the place' do
+        expect { service.call }.not_to change { place.reload.updated_at }
+      end
+    end
+
+    context 'when Geocoder raises an exception' do
+      before do
+        allow(Geocoder).to receive(:search).and_raise(StandardError.new('Geocoding failed'))
+      end
+
+      it 'allows the exception to bubble up' do
+        expect { service.call }.to raise_error(StandardError, 'Geocoding failed')
+      end
+    end
+
+    context 'when place data is malformed' do
+      let(:malformed_place) do
+        double(
+          data: {
+            'geometry' => {
+              'coordinates' => ['invalid', 'coordinates']
+            },
+            'properties' => {
+              'osm_id' => nil
+            }
+          }
+        )
+      end
+
+      before do
+        allow(Geocoder).to receive(:search).and_return([mock_geocoded_place, malformed_place])
+      end
+
+      it 'handles malformed data gracefully' do
+        # With bulk operations using insert_all, validation errors are bypassed
+        # Malformed data will be inserted but may cause issues at the database level
+        place # Force place creation
+        expect { service.call }.not_to raise_error
+      end
+    end
+
+    context 'when using bulk operations' do
+      let(:second_geocoded_place) do
+        double(
+          data: {
+            'geometry' => { 'coordinates' => [14.0, 55.0] },
+            'properties' => {
+              'osm_id' => 99999,
+              'name' => 'Another Place',
+              'osm_value' => 'shop'
+            }
+          }
+        )
+      end
+
+      it 'uses bulk operations for performance' do
+        place # Force place creation first
+
+        allow(Geocoder).to receive(:search).and_return([mock_geocoded_place, second_geocoded_place])
+        # With insert_all, we expect the operation to succeed even with potential validation issues
+        # since bulk operations bypass ActiveRecord validations for performance
+
+        expect { service.call }.to change { Place.count }.by(1)
+      end
+    end
+
+    context 'when database constraint violations occur' do
+      let(:duplicate_place) { create(:place, :with_geodata) }
+      let(:duplicate_data) do
+        double(
+          data: {
+            'geometry' => { 'coordinates' => [13.1, 54.3] },
+            'properties' => {
+              'osm_id' => duplicate_place.geodata.dig('properties', 'osm_id'),
+              'name' => 'Duplicate'
+            }
+          }
+        )
+      end
+
+      before do
+        allow(Geocoder).to receive(:search).and_return([mock_geocoded_place, duplicate_data])
+        # Simulate the place not being found in existing_places due to race condition
+        allow(service).to receive(:find_existing_places).and_return({})
+      end
+
+      it 'handles potential race conditions gracefully' do
+        # The service should handle cases where a place might be created
+        # between the existence check and the actual creation
+        expect { service.call }.not_to raise_error
+      end
+    end
+
+    context 'when place_id does not exist' do
+      subject(:service) { described_class.new(999999) }
+
+      it 'raises ActiveRecord::RecordNotFound' do
+        expect { service }.to raise_error(ActiveRecord::RecordNotFound)
+      end
+    end
+
+    context 'with missing properties in geocoded data' do
+      let(:minimal_place) do
+        double(
+          data: {
+            'geometry' => {
+              'coordinates' => [13.0, 54.0]
+            },
+            'properties' => {
+              'osm_id' => 99999
+              # Missing name, city, country, etc.
+            }
+          }
+        )
+      end
+
+      before do
+        allow(Geocoder).to receive(:search).and_return([mock_geocoded_place, minimal_place])
+      end
+
+      it 'handles missing properties gracefully' do
+        expect { service.call }.not_to raise_error
+      end
+
+      it 'creates place with available data' do
+        place # Force place creation
+        expect { service.call }.to change { Place.count }.by(1)
+
+        created_place = Place.where.not(id: place.id).first
+        expect(created_place.latitude).to eq(54.0)
+        expect(created_place.longitude).to eq(13.0)
+      end
+    end
+
+    context 'when lonlat is already present on existing place' do
+      let!(:existing_place) { create(:place, :with_geodata, lonlat: 'POINT(10.0 50.0)') }
+      let(:existing_data) do
+        double(
+          data: {
+            'geometry' => { 'coordinates' => [15.0, 55.0] },
+            'properties' => {
+              'osm_id' => existing_place.geodata.dig('properties', 'osm_id'),
+              'name' => 'Updated Name'
+            }
+          }
+        )
+      end
+
+      before do
+        allow(Geocoder).to receive(:search).and_return([mock_geocoded_place, existing_data])
+      end
+
+      it 'does not override existing lonlat' do
+        service.call
+
+        existing_place.reload
+        expect(existing_place.lonlat.to_s).to eq('POINT (10.0 50.0)')
+      end
+    end
   end
 end