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.

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

Port Firestore Document to C++ #777

Merged
merged 20 commits into from
Feb 13, 2018
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions Firestore/core/src/firebase/firestore/model/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,22 @@ cc_library(
base_path.h
database_id.cc
database_id.h
document.cc
document.h
document_key.cc
document_key.h
field_path.cc
field_path.h
field_value.cc
field_value.h
snapshot_version.cc
snapshot_version.h
maybe_document.cc
maybe_document.h
no_document.cc
no_document.h
resource_path.cc
resource_path.h
snapshot_version.cc
snapshot_version.h
timestamp.cc
timestamp.h
DEPENDS
Expand Down
51 changes: 51 additions & 0 deletions Firestore/core/src/firebase/firestore/model/document.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* Copyright 2018 Google
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include "Firestore/core/src/firebase/firestore/model/document.h"

#include <utility>

#include "Firestore/core/src/firebase/firestore/util/firebase_assert.h"

namespace firebase {
namespace firestore {
namespace model {

Document::Document(const FieldValue& data,
const DocumentKey& key,
const SnapshotVersion& version,
bool has_local_mutations)
: MaybeDocument(key, version),
data_(data),
has_local_mutations_(has_local_mutations) {
set_type(Type::Document);
FIREBASE_ASSERT(FieldValue::Type::Object == data.type());
}

Document::Document(FieldValue&& data,
const DocumentKey& key,
const SnapshotVersion& version,
bool has_local_mutations)
: MaybeDocument(key, version),
data_(std::move(data)),
has_local_mutations_(has_local_mutations) {
set_type(Type::Document);
FIREBASE_ASSERT(FieldValue::Type::Object == data.type());
}

} // namespace model
} // namespace firestore
} // namespace firebase
71 changes: 71 additions & 0 deletions Firestore/core/src/firebase/firestore/model/document.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/*
* Copyright 2018 Google
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_DOCUMENT_H_
#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_DOCUMENT_H_

#include "Firestore/core/src/firebase/firestore/model/field_value.h"
#include "Firestore/core/src/firebase/firestore/model/maybe_document.h"

namespace firebase {
namespace firestore {
namespace model {

/**
* Represents a document in Firestore with a key, version, data and whether the
* data has local mutations applied to it.
*/
class Document : public MaybeDocument {
public:
Document(const FieldValue& data,
const DocumentKey& key,
const SnapshotVersion& version,
bool has_local_mutations);

Document(FieldValue&& data,
const DocumentKey& key,
const SnapshotVersion& version,
bool has_local_mutations);

const FieldValue& data() const {
return data_;
}

bool has_local_mutations() const {
return has_local_mutations_;
}

private:
FieldValue data_; // This is of type Object.
bool has_local_mutations_;
};

/** Compares against another Document. */
inline bool operator==(const Document& lhs, const Document& rhs) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than duplicating some of the comparisons from the parent class, you could simply call the parent class' op==() and then additionally check stuff from the child class. This would make you future proof should additional fields be added to the parent class.

This assumes that for two Document objects to be equal, their MaybeDocument parent instances are also equal. (Which curiously, isn't quite true here; but see below.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, that's something I want to discuss last week's API review but didn't have time. MaybeDocument (and thus all subclasses) has comparator, mainly for inequality (<) check, which is purely based on DocumentKey. On the other hand, Document has equality check (==) that actually means to check each properties.

return lhs.version() == rhs.version() && lhs.key() == rhs.key() &&
lhs.has_local_mutations() == rhs.has_local_mutations() &&
lhs.data() == rhs.data();
}

inline bool operator!=(const Document& lhs, const Document& rhs) {
return !(lhs == rhs);
}

} // namespace model
} // namespace firestore
} // namespace firebase

#endif // FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_DOCUMENT_H_
30 changes: 30 additions & 0 deletions Firestore/core/src/firebase/firestore/model/maybe_document.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright 2018 Google
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include "Firestore/core/src/firebase/firestore/model/maybe_document.h"

