googleapis / google-cloud-go

Google Cloud Client Libraries for Go.
https://cloud.google.com/go/docs/reference
Apache License 2.0
3.72k stars 1.27k forks source link

latlng.LatLng type mapping to GeoPoint extrange behaiviour #962

Closed mblasi closed 6 years ago

mblasi commented 6 years ago

(delete this for feature requests)

Client

Firestore golang library

Describe Your Environment

The backend is running in GAE flexible

Expected Behavior

import "google.golang.org/genproto/googleapis/type/latlng"

type Location struct{ Name string Point latlng.LatLng }

I do:

_, err := store.Collection("users").Doc(userId).Update(ctx, []firestore.Update{{Path: "last_known_location", Value: user.Location}})

I expect to see the point document as type GeoPoint.

In other place, I have:

type Address struct { Street string, City string, Latitude float64, Longitude float64 }

When I do:

_, err := doc.Collection("sections").Doc(new-id).Set(ctx, section) //section has an Address

I expect to see the "address" as type object with all its properties (street, city, latitude, longitude...)

Actual Behavior

I see the "address" document with type GeoPoint!!! And I have street and city values missing!

jba commented 6 years ago

I tried to reproduce this, but I realized I don't have enough information.

I expect to see the point document as type GeoPoint.

What do you mean by "see"? When I run

    dr := client.Collection("users").Doc(userId)
    if _, err := dr.Set(ctx, map[string]interface{}{
        "last_known_location": Location{
            Name:  "L",
            Point: latlng.LatLng{Latitude: 18, Longitude: 71},
        },
    }); err != nil {
        log.Fatal(err)
    }
    if err != nil {
        log.Fatal(err)
    }
    doc, err := dr.Get(ctx)
    if err != nil {
        log.Fatal(err)
    }
    var data struct {
        L Location `firestore:"last_known_location"`
    }
    if err := doc.DataTo(&data); err != nil {
        log.Fatal(err)
    }
    fmt.Printf("%#v\n", data)
    fmt.Printf("%#v\n", doc.Data())

I see

struct { L main.Location "firestore:\"last_known_location\"" }{L:main.Location{Name:"L", Point:latlng.LatLng{Latitude:18, Longitude:71}}}
map[string]interface {}{"last_known_location":map[string]interface {}{"Name":"L", "Point":map[string]interface {}{"Longitude":71, "Latitude":18}}}

which is the expected output.

GeoPoint isn't even a type in the Go client, so I'm confused.

Can you give me a program that reproduces this problem, and also the problem with Address?

mblasi commented 6 years ago

I'm trying to think how to reproduce in a program, perhaps it could help to understand:

When I say "I expect to see the point document as type GeoPoint.", I refer to documentation:

  • latlng.LatLng converts to GeoPoint. latlng is the package "google.golang.org/genproto/googleapis/type/latlng".

When I insert a doc with a latlng.LatLng field, I see it as a pair of booleans. And when I insert a doc with a Latitude, Longitude float64 pair, I see them as a GeoPoint.

It looks like working inverted...

When I say "I see" I'm talking about de firestore console: https://console.firebase.google.com/project//database/firestore/

jba commented 6 years ago

I see, I didn't realize you were looking at the data with the console.

When I use the console, I see what I would expect to. The document I set in the code above, using your Location struct, looks like this: image

When I use your Address struct and this code:

    _, err = dr2.Set(ctx, map[string]interface{}{
        "addr": Address{
            Street:    "S",
            City:      "C",
            Latitude:  32.5,
            Longitude: 18.2,
        },
    })

the console shows image

mblasi commented 6 years ago

I can't figure out why, but actually, I'm seeing the same for the Address case (the latitude&longitude floats pair, and the rest of the struct values as expected. I did some changes to my model, but I can't understand what fixed that.

By other hand, I still see last_known_location.Point as a pair of floats (same as you) instead of seeing it as a GeoPoint... it is confusing, the console showed me that for the previous case (Address):

addr: GeoPoint(32W 18N)

I would like to understand that, because in the future I'll need to make some geolocation searchs... And I supose it should be correctly modeled...

Regards, Matías.

jba commented 6 years ago

OK, I understand what's happening with Location. You need to use *latlng.LatLng to get a GeoPoint. I consider that a bug and will fix.

What happened with Address I have no idea. Is it possible an earlier version of that struct used *latlng.LatLng?

mblasi commented 6 years ago

Ohh, great news! I'll try that. By sure, Address never had *latlng.LatLng, just the pair of floats, and several other fields (it is mapped to the android Address object)... I can swear it was represented by a GeoPoint missing all other fields. If I can reproduce it, I'll back...

Anyway, I'll try changing my field to a pointer and will be confirming it here!

Thanks a lot!

jba commented 6 years ago

Actually, I think the right thing to do is leave the code as it is, and fix the doc. You should use *latlng.LatLng throughout.

There are two problems with allowing latlng.LatLng: first, retrieving a GeoPoint into an interface{}, like with DocumentSnapshot.Data, will always give you a pointer. Second, embedding like this:

type embed struct {
    latlng.LatLng
}

will not work as expected—the client will treat embed as having two fields named Latitude and Longitude instead of a single LatLng. Embedding a pointer to a LatLng works as expected.