Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DocumentSnapshot timestamp fields broken by proto-plus #398

Closed
tseaver opened this issue Jul 15, 2021 · 2 comments · Fixed by #467
Closed

DocumentSnapshot timestamp fields broken by proto-plus #398

tseaver opened this issue Jul 15, 2021 · 2 comments · Fixed by #467
Labels
api: firestore Issues related to the googleapis/python-firestore API. type: question Request for information or clarification. Not an issue.

Comments

@tseaver
Copy link
Contributor

tseaver commented Jul 15, 2021 •
edited
Loading

We document that DocumentSnapshot.{read_time,create_time,update_time} are of type google.protobuf.timestamp_pb2.Timestamp, and the current implementation of DocumentSnapshot.__hash__ expects that to be true. However, the use of proto-plus by the microgenerator means that those attributes are now actually being populated with instances of proto.datetime_helpers.DatetimeWithNanoseconds: this discrepancy is the source of #391.

Fixing this is tricky: there may be code out there which has adapted to the change in actual type for those attributes. We basically have several choices:

  • Change the DocumentSnapshot ctor to forcibly convert back to the Timestamp (ick).
  • Change the call sites which construct DocumentSnapshot instances to use the _pb version, passing Timestamp messages into DocumentSnapshot.
  • Document that the attributes are of the new type, and change the implementation of __hash__ accordingly.

Any of these choices may involve tweaking e.g. the conformance tests, the firefox_bundle code, etc., to match.

@crwilcox, @craiglabenz WDYT?
@tseaver tseaver added the type: question Request for information or clarification. Not an issue. label Jul 15, 2021
@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/python-firestore API. label Jul 15, 2021
@tseaver
Copy link
Contributor Author

tseaver commented Jul 16, 2021

#392 applies the third strategy (to __hash__).

@tseaver tseaver mentioned this issue Jul 16, 2021
Closed
@crwilcox
Copy link
Contributor

crwilcox commented Sep 9, 2021

I think I agree with changing the underlying hashing. Though #392 has some test issues currently that need addressing.

tseaver added a commit that referenced this issue Oct 4, 2021
@tseaver
fix: hash snapshots using correct type for 'update_time'
af5a175 
Document that 'DocumentSnapshot.create_time' and
'DocumentSnapshot.update_time' are instances of
'proto.datetime_helpers.DatetimeWithNanoseconds'.

Closes #398.
@tseaver tseaver mentioned this issue Oct 4, 2021
Merged
tseaver added a commit that referenced this issue Oct 8, 2021
@tseaver
fix: hash snapshots using correct type for 'update_time' (#467)
5e66a73 
Document that 'DocumentSnapshot.create_time' and
'DocumentSnapshot.update_time' are instances of
'proto.datetime_helpers.DatetimeWithNanoseconds'.

Closes #398.
Closes #391.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
api: firestore Issues related to the googleapis/python-firestore API. type: question Request for information or clarification. Not an issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: hash snapshots using correct type for 'update_time' googleapis/python-firestore
2 participants
@tseaver @crwilcox