namespace firebase {
namespace firestore {
namespace model {

MaybeDocument::MaybeDocument(const DocumentKey& key,
const SnapshotVersion& version)
: type_(Type::Unknown), key_(key), version_(version) {
}

} // namespace model
} // namespace firestore
} // namespace firebase
94 changes: 94 additions & 0 deletions Firestore/core/src/firebase/firestore/model/maybe_document.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
/*
* Copyright 2018 Google
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_MAYBE_DOCUMENT_H_
#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_MAYBE_DOCUMENT_H_

#include "Firestore/core/src/firebase/firestore/model/document_key.h"
#include "Firestore/core/src/firebase/firestore/model/snapshot_version.h"

namespace firebase {
namespace firestore {
namespace model {

/**
* The result of a lookup for a given path may be an existing document or a
* tombstone that marks the path deleted.
*/
class MaybeDocument {
public:
enum class Type {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This deserves a comment. At the very least a porting note on why it exists here but not in the other clients.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Unknown,
Document,
NoDocument,
};

MaybeDocument(const DocumentKey& key, const SnapshotVersion& version);

Type type() const {
return type_;
}

const DocumentKey& key() const {
return key_;
}

const SnapshotVersion& version() const {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth taking a look through the other clients when doing these ports and seeing if there any things we missed in the Objective-C port.

In this case there are some useful comments in android.

/**
 * Returns the version of this document if it exists or a version at which this document was
 * guaranteed to not exist.
 */

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return version_;
}

protected:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could probably make these private. (Or at least key and version; though style guide suggests they should all be private.)

http://go/cpp-style#Access_Control

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

// Only allow subclass to set their types.
void set_type(Type type) {
type_ = type;
}

private:
Type type_;
DocumentKey key_;
SnapshotVersion version_;
};

/** Compares against another MaybeDocument. */
inline bool operator<(const MaybeDocument& lhs, const MaybeDocument& rhs) {
return lhs.key() < rhs.key();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're not comparing type or version. Is that on purpose?

Since version/type are user visible, they should (usually) be included in op<, opt==, etc. (For DocSnap/QuerySnap, we're looking to not include metadata. Perhaps that applies here too? If so, we should add some docs to make this clear as it's otherwise unexpected. OTOH, perhaps just comparing the key is an optimization? i.e. if the keys are the same, then the other fields must be the same? If so, call that out in a comment.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me sync with SFO folks to see what is the design of the comparator.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in chat, we should only define equality for these documents but they should not be comparable by default.

There exists an FSTDocumentKeyComparator but this should be equivalent to an comparison functor (similar to std::less) and shouldn't result in MaybeDocuments being directly comparable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}

inline bool operator>(const MaybeDocument& lhs, const MaybeDocument& rhs) {
return lhs.key() > rhs.key();
}

inline bool operator>=(const MaybeDocument& lhs, const MaybeDocument& rhs) {
return lhs.key() >= rhs.key();
}

inline bool operator<=(const MaybeDocument& lhs, const MaybeDocument& rhs) {
return lhs.key() <= rhs.key();
}

inline bool operator!=(const MaybeDocument& lhs, const MaybeDocument& rhs) {
return lhs.key() != rhs.key() || lhs.type() != rhs.type();
}

inline bool operator==(const MaybeDocument& lhs, const MaybeDocument& rhs) {
return lhs.key() == rhs.key() && lhs.type() == rhs.type();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're not comparing 'version'. Is that on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the objc comparison is purely key-based; but let me sync with folks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be

return lhs.type() == rhs.type() && lhs.equal(rhs);

Where equal is defined here as

private:
 virtual bool equal(const MaybeDocument& lhs, const MaybeDocument& rhs) = 0;

Specific implementations can assume that lhs and rhs are properly the correct type already (and can just static cast).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}

} // namespace model
} // namespace firestore
} // namespace firebase

#endif // FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_MAYBE_DOCUMENT_H_
30 changes: 30 additions & 0 deletions Firestore/core/src/firebase/firestore/model/no_document.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright 2018 Google
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include "Firestore/core/src/firebase/firestore/model/no_document.h"

namespace firebase {
namespace firestore {
namespace model {

NoDocument::NoDocument(const DocumentKey& key, const SnapshotVersion& version)
: MaybeDocument(key, version) {
set_type(Type::NoDocument);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should type just be a constructor parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}

} // namespace model
} // namespace firestore
} // namespace firebase
36 changes: 36 additions & 0 deletions Firestore/core/src/firebase/firestore/model/no_document.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* Copyright 2018 Google
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_NO_DOCUMENT_H_
#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_NO_DOCUMENT_H_

#include "Firestore/core/src/firebase/firestore/model/maybe_document.h"

namespace firebase {
namespace firestore {
namespace model {

/** Represents that no documents exists for the key at the given version. */
class NoDocument : public MaybeDocument {
public:
NoDocument(const DocumentKey& key, const SnapshotVersion& version);
};

} // namespace model
} // namespace firestore
} // namespace firebase

#endif // FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_NO_DOCUMENT_H_
5 changes: 4 additions & 1 deletion Firestore/core/test/firebase/firestore/model/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,14 @@ cc_test(
SOURCES
database_id_test.cc
document_key_test.cc
document_test.cc
field_path_test.cc
field_value_test.cc
maybe_document_test.cc
no_document_test.cc
resource_path_test.cc
snapshot_version_test.cc
timestamp_test.cc
resource_path_test.cc
DEPENDS
firebase_firestore_model
)
Loading