Places: Improve handling of unknown S2 cell ids

Author: lastzero

internal/entity/cell.go

@@ -74,7 +74,7 @@ func (m *Cell) Refresh(api string) (err error) {
 	cellMutex.Lock()
 	defer cellMutex.Unlock()
 
-	// Query geodata API.
+	// Retrieve location details from Places API.
 	if err = l.QueryApi(api); err != nil {
 		return err
 	}
@@ -150,6 +150,7 @@ func (m *Cell) Find(api string) error {
 	cellMutex.Lock()
 	defer cellMutex.Unlock()
 
+	// Retrieve location details from Places API.
 	if err := l.QueryApi(api); err != nil {
 		return err
 	}

internal/entity/cell_test.go

@@ -62,7 +62,7 @@ func TestLocation_Find(t *testing.T) {
 			t.Fatal("error expected")
 		}
 
-		assert.Equal(t, "maps: reverse lookup disabled", err.Error())
+		assert.Equal(t, "maps: location lookup disabled", err.Error())
 	})
 }
 

internal/entity/photo_location.go

@@ -358,36 +358,37 @@ func (m *Photo) GetTakenAtLocal() time.Time {
 // UpdateLocation updates location and labels based on latitude and longitude.
 func (m *Photo) UpdateLocation() (keywords []string, labels classify.Labels) {
 	if m.HasLatLng() {
-		var location = NewCell(m.PhotoLat, m.PhotoLng)
+		var loc = NewCell(m.PhotoLat, m.PhotoLng)
 
-		err := location.Find(GeoApi)
-
-		if err != nil {
+		if loc.Unknown() {
+			// Empty or unknown S2 cell id... should not happen, unless coordinates are invalid.
+			log.Warnf("photo: unknown cell id for lat %f, lng %f (uid %s)", m.PhotoLat, m.PhotoLng, m.PhotoUID)
+		} else if err := loc.Find(GeoApi); err != nil {
 			log.Errorf("photo: %s (find location)", err)
-		} else if location.Place == nil {
-			log.Warnf("photo: failed fetching geo data (uid %s, cell %s)", m.PhotoUID, location.ID)
-		} else if location.ID != UnknownLocation.ID {
-			changed := m.CellID != location.ID
+		} else if loc.Place == nil {
+			log.Warnf("photo: failed fetching geo data (uid %s, cell %s)", m.PhotoUID, loc.ID)
+		} else if loc.ID != UnknownLocation.ID {
+			changed := m.CellID != loc.ID
 
 			if changed {
-				log.Debugf("photo: changing location of %s from %s to %s", m.String(), m.CellID, location.ID)
+				log.Debugf("photo: changing location of %s from %s to %s", m.String(), m.CellID, loc.ID)
 				m.RemoveLocationLabels()
 			}
 
-			m.Cell = location
-			m.CellID = location.ID
-			m.Place = location.Place
-			m.PlaceID = location.PlaceID
-			m.PhotoCountry = location.CountryCode()
+			m.Cell = loc
+			m.CellID = loc.ID
+			m.Place = loc.Place
+			m.PlaceID = loc.PlaceID
+			m.PhotoCountry = loc.CountryCode()
 
 			if changed && m.TakenSrc != SrcManual {
 				m.UpdateTimeZone(m.GetTimeZone())
 			}
 
-			FirstOrCreateCountry(NewCountry(location.CountryCode(), location.CountryName()))
+			FirstOrCreateCountry(NewCountry(loc.CountryCode(), loc.CountryName()))
 
-			locCategory := location.Category()
-			keywords = append(keywords, location.Keywords()...)
+			locCategory := loc.Category()
+			keywords = append(keywords, loc.Keywords()...)
 
 			// Append category from reverse location lookup
 			if locCategory != "" {

internal/hub/places/location.go

@@ -25,6 +25,7 @@ type Location struct {
 	Cached      bool    `json:"-"`
 }
 
+// ApiName is the backend API name.
 const ApiName = "places"
 
 var Key = "f60f5b25d59c397989e3cd374f81cdd7710a4fca"
@@ -36,18 +37,30 @@ var Retries = 3
 var RetryDelay = 33 * time.Millisecond
 var client = &http.Client{Timeout: 60 * time.Second}
 
+// FindLocation retrieves location details from the backend API.
 func FindLocation(id string) (result Location, err error) {
-	if len(id) > 16 || len(id) == 0 {
-		return result, fmt.Errorf("invalid cell %s (%s)", id, ApiName)
+	// Normalize S2 Cell ID.
+	id = s2.NormalizeToken(id)
+
+	// Valid?
+	if len(id) == 0 {
+		return result, fmt.Errorf("empty cell id")
+	} else if n := len(id); n < 4 || n > 16 {
+		return result, fmt.Errorf("invalid cell id %s", txt.Quote(id))
 	}
 
+	// Remember start time.
 	start := time.Now()
+
+	// Convert S2 Cell ID to latitude and longitude.
 	lat, lng := s2.LatLng(id)
 
+	// Return if latitude and longitude are null.
 	if lat == 0.0 || lng == 0.0 {
-		return result, fmt.Errorf("skipping lat %f, lng %f (%s)", lat, lng, ApiName)
+		return result, fmt.Errorf("skipping lat %f, lng %f", lat, lng)
 	}
 
+	// Location details cached?
 	if hit, ok := cache.Get(id); ok {
 		log.Tracef("places: cache hit for lat %f, lng %f", lat, lng)
 		cached := hit.(Location)
@@ -58,11 +71,13 @@ func FindLocation(id string) (result Location, err error) {
 	// Compose request URL.
 	url := fmt.Sprintf(ReverseLookupURL, id)
 
+	// Log request URL.
 	log.Tracef("places: sending request to %s", url)
 
 	// Create GET request instance.
 	req, err := http.NewRequest(http.MethodGet, url, nil)
 
+	// Ok?
 	if err != nil {
 		log.Errorf("places: %s", err.Error())
 		return result, err
@@ -98,89 +113,104 @@ func FindLocation(id string) (result Location, err error) {
 
 	// Failed?
 	if err != nil {
-		log.Errorf("places: %s (http request)", err.Error())
+		log.Errorf("places: %s (http request failed)", err.Error())
 		return result, err
 	} else if r.StatusCode >= 400 {
-		err = fmt.Errorf("request failed with code %d (%s)", r.StatusCode, ApiName)
+		err = fmt.Errorf("request failed with code %d", r.StatusCode)
 		return result, err
 	}
 
 	// Decode JSON response body.
 	err = json.NewDecoder(r.Body).Decode(&result)
 
 	if err != nil {
-		log.Errorf("places: %s (decode json)", err.Error())
+		log.Errorf("places: %s (decode json failed)", err.Error())
 		return result, err
 	}
 
 	if result.ID == "" {
-		return result, fmt.Errorf("no result for %s (%s)", id, ApiName)
+		return result, fmt.Errorf("no result for %s", id)
 	}
 
 	cache.SetDefault(id, result)
-	log.Tracef("places: cached cell %s [%s]", id, time.Since(start))
+	log.Tracef("places: cached cell %s [%s]", txt.Quote(id), time.Since(start))
 
 	result.Cached = false
 
 	return result, nil
 }
 
+// CellID returns the S2 cell identifier string.
 func (l Location) CellID() string {
 	return l.ID
 }
 
+// PlaceID returns the place identifier string.
 func (l Location) PlaceID() string {
 	return l.Place.PlaceID
 }
 
+// Name returns the location name if any.
 func (l Location) Name() (result string) {
 	return strings.SplitN(l.LocName, "/", 2)[0]
 }
 
+// Street returns the location street if any.
 func (l Location) Street() (result string) {
 	return strings.SplitN(l.LocStreet, "/", 2)[0]
 }
 
+// Postcode returns the location postcode if any.
 func (l Location) Postcode() (result string) {
 	return strings.SplitN(l.LocPostcode, "/", 2)[0]
 }
 
+// Category returns the location category if any.
 func (l Location) Category() (result string) {
 	return l.LocCategory
 }
 
+// Label returns the location label.
 func (l Location) Label() (result string) {
 	return l.Place.LocLabel
 }
 
+// City returns the location address city name.
 func (l Location) City() (result string) {
 	return l.Place.LocCity
 }
 
+// District returns the location address district name.
 func (l Location) District() (result string) {
 	return l.Place.LocDistrict
 }
 
+// CountryCode returns the location address country code.
 func (l Location) CountryCode() (result string) {
 	return l.Place.LocCountry
 }
 
+// State returns the location address state name.
 func (l Location) State() (result string) {
 	return txt.NormalizeState(l.Place.LocState, l.CountryCode())
 }
 
+// Latitude returns the location position latitude.
 func (l Location) Latitude() (result float64) {
 	return l.LocLat
 }
 
+// Longitude returns the location position longitude.
 func (l Location) Longitude() (result float64) {
 	return l.LocLng
 }
 
+// Keywords returns location keywords if any.
 func (l Location) Keywords() (result []string) {
 	return txt.UniqueWords(txt.Words(l.Place.LocKeywords))
 }
 
+// Source returns the backend API name.
 func (l Location) Source() string {
 	return "places"
 }

internal/maps/location.go

@@ -43,11 +43,11 @@ type LocationSource interface {
 
 func (l *Location) QueryApi(api string) error {
 	switch api {
-	case "places":
+	case places.ApiName:
 		return l.QueryPlaces()
 	}
 
-	return errors.New("maps: reverse lookup disabled")
+	return errors.New("maps: location lookup disabled")
 }
 
 func (l *Location) QueryPlaces() error {