geocoder-php / Geocoder

The most featured Geocoder library written in PHP.
https://geocoder-php.org
MIT License
3.94k stars 517 forks source link

Multiple cached provider shared same cacheKeys #1044

Closed AntoineLemaire closed 1 year ago

AntoineLemaire commented 4 years ago

I saw that every cache shared the same cacheKey. Even if you don't use the same provider.

That could be annoying if the results of my first provider give me a result, but does not contain what I need, and I want try an other one.

e.g, sometimes Google does not provide me postalCode of a city, but OpenStreetMap does

$httpClient = new Client();
$psr6Cache = new ArrayCachePool();

$googleProvider = new GoogleMaps($httpClient,null,'AIzaSyDv0F0CikvxzA__WFbAIgJ7v1B0Ll3QaNk');
$googleCachedProvider = new ProviderCache($googleProvider, $psr6Cache);

// Will come from Google Maps API
$result1 = $googleCachedProvider->geocodeQuery(GeocodeQuery::create('Buckingham Palace, London'));
// Will come from the cache
$result2 = $googleCachedProvider->geocodeQuery(GeocodeQuery::create('Buckingham Palace, London'));

$nominatimProvider = new Nominatim(
    $httpClient,
    'https://nominatim.openstreetmap.org',
    'MyUserAgent'
);
$nominatimCachedProvider = new ProviderCache($nominatimProvider, $psr6Cache);

// Will come from the cache of Google Maps API, and not OpenStreetMap
$result3 = $nominatimCachedProvider->geocodeQuery(GeocodeQuery::create('Buckingham Palace, London'));

Could we add a new argument to ProviderCache to include realProvider name into cache key ?

jbelien commented 4 years ago

Hello @AntoineLemaire , I understand your point !

The Cache Provider has been designed before I joined the project so I'm not sure if sharing the cache is done on purpose or not. I'll have to ask @Nyholm !

mwansinck commented 3 years ago

@jbelien @Nyholm Any updates on this?

In my opinion using the same cache key for different providers is incorrect and results unpredictable behavior, as @AntoineLemaire explained already.

bazinga_geocoder:
  providers:
    googleMaps:
      factory: Bazinga\GeocoderBundle\ProviderFactory\GoogleMapsFactory
      cache: 'app.psr16_cache'
      cache_lifetime: 31536000 
      cache_precision: 6
      options:
        api_key: '%env(GOOGLE_API_KEY)%'
    nationaalGeoregister:
      factory: App\Geocoder\ProviderFactory\NationaalGeoregisterFactory
      cache: 'app.psr16_cache'
      cache_lifetime: 2592000 
      cache_precision: 6
    chain:
      factory: Bazinga\GeocoderBundle\ProviderFactory\ChainFactory
      options:
        services: [  '@bazinga_geocoder.provider.nationaalGeoregister', '@bazinga_geocoder.provider.googleMaps' ]
<?php

$text = 'Empire state building';

$nationaalGeoregisterResult = $this->nationaalGeoregisterGeocoder->geocodeQuery(GeocodeQuery::create($text));
$googleMapsResult = $this->googleMapsGeocoder->geocodeQuery(GeocodeQuery::create($text));

In this case Nationaal Georegister Geocoder (free geocoder containing only Dutch data) comes with an illogic response (as could be predicted based on our search text). However due to caching configuration there is no way to retrieve a logic result from the Google Maps Geocoder because it's also returning the response from the Nationaal Georegister Geocoder.

Therefore caching should be separated for each provider, and if one would like to "simulate" the above behavior this could me simply done by adding caching information on the chain instead of the providers.