Firestore/encoding text

[giautm]

Re-open PR for #1425

cc: @treeder

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

[giautm]

@googlebot I fixed it.

[giautm]

As I said on Grit time.Time also implement TextMarshal, so it break the test case. And I don't have solution to fix that test, or we just ignore that test case.

Patch Set 3:

Follow the resultstore links to see test failures. Currently it's:

=== RUN TestToProtoValue_Embedded
--- FAIL: TestToProtoValue_Embedded (0.00s)
to_value_test.go:433: got string_value:"2016-12-25T00:00:00.123456789Z" , want map_value:<fields:<key:"LatLng" >value:<geo_point_value:<latitude:20 longitude:30 > > > fields:<key:"Time" value:<timestamp_value:<seconds:1482624000 >nanos:123456789 > > > fields:<key:"Timestamp" value:<timestamp_value:<seconds:12345 nanos:67890 > > > >

Well, because in this test case, that structure embed time.Time, it implemented TextMarshal too. No solution yet.

[tritone]

I'm not super familiar with this code but IIUC I don't think ignoring the test case is a reasonable solution-- embedding types that also implement TextMarshaler should be permitted.

Do you have any other ideas about how to resolve this? Also, could you file a feature request issue in this repo describing what you're trying to achieve with this change (or let me know if there already is one that should be linked)?

[treeder]

Some previous issues related to this:

• Firestore: Support Custom Marsheling and Unmarsheling #1416
• Firestore: support custom JSON Marshal/Unmarshal #1475

Also, things like shopspring/decimal just plain don't work even though it's got all the standard marshaller's implemented.

[treeder]

The only workaround seems to be doing things like this:

type X struct {
  AmountS  string `firestore:"amount" json:"-"`                                // in decimal format
  Amount decimal.Decimal `firestore:"-" json:"amount"`
}

// then before saving to firestore:
t.AmountS = t.Amount.String()
// And do the reverse on the way out

[tritone]

@treeder thanks very much for filling me in on the context!

Do you have any thoughts on how to resolve the conflict with time.Time which also implements TextUnmarshaler?

Also cc @BenWhitehead who is assigned to some of the issues in question.

[treeder]

I don't really understand what the issue is with embedding the time.Time as I can't see your internal tools. From the little bit I do see in that comment, is this because you are marshalling time.Time's with your own special marshaller?

[treeder]

time.Time's MarshalText value should be able to be used directly with your API, this is the format MarshalText() returns:

2009-11-10T23:00:00Z

See https://play.golang.org/p/d5XPO3-M720

And that is what your API expects, so I'm not sure why it wouldn't work. Perhaps your test should be changed?

[treeder]

Or if you could share the test code, that would be useful to offer suggestions.

[giautm]

@treeder this is failed test case. embed struct also embed time.Time, so it implemented encoding.TextMarshaler. That why the test case failed.

func TestToProtoValue_Embedded(t *testing.T) {
	// Embedded time.Time, LatLng, or Timestamp should behave like non-embedded.
	type embed struct {
		time.Time
		*latlng.LatLng
		*ts.Timestamp
	}

	got, _, err := toProtoValue(reflect.ValueOf(embed{tm, ll, ptm}))
	if err != nil {
		t.Fatal(err)
	}
	want := mapval(map[string]*pb.Value{
		"Time":      tsval(tm),
		"LatLng":    geoval(ll),
		"Timestamp": {ValueType: &pb.Value_TimestampValue{ptm}},
	})
	if !testEqual(got, want) {
		t.Errorf("got %+v, want %+v", got, want)
	}
}

Result:

2020/06/30 09:38:35 No 'Application Default Credentials' found.
--- FAIL: TestToProtoValue_Embedded (0.00s)
    to_value_test.go:433: got string_value:"2016-12-25T00:00:00.123456789Z" , want map_value:<fields:<key:"LatLng" value:<geo_point_value:<latitude:20 longitude:30 > > > fields:<key:"Time" value:<timestamp_value:<seconds:1482624000 nanos:123456789 > > > fields:<key:"Timestamp" value:<timestamp_value:<seconds:12345 nanos:67890 > > > > 
FAIL
coverage: 84.8% of statements
FAIL	cloud.google.com/go/firestore	4.279s

[treeder]

Hmm, that seems like a language shortcoming to me, discussed here, here, here and even a proposal to remove embedding completely because of things like this here: golang/go#22013

I would argue that embedding time.Time in another struct is extremely uncommon and probably not very useful especially when using Firestore (can't think of a case where that would make sense).

[tritone]

Hmm, that seems like a language shortcoming to me, discussed here, here, here and even a proposal to remove embedding completely because of things like this here: golang/go#22013

I would argue that embedding time.Time in another struct is extremely uncommon and probably not very useful especially when using Firestore (can't think of a case where that would make sense).

Hah, that proposal regarding dropping embedding from the language completely looks like a big and controversial change! I see the frustration but I would be very surprised if that comes to pass.

In any case, having looked into this a bit more I believe dropping support for embedded types which implement TextUnmarshaler (which includes types other than time.Time of course) would be a breaking change, which we can't do short of a new major version for the library. So unfortunately I don't think we can accept this change as is.

There are a couple of other related issues including #1740 which have been raised as impacting the usability of this package; they unfortunately haven't made it to the top of the priority list quite yet and are going to take some concerted time and attention for us to collectively unravel. We'll keep this PR in mind when designing out a solution to improve the unmarshaling experience overall, and appreciate your patience.

[treeder]

This same thing would occur with JSON marshallng, etc. See: https://play.golang.org/p/o48AZVFesEy .

You'd think at the very least you could make this library behave like JSON and in that case, your test should pass. Why not do a major version bump? With the main goal being: "Version 2 will marshal like you'd expect with JSON marshaling"?

[tritone]

This same thing would occur with JSON marshallng, etc. See: https://play.golang.org/p/o48AZVFesEy .

You'd think at the very least you could make this library behave like JSON and in that case, your test should pass. Why not do a major version bump? With the main goal being: "Version 2 will marshal like you'd expect with JSON marshaling"?

Just to be clear, we do plan on figuring out some solution for this use case that is backwards-compatible, so that we don't need a new major version. It's just going to take a bit of work on our end to figure out a good approach which will handle edge cases such as embedded types without causing problems.

[treeder]

Any news on this? 3 years now...

Had an idea pop into my head after over 3 years of being annoyed and if you still don't want to do a major version, how about a flag when creating the Firestore object to enable "what you'd expect, like JSON" handling, eg:

fs, err := firebaseApp.Firestore(ctx, &firestore.Options{ActLikeJSON: true}) 

Otherwise it will stick to existing behavior.