Document/MutableDocument changes #7996
Conversation
Generated by 🚫 Danger |
Firestore/core/src/model/document.h
Outdated
| @@ -17,125 +17,57 @@ | |||
| #ifndef FIRESTORE_CORE_SRC_MODEL_DOCUMENT_H_ | |||
| #define FIRESTORE_CORE_SRC_MODEL_DOCUMENT_H_ | |||
|
|
|||
| #include <iosfwd> | |||
| #include <memory> | |||
| #include <iostream> | |||
There was a problem hiding this comment.
<iostream> is a relatively heavy-weight header, so if <iosfwd> suffices, it's better to use <iosfwd>. It looks like you can do that if you move the definition of operator<< into the source file.
There was a problem hiding this comment.
I did not address this yet as this would require me to create document.cc and I am not sure if that tradeoff is worth making.
There was a problem hiding this comment.
I don't think there's any harm in creating a .cc file, even if it's just for one function.
Firestore/core/src/model/document.h
Outdated
| * data has local mutations applied to it. | ||
| */ | ||
| class Document : public MaybeDocument { | ||
| /** Represents a non-mutable document in Firestore. */ |
There was a problem hiding this comment.
Ultranit: s/non-mutable/immutable/?
| value_{std::move(value)}, | ||
| document_state_{document_state} { | ||
| } | ||
|
|
||
| DocumentKey key_; | ||
| DocumentType document_type_ = DocumentType::kInvalid; | ||
| SnapshotVersion version_; | ||
| ObjectValue value_; | ||
| std::shared_ptr<const ObjectValue> value_ = std::make_shared<ObjectValue>(); |
There was a problem hiding this comment.
Nit: std::make_shared<const ObjectValue>()?
| @@ -17,17 +17,19 @@ | |||
| #ifndef FIRESTORE_CORE_SRC_MODEL_MUTABLE_DOCUMENT_H_ | |||
| #define FIRESTORE_CORE_SRC_MODEL_MUTABLE_DOCUMENT_H_ | |||
|
|
|||
| #include <memory> | |||
| #include <ostream> | |||
There was a problem hiding this comment.
Note: this file could probably replace <ostream> with <iosfwd> as well.
| value_{std::move(value)}, | ||
| document_state_{document_state} { | ||
| } | ||
|
|
||
| DocumentKey key_; | ||
| DocumentType document_type_ = DocumentType::kInvalid; | ||
| SnapshotVersion version_; | ||
| ObjectValue value_; | ||
| std::shared_ptr<const ObjectValue> value_ = std::make_shared<ObjectValue>(); |
There was a problem hiding this comment.
Can you add a comment to explain the rationale of making this a shared pointer?
| std::string MutableDocument::ToString() const { | ||
| std::stringstream stream; | ||
| stream << "MutableDocument(key=" << key_ << ", type=" << document_type_ | ||
| << ", version=" << version_ << ", value=" << value_ | ||
| << ", version=" << version_ << ", value=" << CanonicalId(value_->Get()) |
There was a problem hiding this comment.
Question: is it necessary to call CanonicalId and Get rather than just do << *value_?
| #include <sstream> | ||
| #include "Firestore/core/src/model/mutable_document.h" |
There was a problem hiding this comment.
Nits:
- the
mutable_document.hinclude should be first (it's the related header), i.e. undo the change; - there has to be a blank line in-between the last standard header and the first Google header (also a blank line in-between the related header and the first standard header).
See go/cstyle#Names_and_Order_of_Includes.
| @@ -63,7 +59,7 @@ MutableDocument& MutableDocument::ConvertToNoDocument( | |||
| const SnapshotVersion& version) { | |||
| version_ = version; | |||
| document_type_ = DocumentType::kNoDocument; | |||
| value_ = {}; | |||
| value_ = std::make_shared<ObjectValue>(); | |||
There was a problem hiding this comment.
You want to make sure value_ is never null to avoid checking it before each access, right?
There was a problem hiding this comment.
Yes, otherwise I have to dynamically create a new value before each access.
| @@ -212,7 +212,10 @@ class MutableDocument { | |||
| DocumentKey key_; | |||
| DocumentType document_type_ = DocumentType::kInvalid; | |||
| SnapshotVersion version_; | |||
| std::shared_ptr<const ObjectValue> value_ = std::make_shared<ObjectValue>(); | |||
| // Using a shared pointer to ObjectValue makes MutableDocument copy-assignable | |||
| // without having to manually create a deep clone of its Protobuf contents | |||
There was a problem hiding this comment.
Ultranit: please add a full stop at the end of the sentence.
This is part of #7904. It will break the feature branch, but I am planning to send out small reviewable chunks with an end goal of a feature branch that passes CI.
This PR changes MutableDocument to be copy-assignable (since ObjectValue is getting changed to be backed by a SharedMessage). It also introduces Document, as a wrapper for MutableDocuments that can no longer be mutated. These are used once documents become part of a view.