geocoder 1.7.4 broken

[ollym]

c32da2e

Released in geocoder 1.7.4 just broke our app.

We happen to have a top level class called Google totally unrelated to geocoder, and also

Geocoder.configure(
  lookup: :google
)

So tell me - how do I after this patch how do i make sure it picks the Google geocoder module instead?

[alexreisner]

@ollym thanks for pointing this out. We'll figure out a fix shortly.

@januszm do you have any recommendations? Should we change the design?

I'm thinking we need to add some namespacing requirement.

[ollym]

The short term fix is to do this in your initialiser:

require 'geocoder/lookups/google'

Geocoder.configure(
  lookup: :google
)

[alexreisner]

Yep, or downgrade temporarily to 1.7.3 (there are no security-related updates in 1.7.4).

[januszm]

@ollym Exactly what error did you see in your application?
Is it a Rails application, and if so, is it with Zeitwerk? Or just a Ruby app without autoloading?

[ollym]

@januszm yes, this is using Rails w/ Zeitwerk

The problem is Geocoder::Lookup.const_get('Google') will discover ::Google if it's defined (which it is in my case) because require "geocoder/lookups/google" is no longer being called first.

I suggest you change this

def lookup_instance_from(name)
  class_name = classify_name(name)
  begin
    Geocoder::Lookup.const_get(class_name)
  rescue NameError
    require "geocoder/lookups/#{name}"
  end
  Geocoder::Lookup.const_get(class_name).new
end

To this:

def lookup_instance_from(name)
  class_name = classify_name(name)
  begin
    require "geocoder/lookups/#{name}"
  rescue LoadError
  end
  Geocoder::Lookup.const_get(class_name).new
end

[januszm]

@ollym could you please paste the exact error message you're getting?

I could not reproduce this on Rails 7 application with Zeitwerk. I defined an almost empty Google class in the top-level namespace, put Geocoder.configure(lookup: :google) in the initializer and everything went well, I could use both the Geocoder and the Google class.

[ollym]

NoMethodError
undefined method `new' for Google:Module Geocoder::Lookup.const_get(class_name).new

Google in our case is already defined by google-cloud-trace gem

[ollym]

@januszm @alexreisner Something simple to reproduce it:

require 'geocoder'

module Google
end

Geocoder.search('Some Address', lookup: :google)

[januszm]

So in your case Google is a module. I still can't reproduce this, with the debugger inside the gem code:

> Geocoder.search('Some Address', lookup: :google)

From: /lib/geocoder/lookup.rb:142 Geocoder::Lookup#instantiate_lookup:

    138: def instantiate_lookup(name)
    139:   class_name = classify_name(name)
    140:   binding.pry
    141:   begin
 => 142:     Geocoder::Lookup.const_get(class_name)
    143:   rescue NameError
    144:     require "geocoder/lookups/#{name}"
    145:   end
    146:   Geocoder::Lookup.const_get(class_name).new
    147: end

> class_name
=> "Google"

This could be a problem if we used Object.const_get, but since we're already calling this method from Geocoder, it resolves correctly:

> Object.const_get("Google")
=> Google

> Geocoder::Lookup.const_get("Google")
=> Geocoder::Lookup::Google

Perhaps we should prepend Geocoder::Lookup:: to the class_name variable when we const_get it.
I'll give it another try later today.

[januszm]

@ollym I was able to reproduce it in development environment, with eager loading off. It works fine in production or with eager_load true.

[alexreisner]

We should definitely get this working in all environments, including non-Rails.

[januszm]

Indeed. We have several options, including requiring all predefined lookups, but I guess there was a reason we only spawn what's necessary, so this should do the trick:

def instantiate_lookup(name)
  class_name = "Geocoder::Lookup::#{classify_name(name)}"

  begin
    Geocoder::Lookup.const_get(class_name)
  rescue NameError
    require "geocoder/lookups/#{name}"
  end
  Geocoder::Lookup.const_get(class_name).new
end

It would be great to have a test case with a "Dummy" application where we could define at least one module with a name conflict with one of the lookups, but at the moment I don't have enough time to do such a test for the gem.

[alexreisner]

Thanks @januszm! I just merged your PR and will release a new gem version within the hour.