From 4e358bbfd488eda86efa3265a6c443be0ae8f038 Mon Sep 17 00:00:00 2001 From: Michael Mayer Date: Thu, 9 Dec 2021 01:10:15 +0100 Subject: [PATCH] Places: Improve handling of unknown S2 cell ids --- internal/entity/cell.go | 3 +- internal/entity/cell_test.go | 2 +- internal/entity/photo_location.go | 35 +++++++++++------------ internal/hub/places/location.go | 46 +++++++++++++++++++++++++------ internal/maps/location.go | 4 +-- 5 files changed, 61 insertions(+), 29 deletions(-) diff --git a/internal/entity/cell.go b/internal/entity/cell.go index f52244d4d..f37d24787 100644 --- a/internal/entity/cell.go +++ b/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 } diff --git a/internal/entity/cell_test.go b/internal/entity/cell_test.go index e38922ec7..2a42e35f7 100644 --- a/internal/entity/cell_test.go +++ b/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()) }) } diff --git a/internal/entity/photo_location.go b/internal/entity/photo_location.go index a50f73700..08e61f80d 100644 --- a/internal/entity/photo_location.go +++ b/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 != "" { diff --git a/internal/hub/places/location.go b/internal/hub/places/location.go index d0e684f56..fe40c89eb 100644 --- a/internal/hub/places/location.go +++ b/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,10 +113,10 @@ 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 } @@ -109,78 +124,93 @@ func FindLocation(id string) (result Location, err error) { 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" } diff --git a/internal/maps/location.go b/internal/maps/location.go index daa3742d1..44850d385 100644 --- a/internal/maps/location.go +++ b/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 {