Invalid Json gets cached

[travisp]

Using a short term redis cache with Geocoder, we found that geocoder cached the following response:

status: PROXY_OVER_QUERY_LIMIT

And would continue therefore, to raise a JSON parser error every time we tried the same query. This response was caused by a proxy we had between geocoder and a service (quotaguard), but it seems that geocoder should maybe not cache a result like this if possible

[alexreisner]

Yep, thanks, this looks like a bug. There are caching issues with some of the lookups since each behaves differently with regard to errors. Geocoder's default behavior is to cache any response with a 2xx or 3xx HTTP response code. For Geocodio this appears not to be sufficient. #840 and #847 show the handling of a similar case with Bing. If anyone's interested in submitting a PR, this is probably just a matter of researching what kinds of responses are returned by Geocodio and implementing valid_response? in the lookup.

[travisp]

Sorry @alexreisner, I edited the report shortly after I submitted it, when I probably should have updated it via a new comment. The error was actually not coming from geocodio, but from our proxy service (quotaguard). The geocodio lookup responses seem to be accurate.

I don't know whether or not that's something like the error returned by quotaguard is appropriate for geocoder to handle, but it seems that it might be helpful to have a way to not store something in the cache if any error is encountered while processing it. This would have the added benefit of dealing with services that add an unexpected new response (and preventing situations where something like a "bad" Bing response is cached prior to a geocoder upgrade). However, I imagine this would be a more involved change to the caching logic than what was required for Bing.

[alexreisner]

Hmm...I think I understand. Is it possible to distinguish this kind of error from a good response at the lookup level? In other words, could this be handled by valid_response? in the base lookup?

[travisp]

If we knew that successful response were expected to be in json format, I think the base lookup could handle this, but otherwise, it might be a bit too much for the base lookup to know about. Some of the individual valid_response? lookup methods do call parse_json, which I think would catch this error, but I don't know that we could extend that across all lookups.

Alternately, if we wanted the base lookup to know about QuotaGuard, it could look for this type of response in the base lookup. However, QuotaGuard may be a bit too specialized to justify modifying the base lookup for its response. So, unless you think otherwise, I'll probably just override the base valid_response? in our own code base to look for PROXY_OVER_QUERY_LIMIT.

[alexreisner]

It's a little tricky to solve, but the situation seems common enough that you shouldn't need to modify the gem to get around it. One solution could be the addition of a proxy_valid_response? method that is called when valid_response? is called. It could check for known/common responses from services like QuotaGuard. And/or it could allow a developer to configure what kinds of things a proxy might insert into a response to mark it uncacheable.

I'm not sold on any of this, but I think it seems worth considering some kind of solution.

[rdlugosz]

I recently ran into a similar problem where somehow certain addresses were cached with an empty result ( [] ). Subsequent searches on this address would return the empty result from the cache even though putting the same address into Google gave a valid geocode.

I suspect what happened is that the Google geocoder was having an issue when the result was originally cached, and now it's stuck in there. Was thinking a quick fix would be to automatically retry a search if it came back as a cache_hit & an empty result... but looks like there isn't a simple way to force a cache bypass—and even if you could bypass it, you'd want to overwrite the cached empty value with the valid results.

Maybe an option like Geocoder.search(address, overwrite_cache: true) would resolve this?

Thoughts? Seems like in general it'd be useful to have a little more access to/control over the cache beyond what we have today.

[alexreisner]

@rdlugosz this is not quite the same thing @travisp is talking about. An empty result is in many cases correct and therefore should be cached. In my opinion we really need to focus more on identifying all the ways in which each API indicates an error, or any reason a result shouldn't be cached. But if you want to discuss this further, please create a separate issue?

[rdlugosz]

That's reasonable. In our project we actually reimplemented the API call so that we could have more control over the caching & moved away from the gem (we don't use the ActiveRecord integration the gem provides).

One thing I saw while implementing this was that occasionally you'll get a 500 returned from the service along with a response body that looks like this: { "results" : [], "status" : "UNKNOWN_ERROR" }. Can't say for sure if this is the empty result set being cached in my case or what the Geocoder gem does with this exact response, but if I get a minute I'll mock it and try it out.

[alexreisner]

@rdlugosz thanks! Any additional info would be much appreciated. I'm very interested in improving this part of the gem, especially since it caused you to move to another solution. Currently it should only be caching responses from Google if the status is OK or ZERO_RESULTS, but maybe others are somehow getting through the valid_response? method:

def valid_response?(response)
  json = parse_json(response.body)
  status = json["status"] if json
  super(response) and ['OK', 'ZERO_RESULTS'].include?(status)
end

[rdlugosz]

Hm... if that's the case then it would seem that Google will occasionally return an OK or ZERO_RESULTS status in spite of an errant empty result set. I've opened a new issue to track this one.

fwiw, sorry to migrate away from the gem. No offense implied. :) We were using such a small fraction of the capabilities (the single call to the Google geocoder) that it made sense to implement ourselves instead of picking up all the unused features.

[alexreisner]

And no offense taken! It makes total sense. Also seems like a good opportunity to improve Geocoder. Let's continue discussing at #1148. Thanks!