Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Geocoding: "undefined method `split'" when Nominatim (version 5) doesn't return full URL in more_url field #4137

Closed
mtmail opened this issue Aug 3, 2023 · 12 comments

Comments

@mtmail
Copy link
Contributor

mtmail commented Aug 3, 2023

URL

https://www.openstreetmap.org/search?query=banana

How to reproduce the issue?

Right now Nominatim doesn't seem to be reachable. The API request is done in Rails so I can't inspect the HTTP code or error message. If that happens the Rails prints an additional Ruby error "undefined method `split' for nil:NilClass" (which might or might not also be in the webserver error log)

Screenshot(s) or anything else?

image

@gravitystorm
Copy link
Collaborator

It's working for me (both on osm.org and also on a local machine) so perhaps it's an intermittent problem?

Alternatively, it could be based on the location you are looking at. The only .spilt call in geocoder_controller comes while parsing the bounding box attribute of the returned results, so it might be something location-specific that triggers it.

min_lat, max_lat, min_lon, max_lon = place.attributes["boundingbox"].split(",")

@mtmail
Copy link
Contributor Author

mtmail commented Aug 3, 2023

I found the issue is a couple of lines up:

 more_url_params = CGI.parse(URI.parse(results.attributes["more_url"]).query)

dulcy server returns just the query part of the URL in more_url, the other server return a full URL

curl 'https://dulcy.openstreetmap.org/search.php?q=london&format=xml'
more_url="q=london&addressdetails=1&limit=..."

curl 'https://vhagar.openstreetmap.org/search.php?q=london&format=xml'
more_url='https://vhagar.openstreetmap.org/search.php/?q=london&exclude_p...

That causes the error:

ruby -e'require "cgi"; p CGI.parse(URI.parse("q=London").query)'
[...]/ruby/2.6.0/cgi/core.rb:384:in `parse': undefined method `split' for nil:NilClass

ruby -e'require "cgi"; p CGI.parse(URI.parse("http://example.com/search?q=London").query)'
{"q"=>["London"]}

Dulcy is running the next version of Nominatim, the rewrite in Python, with Python webserver. You might also notice it puts the attributes in single quotes instead of double quotes (Python XML vs PHP XML serializing library).

Adding @lonvia to ask if that's an oversight, the attribute is called 'URL' after all or meant to stay.

@Firefishy
Copy link
Member

Firefishy commented Aug 3, 2023

Lonvia is on leave at the moment.

The python based server can be taken out of service by Ops if critical.

@tomhughes
Copy link
Member

tomhughes commented Aug 3, 2023

In any case it's a matter for the Nominatim issue tracker not this one.

@mtmail
Copy link
Contributor Author

mtmail commented Aug 3, 2023

now tracked at osm-search/Nominatim#3138

@pnorman
Copy link
Contributor

pnorman commented Aug 4, 2023

I think there's still a bug where it's giving an incorrect error

@tomhughes
Copy link
Member

Oh sorry I misunderstood. I thought the split was on the python side.

@tomhughes tomhughes reopened this Aug 4, 2023
@tomhughes
Copy link
Member

tomhughes commented Aug 4, 2023

I think @mtmail has the correct diagnosis - this is the backtrace I get:

NoMethodError - undefined method `split' for nil:NilClass:
  app/controllers/geocoder_controller.rb:94:in `search_osm_nominatim'
  app/controllers/application_controller.rb:295:in `better_errors_allow_inline'
  config/initializers/compressed_requests.rb:27:in `call'
  config/initializers/cors.rb:9:in `call'

In which case I think that this is a Nominatim side bug and there's not a huge amount we can do on our side as the exception is in a library.

What did you mean by "incorrect error" exactly @pnorman?

@mtmail mtmail changed the title undefined method `split' when Nomintim isn't reachable undefined method `split' when Nominatim isn't reachable Aug 4, 2023
@pnorman
Copy link
Contributor

pnorman commented Aug 5, 2023

I find the message a bit misleading because it's not an error contacting, it's a (potentially) invalid response.

@mtmail mtmail changed the title undefined method `split' when Nominatim isn't reachable Geocoding: "undefined method `split'" when Nominatim (version 5) doesn't return full URL in more_url field Aug 5, 2023
@mtmail
Copy link
Contributor Author

mtmail commented Aug 5, 2023

@pnorman You're right, I updated the title

@tomhughes
Copy link
Member

That's because we surround the entire method in an exception handler but the only exceptions we really expect to get are network related ones.

@lonvia
Copy link
Contributor

lonvia commented Aug 6, 2023

Fixed the regression on the Nominatim side in osm-search/Nominatim@996026e.

Looking at the code, the whole URL parsing is not really necessary. It is only done to get the excluded place IDs and they can be easier extracted from the exclude_place_ids attribute directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants