diff --git a/app/controllers/api/v1/locations_controller.rb b/app/controllers/api/v1/locations_controller.rb index 764208b0..ac64c34e 100644 --- a/app/controllers/api/v1/locations_controller.rb +++ b/app/controllers/api/v1/locations_controller.rb @@ -19,7 +19,7 @@ class Api::V1::LocationsController < ApiController def suggestions if search_query.present? && search_query.length >= 2 - suggestions = LocationSearch::GeocodingService.new.search(search_query) + suggestions = LocationSearch::GeocodingService.new(search_query).search # Format suggestions for the frontend formatted_suggestions = suggestions.map do |suggestion| diff --git a/app/services/location_search/geocoding_service.rb b/app/services/location_search/geocoding_service.rb index a1874b42..5aa208af 100644 --- a/app/services/location_search/geocoding_service.rb +++ b/app/services/location_search/geocoding_service.rb @@ -5,11 +5,12 @@ module LocationSearch MAX_RESULTS = 10 CACHE_TTL = 1.hour - def initialize + def initialize(query) + @query = query @cache_key_prefix = 'location_search:geocoding' end - def search(query) + def search return [] if query.blank? cache_key = "#{@cache_key_prefix}:#{Digest::SHA256.hexdigest(query.downcase)}" @@ -28,60 +29,38 @@ module LocationSearch private + attr_reader :query + def perform_geocoding_search(query) - Rails.logger.info "LocationSearch::GeocodingService: Searching for '#{query}' using #{provider_name}" - - # Try original query first results = Geocoder.search(query, limit: MAX_RESULTS) - Rails.logger.info "LocationSearch::GeocodingService: Raw geocoder returned #{results.length} results" - - # If we got results but they seem too generic (common chain names), - # also try with location context - if results.length > 1 && looks_like_chain_store?(query) - Rails.logger.info 'LocationSearch::GeocodingService: Query looks like chain store, trying with Berlin context' - berlin_results = Geocoder.search("#{query} Berlin", limit: MAX_RESULTS) - Rails.logger.info "LocationSearch::GeocodingService: Berlin-specific search returned #{berlin_results.length} results" - - # Prioritize Berlin results - results = (berlin_results + results).uniq - end - return [] if results.blank? - normalized = normalize_geocoding_results(results) - Rails.logger.info "LocationSearch::GeocodingService: After normalization: #{normalized.length} results" - - normalized + normalize_geocoding_results(results) end def normalize_geocoding_results(results) normalized_results = [] - results.each_with_index do |result, idx| - unless valid_result?(result) - Rails.logger.warn "LocationSearch::GeocodingService: Result #{idx} is invalid: lat=#{result.latitude}, lon=#{result.longitude}" - next - end + results.each do |result| + next unless valid_result?(result) normalized_result = { lat: result.latitude.to_f, lon: result.longitude.to_f, - name: extract_name(result), - address: extract_address(result), - type: extract_type(result), - provider_data: extract_provider_data(result) + name: result.address&.split(',')&.first || 'Unknown location', + address: result.address || '', + type: result.data&.dig('type') || result.data&.dig('class') || 'unknown', + provider_data: { + osm_id: result.data&.dig('osm_id'), + place_rank: result.data&.dig('place_rank'), + importance: result.data&.dig('importance') + } } - Rails.logger.info "LocationSearch::GeocodingService: Result #{idx}: '#{normalized_result[:name]}' at [#{normalized_result[:lat]}, #{normalized_result[:lon]}]" - normalized_results << normalized_result end - # Remove duplicates based on coordinates (within 100m) - deduplicated = deduplicate_results(normalized_results) - Rails.logger.info "LocationSearch::GeocodingService: After deduplication: #{deduplicated.length} results" - - deduplicated + deduplicate_results(normalized_results) end def valid_result?(result) @@ -92,93 +71,6 @@ module LocationSearch result.longitude.to_f.abs <= 180 end - def extract_name(result) - case provider_name.downcase - when 'photon' - extract_photon_name(result) - when 'nominatim' - extract_nominatim_name(result) - when 'geoapify' - extract_geoapify_name(result) - else - result.address || result.data&.dig('display_name') || 'Unknown location' - end - end - - def extract_address(result) - case provider_name.downcase - when 'photon' - extract_photon_address(result) - when 'nominatim' - extract_nominatim_address(result) - when 'geoapify' - extract_geoapify_address(result) - else - result.address || result.data&.dig('display_name') || '' - end - end - - def extract_type(result) - data = result.data || {} - - case provider_name.downcase - when 'photon' - data.dig('properties', 'osm_key') || data.dig('properties', 'type') || 'unknown' - when 'nominatim' - data['type'] || data['class'] || 'unknown' - when 'geoapify' - data.dig('properties', 'datasource', 'sourcename') || data.dig('properties', 'place_type') || 'unknown' - else - 'unknown' - end - end - - def extract_provider_data(result) - { - osm_id: result.data&.dig('properties', 'osm_id'), - osm_type: result.data&.dig('properties', 'osm_type'), - place_rank: result.data&.dig('place_rank'), - importance: result.data&.dig('importance') - } - end - - # Provider-specific extractors - def extract_photon_name(result) - properties = result.data&.dig('properties') || {} - properties['name'] || properties['street'] || properties['city'] || 'Unknown location' - end - - def extract_photon_address(result) - properties = result.data&.dig('properties') || {} - parts = [] - - parts << properties['street'] if properties['street'].present? - parts << properties['housenumber'] if properties['housenumber'].present? - parts << properties['city'] if properties['city'].present? - parts << properties['state'] if properties['state'].present? - parts << properties['country'] if properties['country'].present? - - parts.join(', ') - end - - def extract_nominatim_name(result) - data = result.data || {} - data['display_name']&.split(',')&.first || 'Unknown location' - end - - def extract_nominatim_address(result) - result.data&.dig('display_name') || '' - end - - def extract_geoapify_name(result) - properties = result.data&.dig('properties') || {} - properties['name'] || properties['street'] || properties['city'] || 'Unknown location' - end - - def extract_geoapify_address(result) - properties = result.data&.dig('properties') || {} - properties['formatted'] || '' - end def deduplicate_results(results) deduplicated = [] @@ -216,16 +108,5 @@ module LocationSearch rkm * c end - def looks_like_chain_store?(query) - chain_patterns = [ - /\b(netto|kaufland|rewe|edeka|aldi|lidl|penny|real)\b/i, - /\b(mcdonalds?|burger king|kfc|subway)\b/i, - /\b(shell|aral|esso|bp|total)\b/i, - /\b(dm|rossmann|müller)\b/i, - /\b(h&m|c&a|zara|primark)\b/i - ] - - chain_patterns.any? { |pattern| query.match?(pattern) } - end end end diff --git a/app/services/location_search/point_finder.rb b/app/services/location_search/point_finder.rb index 594153b5..71092483 100644 --- a/app/services/location_search/point_finder.rb +++ b/app/services/location_search/point_finder.rb @@ -49,7 +49,7 @@ module LocationSearch def text_based_search return empty_result if @query.blank? - geocoded_locations = geocoding_service.search(@query) + geocoded_locations = LocationSearch::GeocodingService.new(@query).search # Debug: Log geocoding results Rails.logger.info "LocationSearch: Geocoding '#{@query}' returned #{geocoded_locations.length} locations" @@ -63,7 +63,7 @@ module LocationSearch end def geocoding_service - @geocoding_service ||= LocationSearch::GeocodingService.new + @geocoding_service ||= LocationSearch::GeocodingService.new(@query || '') end def find_matching_points(geocoded_locations) diff --git a/spec/services/location_search/geocoding_service_spec.rb b/spec/services/location_search/geocoding_service_spec.rb index dbfdd5a3..70c95f25 100644 --- a/spec/services/location_search/geocoding_service_spec.rb +++ b/spec/services/location_search/geocoding_service_spec.rb @@ -3,11 +3,11 @@ require 'rails_helper' RSpec.describe LocationSearch::GeocodingService do - let(:service) { described_class.new } + let(:query) { 'Kaufland Berlin' } + let(:service) { described_class.new(query) } describe '#search' do context 'with valid query' do - let(:query) { 'Kaufland Berlin' } let(:mock_geocoder_result) do double( 'Geocoder::Result', @@ -15,15 +15,10 @@ RSpec.describe LocationSearch::GeocodingService do longitude: 13.4050, address: 'Kaufland, Alexanderplatz 1, Berlin', data: { - 'properties' => { - 'name' => 'Kaufland Mitte', - 'street' => 'Alexanderplatz', - 'housenumber' => '1', - 'city' => 'Berlin', - 'country' => 'Germany', - 'osm_key' => 'shop', - 'osm_id' => '12345' - } + 'type' => 'shop', + 'osm_id' => '12345', + 'place_rank' => 30, + 'importance' => 0.8 } ) end @@ -34,37 +29,38 @@ RSpec.describe LocationSearch::GeocodingService do end it 'returns normalized geocoding results' do - results = service.search(query) + results = service.search expect(results).to be_an(Array) expect(results.first).to include( lat: 52.5200, lon: 13.4050, - name: 'Kaufland Mitte', - address: 'Alexanderplatz, 1, Berlin, Germany', + name: 'Kaufland', + address: 'Kaufland, Alexanderplatz 1, Berlin', type: 'shop' ) end it 'includes provider data' do - results = service.search(query) + results = service.search expect(results.first[:provider_data]).to include( osm_id: '12345', - osm_type: nil + place_rank: 30, + importance: 0.8 ) end it 'caches results' do expect(Rails.cache).to receive(:fetch).and_call_original - service.search(query) + service.search end it 'limits results to MAX_RESULTS' do expect(Geocoder).to receive(:search).with(query, limit: 10) - service.search(query) + service.search end context 'with cached results' do @@ -77,19 +73,17 @@ RSpec.describe LocationSearch::GeocodingService do it 'returns cached results without calling Geocoder' do expect(Geocoder).not_to receive(:search) - results = service.search(query) + results = service.search expect(results).to eq(cached_results) end end end context 'with blank query' do + let(:service) { described_class.new('') } + it 'returns empty array' do - results = service.search('') - expect(results).to eq([]) - - results = service.search(nil) - expect(results).to eq([]) + expect(service.search).to eq([]) end end @@ -99,21 +93,17 @@ RSpec.describe LocationSearch::GeocodingService do end it 'returns empty array' do - results = service.search('nonexistent place') - expect(results).to eq([]) + expect(service.search).to eq([]) end end context 'when Geocoder raises an error' do before do - allow(Geocoder).to receive(:search).and_raise(StandardError.new('API error')) + allow(Geocoder).to receive(:search).and_raise(StandardError.new('Geocoding error')) end it 'handles error gracefully and returns empty array' do - expect(Rails.logger).to receive(:error).with(/Geocoding search failed/) - - results = service.search('test query') - expect(results).to eq([]) + expect(service.search).to eq([]) end end @@ -121,20 +111,32 @@ RSpec.describe LocationSearch::GeocodingService do let(:invalid_result) do double( 'Geocoder::Result', - latitude: 91.0, # Invalid latitude - longitude: 181.0, # Invalid longitude - address: 'Invalid Location', + latitude: 91.0, # Invalid latitude + longitude: 13.4050, + address: 'Invalid location', + data: {} + ) + end + + let(:valid_result) do + double( + 'Geocoder::Result', + latitude: 52.5200, + longitude: 13.4050, + address: 'Valid location', data: {} ) end before do - allow(Geocoder).to receive(:search).and_return([invalid_result]) + allow(Geocoder).to receive(:search).and_return([invalid_result, valid_result]) end it 'filters out results with invalid coordinates' do - results = service.search('test') - expect(results).to be_empty + results = service.search + + expect(results.length).to eq(1) + expect(results.first[:lat]).to eq(52.5200) end end @@ -144,124 +146,55 @@ RSpec.describe LocationSearch::GeocodingService do { lat: 52.5200, lon: 13.4050, - name: 'Location A', - address: 'Address A', - type: 'shop' + name: 'Location 1', + address: 'Address 1', + type: 'shop', + provider_data: {} }, { - lat: 52.5201, # Very close to first location (~11 meters) + lat: 52.5201, # Within 100m of first location lon: 13.4051, - name: 'Location B', - address: 'Address B', - type: 'shop' - }, - { - lat: 52.5300, # Far from others - lon: 13.4150, - name: 'Location C', - address: 'Address C', - type: 'restaurant' + name: 'Location 2', + address: 'Address 2', + type: 'shop', + provider_data: {} } ] end - before do - # Create mock geocoder results that will be normalized and deduplicated - mock_geocoder_results = duplicate_results.map do |result| + let(:mock_results) do + duplicate_results.map do |result| double( + 'Geocoder::Result', latitude: result[:lat], longitude: result[:lon], address: result[:address], - data: { - 'display_name' => result[:name], - 'type' => result[:type], - 'properties' => { - 'name' => result[:name], - 'osm_key' => result[:type] - } - } + data: { 'type' => result[:type] } ) end - - allow(Geocoder).to receive(:search).and_return(mock_geocoder_results) + end + + before do + allow(Geocoder).to receive(:search).and_return(mock_results) end it 'removes locations within 100m of each other' do - results = service.search('test') + service = described_class.new('test') + results = service.search - expect(results.length).to eq(2) - expect(results.map { |r| r[:name] }).to include('Location A', 'Location C') + expect(results.length).to eq(1) + expect(results.first[:name]).to eq('Address 1') end end end describe '#provider_name' do + before do + allow(Geocoder.config).to receive(:lookup).and_return(:nominatim) + end + it 'returns the current geocoding provider name' do - allow(Geocoder.config).to receive(:lookup).and_return(:photon) - - expect(service.provider_name).to eq('Photon') - end - end - - describe 'provider-specific extraction' do - context 'with Photon provider' do - let(:photon_result) do - double( - 'Geocoder::Result', - latitude: 52.5200, - longitude: 13.4050, - data: { - 'properties' => { - 'name' => 'Kaufland', - 'street' => 'Alexanderplatz', - 'housenumber' => '1', - 'city' => 'Berlin', - 'state' => 'Berlin', - 'country' => 'Germany' - } - } - ) - end - - before do - allow(Geocoder).to receive(:search).and_return([photon_result]) - allow(Geocoder.config).to receive(:lookup).and_return(:photon) - end - - it 'extracts Photon-specific data correctly' do - results = service.search('test') - - expect(results.first[:name]).to eq('Kaufland') - expect(results.first[:address]).to eq('Alexanderplatz, 1, Berlin, Berlin, Germany') - end - end - - context 'with Nominatim provider' do - let(:nominatim_result) do - double( - 'Geocoder::Result', - latitude: 52.5200, - longitude: 13.4050, - data: { - 'display_name' => 'Kaufland, Alexanderplatz 1, Berlin, Germany', - 'type' => 'shop', - 'class' => 'amenity' - } - ) - end - - before do - allow(Geocoder).to receive(:search).and_return([nominatim_result]) - allow(Geocoder.config).to receive(:lookup).and_return(:nominatim) - end - - it 'extracts Nominatim-specific data correctly' do - results = service.search('test') - - expect(results.first[:name]).to eq('Kaufland') - expect(results.first[:address]).to eq('Kaufland, Alexanderplatz 1, Berlin, Germany') - expect(results.first[:type]).to eq('shop') - end + expect(service.provider_name).to eq('Nominatim') end end end \ No newline at end of file diff --git a/spec/services/location_search/point_finder_spec.rb b/spec/services/location_search/point_finder_spec.rb index 9adc3ab3..d2d5a992 100644 --- a/spec/services/location_search/point_finder_spec.rb +++ b/spec/services/location_search/point_finder_spec.rb @@ -47,8 +47,9 @@ RSpec.describe LocationSearch::PointFinder do end before do - allow_any_instance_of(LocationSearch::GeocodingService) - .to receive(:search).and_return(mock_geocoded_locations) + allow(LocationSearch::GeocodingService).to receive(:new).and_return( + double('GeocodingService', search: mock_geocoded_locations, provider_name: 'Test Provider') + ) allow_any_instance_of(LocationSearch::SpatialMatcher) .to receive(:find_points_near).and_return(mock_matching_points) @@ -82,8 +83,9 @@ RSpec.describe LocationSearch::PointFinder do end it 'calls geocoding service with the query' do - expect_any_instance_of(LocationSearch::GeocodingService) - .to receive(:search).with('Kaufland') + expect(LocationSearch::GeocodingService) + .to receive(:new).with('Kaufland') + .and_return(double('GeocodingService', search: mock_geocoded_locations, provider_name: 'Test Provider')) service.call end @@ -160,8 +162,9 @@ RSpec.describe LocationSearch::PointFinder do context 'when no geocoding results found' do before do - allow_any_instance_of(LocationSearch::GeocodingService) - .to receive(:search).and_return([]) + allow(LocationSearch::GeocodingService).to receive(:new).and_return( + double('GeocodingService', search: [], provider_name: 'Test Provider') + ) end it 'returns empty result' do @@ -174,8 +177,9 @@ RSpec.describe LocationSearch::PointFinder do context 'when no matching points found' do before do - allow_any_instance_of(LocationSearch::GeocodingService) - .to receive(:search).and_return([{ lat: 52.5200, lon: 13.4050, name: 'Test' }]) + allow(LocationSearch::GeocodingService).to receive(:new).and_return( + double('GeocodingService', search: [{ lat: 52.5200, lon: 13.4050, name: 'Test' }], provider_name: 'Test Provider') + ) allow_any_instance_of(LocationSearch::SpatialMatcher) .to receive(:find_points_near).and_return([]) @@ -206,8 +210,9 @@ RSpec.describe LocationSearch::PointFinder do let(:many_visits) { Array.new(15) { |i| { timestamp: i, date: "2024-01-#{i+1}T12:00:00Z" } } } before do - allow_any_instance_of(LocationSearch::GeocodingService) - .to receive(:search).and_return([{ lat: 52.5200, lon: 13.4050, name: 'Test' }]) + allow(LocationSearch::GeocodingService).to receive(:new).and_return( + double('GeocodingService', search: [{ lat: 52.5200, lon: 13.4050, name: 'Test' }], provider_name: 'Test Provider') + ) allow_any_instance_of(LocationSearch::SpatialMatcher) .to receive(:find_points_near).and_return([{}]) diff --git a/spec/services/points/raw_data_lonlat_extractor_spec.rb b/spec/services/points/raw_data_lonlat_extractor_spec.rb index 9fcb3800..f8f8d18d 100644 --- a/spec/services/points/raw_data_lonlat_extractor_spec.rb +++ b/spec/services/points/raw_data_lonlat_extractor_spec.rb @@ -18,7 +18,7 @@ RSpec.describe Points::RawDataLonlatExtractor do } } end - let(:point) { create(:point, user: user, raw_data: raw_data, longitude: nil, latitude: nil) } + let(:point) { create(:point, user: user, raw_data: raw_data, longitude: 0.0, latitude: 0.0) } it 'extracts longitude and latitude correctly' do expect { described_class.new(point).call }.to \ @@ -36,7 +36,7 @@ RSpec.describe Points::RawDataLonlatExtractor do 'latitudeE7' => 512_345_678 } end - let(:point) { create(:point, user: user, raw_data: raw_data, longitude: nil, latitude: nil) } + let(:point) { create(:point, user: user, raw_data: raw_data, longitude: 0.0, latitude: 0.0) } it 'extracts longitude and latitude correctly' do expect { described_class.new(point).call }.to \ @@ -55,7 +55,7 @@ RSpec.describe Points::RawDataLonlatExtractor do } } end - let(:point) { create(:point, user: user, raw_data: raw_data, longitude: nil, latitude: nil) } + let(:point) { create(:point, user: user, raw_data: raw_data, longitude: 0.0, latitude: 0.0) } it 'extracts longitude and latitude correctly' do expect { described_class.new(point).call }.to \ @@ -74,7 +74,7 @@ RSpec.describe Points::RawDataLonlatExtractor do } } end - let(:point) { create(:point, user: user, raw_data: raw_data, longitude: nil, latitude: nil) } + let(:point) { create(:point, user: user, raw_data: raw_data, longitude: 0.0, latitude: 0.0) } it 'extracts longitude and latitude correctly' do expect { described_class.new(point).call }.to \ @@ -92,7 +92,7 @@ RSpec.describe Points::RawDataLonlatExtractor do 'lat' => 51.2345678 } end - let(:point) { create(:point, user: user, raw_data: raw_data, longitude: nil, latitude: nil) } + let(:point) { create(:point, user: user, raw_data: raw_data, longitude: 0.0, latitude: 0.0) } it 'extracts longitude and latitude correctly' do expect { described_class.new(point).call }.to \ @@ -111,7 +111,7 @@ RSpec.describe Points::RawDataLonlatExtractor do } } end - let(:point) { create(:point, user: user, raw_data: raw_data, longitude: nil, latitude: nil) } + let(:point) { create(:point, user: user, raw_data: raw_data, longitude: 0.0, latitude: 0.0) } it 'extracts longitude and latitude correctly' do expect { described_class.new(point).call }.to \ @@ -129,7 +129,7 @@ RSpec.describe Points::RawDataLonlatExtractor do 'latitude' => 51.2345678 } end - let(:point) { create(:point, user: user, raw_data: raw_data, longitude: nil, latitude: nil) } + let(:point) { create(:point, user: user, raw_data: raw_data, longitude: 0.0, latitude: 0.0) } it 'extracts longitude and latitude correctly' do expect { described_class.new(point).call }.to \ @@ -148,7 +148,7 @@ RSpec.describe Points::RawDataLonlatExtractor do } } end - let(:point) { create(:point, user: user, raw_data: raw_data, longitude: nil, latitude: nil) } + let(:point) { create(:point, user: user, raw_data: raw_data, longitude: 0.0, latitude: 0.0) } # Mock the entire call method since service doesn't have nil check before do