JustinCanton / Geo.NET

A lightweight method for communicating with the multiple online geocoding APIs.
MIT License
13 stars 8 forks source link

MapBox 404 not handled. #42

Closed dernewt closed 1 year ago

dernewt commented 1 year ago

When a query to MapBox returns 0 results the library isn't handling it gracefully.

To Reproduce using nuget Geo.MapBox 1.1.1

Call Mapbox.GeocodingAsync with some query that returns no results.

Raw request:

Raw response

HTTP/1.1 404 Not Found
Content-Type: application/json; charset=utf-8
Content-Length: 23
Connection: keep-alive
Date: Tue, 09 Aug 2022 00:01:06 GMT
X-Powered-By: Express
Access-Control-Allow-Origin: *
Access-Control-Expose-Headers: x-rate-limit-interval,x-rate-limit-limit,x-rate-limit-remaining,x-rate-limit-reset
Cache-Control: max-age=604800
X-Rate-Limit-Limit: 600
X-Rate-Limit-Interval: 60
X-Rate-Limit-Reset: 1660003326
Last-Modified: Thu, 04 Aug 2022 19:09:01 GMT
Vary: Accept-Encoding
X-Cache: Error from cloudfront
Via: 1.1 5ece3a8d1e959c303daa9320e4fea502.cloudfront.net (CloudFront)
Age: 77

{"message":"Not Found"}

The library throws an exception with no exception data or inner exception.

Geo.MapBox.Models.Exceptions.MapBoxException
  HResult=0x80131500
  Message=The call to MapBox did not return a successful http status code. See the exception data for more information. See the inner exception for more information.
  Source=Geo.Core
  StackTrace:
   at Geo.Core.ClientExecutor.<CallAsync>d__4`2.MoveNext()
   at Geo.MapBox.Services.MapBoxGeocoding.<GeocodingAsync>d__9.MoveNext()
InnerException | null | System.Exception

Expected behavior One of the following in order of preference (you should probably standardize the "no result" response across your other providers:

  1. A Empty set/object
  2. null
  3. An exception properly indicating no results found.
JustinCanton commented 1 year ago

Thank you for the detailed ticket!

I agree that in cases of a 404 response, it is best to return an empty set, since that is what the 404 represents. I will make an update to fix this issue.

I am more concerned about the fact that this probably means other "bad" responses coming from the geocoding APIs are not properly being handled and returned in the exception, which would definitely be confusing.

I will try to release a fix/update for this ASAP.

JustinCanton commented 1 year ago

@dernewt So digging into this a little bit, the issue might be slightly different that you were thinking.

I wasn't able to reproduce it immediately. Sending a request to with a non-existent place, I get back and empty result from MapBox, as one would expect.

Raw request:

https://api.mapbox.com/geocoding/v5/mapbox.places/NONEXISTENTPLACE.json?autocomplete=true&fuzzyMatch=true&limit=1&routing=false&access_token=TOKEN

I get back the response

{
   "type":"FeatureCollection",
   "query":[
      "nonexistentplace"
   ],
   "features":[      
   ],
   "attribution":"NOTICE: © 2022 Mapbox and its suppliers. All rights reserved. Use of this data is subject to the Mapbox Terms of Service (https://www.mapbox.com/about/maps/). This response and the information it contains may not be retained. POI(s) provided by Foursquare."
}

Which is what I would expect. This is properly mapped and returns a correct response from the library.

It wasn't until I passed an "invalid" character (like you mentioned in https://github.com/JustinCanton/Geo.NET/issues/43), that I got a bad request.

Raw request:

https://api.mapbox.com/geocoding/v5/mapbox.places/?autocomplete=true&fuzzyMatch=true&limit=1&routing=false&access_token=TOKEN#.json

You will notice that the #.json is at the end of the string, which is unexpected, and like you mentioned, being caused by the URL builder. Sending this request, I get back the response

"{\"message\":\"Not Found\"}"

My assumption is this will be fixed once the uri is properly created and there is some method of handling # characters.

All of this said, the exception that is thrown does in fact contain a bit of information about the exception. It has the response body at the moment: image

I will expand a bit on the exception and what information it has, as well as the logging of the exception to actually display the data that it contains. But the main fix for this will have to be a part of https://github.com/JustinCanton/Geo.NET/issues/43.

dernewt commented 1 year ago

Whoops, sorry for the bad scenario, good catch on the actual problem. Overall really great repo btw.