From 433fec1da6816c5fd03e448b2faf66bf6645e08c Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Sun, 17 Mar 2019 02:26:29 -0700 Subject: [PATCH 01/11] Replace FSTDocumentSet's guts with immutable::SortedSet. --- .../Tests/Model/FSTDocumentSetTests.mm | 23 ++++-- Firestore/Source/Core/FSTView.mm | 2 +- Firestore/Source/Model/FSTDocumentSet.h | 25 +++++- Firestore/Source/Model/FSTDocumentSet.mm | 76 +++++++++++-------- .../firebase/firestore/api/query_snapshot.mm | 2 +- .../firebase/firestore/core/view_snapshot.mm | 2 +- .../firebase/firestore/immutable/sorted_map.h | 2 +- .../firebase/firestore/immutable/sorted_set.h | 2 +- 8 files changed, 91 insertions(+), 43 deletions(-) diff --git a/Firestore/Example/Tests/Model/FSTDocumentSetTests.mm b/Firestore/Example/Tests/Model/FSTDocumentSetTests.mm index 05be1b7eeab..c6bb9812329 100644 --- a/Firestore/Example/Tests/Model/FSTDocumentSetTests.mm +++ b/Firestore/Example/Tests/Model/FSTDocumentSetTests.mm @@ -18,6 +18,8 @@ #import +#include + #import "Firestore/Source/Model/FSTDocument.h" #import "Firestore/Example/Tests/Util/FSTHelpers.h" @@ -76,21 +78,21 @@ - (void)testFirstAndLastDocument { - (void)testKeepsDocumentsInTheRightOrder { FSTDocumentSet *set = FSTTestDocSet(_comp, @[ _doc1, _doc2, _doc3 ]); - XCTAssertEqualObjects([[set documentEnumerator] allObjects], (@[ _doc3, _doc1, _doc2 ])); + XCTAssertEqual([self allDocuments:set], (std::vector{_doc3, _doc1, _doc2})); } - (void)testDeletes { FSTDocumentSet *set = FSTTestDocSet(_comp, @[ _doc1, _doc2, _doc3 ]); FSTDocumentSet *setWithoutDoc1 = [set documentSetByRemovingKey:_doc1.key]; - XCTAssertEqualObjects([[setWithoutDoc1 documentEnumerator] allObjects], (@[ _doc3, _doc2 ])); + XCTAssertEqual([self allDocuments:setWithoutDoc1], (std::vector{_doc3, _doc2})); XCTAssertEqual([setWithoutDoc1 count], 2); // Original remains unchanged - XCTAssertEqualObjects([[set documentEnumerator] allObjects], (@[ _doc3, _doc1, _doc2 ])); + XCTAssertEqual([self allDocuments:set], (std::vector{_doc3, _doc1, _doc2})); FSTDocumentSet *setWithoutDoc3 = [setWithoutDoc1 documentSetByRemovingKey:_doc3.key]; - XCTAssertEqualObjects([[setWithoutDoc3 documentEnumerator] allObjects], (@[ _doc2 ])); + XCTAssertEqual([self allDocuments:setWithoutDoc3], (std::vector{_doc2})); XCTAssertEqual([setWithoutDoc3 count], 1); } @@ -102,14 +104,14 @@ - (void)testUpdates { set = [set documentSetByAddingDocument:doc2Prime]; XCTAssertEqual([set count], 3); XCTAssertEqualObjects([set documentForKey:doc2Prime.key], doc2Prime); - XCTAssertEqualObjects([[set documentEnumerator] allObjects], (@[ _doc3, _doc1, doc2Prime ])); + XCTAssertEqual([self allDocuments:set], (std::vector{_doc3, _doc1, doc2Prime})); } - (void)testAddsDocsWithEqualComparisonValues { FSTDocument *doc4 = FSTTestDoc("docs/4", 0, @{@"sort" : @2}, FSTDocumentStateSynced); FSTDocumentSet *set = FSTTestDocSet(_comp, @[ _doc1, doc4 ]); - XCTAssertEqualObjects([[set documentEnumerator] allObjects], (@[ _doc1, doc4 ])); + XCTAssertEqual([self allDocuments:set], (std::vector{_doc1, doc4})); } - (void)testIsEqual { @@ -129,6 +131,15 @@ - (void)testIsEqual { XCTAssertNotEqualObjects(set1, shortSet); XCTAssertNotEqualObjects(set1, sortedSet1); } + +- (std::vector)allDocuments:(FSTDocumentSet *)documents { + std::vector result; + for (FSTDocument *doc : documents.documents) { + result.push_back(doc); + } + return result; +} + @end NS_ASSUME_NONNULL_END diff --git a/Firestore/Source/Core/FSTView.mm b/Firestore/Source/Core/FSTView.mm index f839a5682b8..687dddfe4d9 100644 --- a/Firestore/Source/Core/FSTView.mm +++ b/Firestore/Source/Core/FSTView.mm @@ -489,7 +489,7 @@ - (void)applyTargetChange:(const absl::optional &)maybeTargetChang // TODO(klimt): Do this incrementally so that it's not quadratic when updating many documents. DocumentKeySet oldLimboDocuments = std::move(_limboDocuments); _limboDocuments = DocumentKeySet{}; - for (FSTDocument *doc in self.documentSet.documentEnumerator) { + for (FSTDocument *doc : self.documentSet.documents) { if ([self shouldBeLimboDocumentKey:doc.key]) { _limboDocuments = _limboDocuments.insert(doc.key); } diff --git a/Firestore/Source/Model/FSTDocumentSet.h b/Firestore/Source/Model/FSTDocumentSet.h index 2087a3da59a..862f779f824 100644 --- a/Firestore/Source/Model/FSTDocumentSet.h +++ b/Firestore/Source/Model/FSTDocumentSet.h @@ -16,6 +16,7 @@ #import +#include "Firestore/core/src/firebase/firestore/immutable/sorted_set.h" #include "Firestore/core/src/firebase/firestore/model/document_key.h" #include "Firestore/core/src/firebase/firestore/model/document_map.h" @@ -23,6 +24,27 @@ NS_ASSUME_NONNULL_BEGIN +namespace firebase { +namespace firestore { +namespace model { + +class DocumentSetComparator { + public: + explicit DocumentSetComparator(NSComparator delegate = nil) : delegate_(delegate) { + } + + bool operator()(FSTDocument *lhs, FSTDocument *rhs) const { + return delegate_(lhs, rhs) == NSOrderedAscending; + } + + private: + NSComparator delegate_; +}; + +} // namespace model +} // namespace firestore +} // namespace firebase + /** * DocumentSet is an immutable (copy-on-write) collection that holds documents in order specified * by the provided comparator. We always add a document key comparator on top of what is provided @@ -64,7 +86,8 @@ NS_ASSUME_NONNULL_BEGIN */ - (NSUInteger)indexOfKey:(const firebase::firestore::model::DocumentKey &)key; -- (NSEnumerator *)documentEnumerator; +- (const firebase::firestore::immutable:: + SortedSet &)documents; /** Returns a copy of the documents in this set as an array. This is O(n) on the size of the set. */ - (NSArray *)arrayValue; diff --git a/Firestore/Source/Model/FSTDocumentSet.mm b/Firestore/Source/Model/FSTDocumentSet.mm index c87e2039e54..5908541552f 100644 --- a/Firestore/Source/Model/FSTDocumentSet.mm +++ b/Firestore/Source/Model/FSTDocumentSet.mm @@ -21,33 +21,42 @@ #import "Firestore/Source/Model/FSTDocument.h" #import "Firestore/third_party/Immutable/FSTImmutableSortedSet.h" +#include "Firestore/core/src/firebase/firestore/immutable/sorted_set.h" #include "Firestore/core/src/firebase/firestore/model/document_key.h" +#include "Firestore/core/src/firebase/firestore/util/objc_compatibility.h" +#include "Firestore/core/src/firebase/firestore/util/range.h" +namespace objc = firebase::firestore::util::objc; +using firebase::firestore::immutable::SortedSet; using firebase::firestore::model::DocumentMap; using firebase::firestore::model::DocumentKey; +using firebase::firestore::util::range; NS_ASSUME_NONNULL_BEGIN +using firebase::firestore::model::DocumentSetComparator; + /** * The type of the main collection of documents in an FSTDocumentSet. * @see FSTDocumentSet#sortedSet */ -typedef FSTImmutableSortedSet SetType; +using SetType = SortedSet; @interface FSTDocumentSet () - (instancetype)initWithIndex:(DocumentMap &&)index - set:(SetType *)sortedSet NS_DESIGNATED_INITIALIZER; + set:(SetType &&)sortedSet NS_DESIGNATED_INITIALIZER; -/** - * The main collection of documents in the FSTDocumentSet. The documents are ordered by a - * comparator supplied from a query. The SetType collection exists in addition to the index to - * allow ordered traversal of the FSTDocumentSet. - */ -@property(nonatomic, strong, readonly) SetType *sortedSet; @end @implementation FSTDocumentSet { + /** + * The main collection of documents in the FSTDocumentSet. The documents are ordered by a + * comparator supplied from a query. The SetType collection exists in addition to the index to + * allow ordered traversal of the FSTDocumentSet. + */ + SetType _sortedSet; + /** * An index of the documents in the FSTDocumentSet, indexed by document key. The index * exists to guarantee the uniqueness of document keys in the set and to allow lookup and removal @@ -57,15 +66,15 @@ @implementation FSTDocumentSet { } + (instancetype)documentSetWithComparator:(NSComparator)comparator { - SetType *set = [FSTImmutableSortedSet setWithComparator:comparator]; - return [[FSTDocumentSet alloc] initWithIndex:DocumentMap {} set:set]; + SetType set{DocumentSetComparator(comparator)}; + return [[FSTDocumentSet alloc] initWithIndex:DocumentMap {} set:std::move(set)]; } -- (instancetype)initWithIndex:(DocumentMap &&)index set:(SetType *)sortedSet { +- (instancetype)initWithIndex:(DocumentMap &&)index set:(SetType &&)sortedSet { self = [super init]; if (self) { _index = std::move(index); - _sortedSet = sortedSet; + _sortedSet = std::move(sortedSet); } return self; } @@ -83,31 +92,34 @@ - (BOOL)isEqual:(id)other { return NO; } - NSEnumerator *selfIter = [self.sortedSet objectEnumerator]; - NSEnumerator *otherIter = [otherSet.sortedSet objectEnumerator]; + SetType::const_iterator selfIter = _sortedSet.begin(); + SetType::const_iterator selfEnd = _sortedSet.end(); + + SetType::const_iterator otherIter = otherSet->_sortedSet.begin(); + SetType::const_iterator otherEnd = otherSet->_sortedSet.end(); - FSTDocument *selfDoc = [selfIter nextObject]; - FSTDocument *otherDoc = [otherIter nextObject]; - while (selfDoc) { + while (selfIter != selfEnd && otherIter != otherEnd) { + FSTDocument *selfDoc = *selfIter; + FSTDocument *otherDoc = *otherIter; if (![selfDoc isEqual:otherDoc]) { return NO; } - selfDoc = [selfIter nextObject]; - otherDoc = [otherIter nextObject]; + ++selfIter; + ++otherIter; } return YES; } - (NSUInteger)hash { NSUInteger hash = 0; - for (FSTDocument *doc in self.sortedSet.objectEnumerator) { + for (FSTDocument *doc : _sortedSet) { hash = 31 * hash + [doc hash]; } return hash; } - (NSString *)description { - return [self.sortedSet description]; + return objc::Description(_sortedSet); } - (NSUInteger)count { @@ -128,25 +140,27 @@ - (FSTDocument *_Nullable)documentForKey:(const DocumentKey &)key { } - (FSTDocument *_Nullable)firstDocument { - return [self.sortedSet firstObject]; + auto result = _sortedSet.min(); + return result != _sortedSet.end() ? *result : nil; } - (FSTDocument *_Nullable)lastDocument { - return [self.sortedSet lastObject]; + auto result = _sortedSet.max(); + return result != _sortedSet.end() ? *result : nil; } - (NSUInteger)indexOfKey:(const DocumentKey &)key { FSTDocument *doc = [self documentForKey:key]; - return doc ? [self.sortedSet indexOfObject:doc] : NSNotFound; + return doc ? _sortedSet.find_index(doc) : NSNotFound; } -- (NSEnumerator *)documentEnumerator { - return [self.sortedSet objectEnumerator]; +- (const SetType &)documents { + return _sortedSet; } - (NSArray *)arrayValue { NSMutableArray *result = [NSMutableArray arrayWithCapacity:self.count]; - for (FSTDocument *doc in self.documentEnumerator) { + for (FSTDocument *doc : _sortedSet) { [result addObject:doc]; } return result; @@ -167,8 +181,8 @@ - (instancetype)documentSetByAddingDocument:(FSTDocument *_Nullable)document { FSTDocumentSet *removed = [self documentSetByRemovingKey:document.key]; DocumentMap index = removed->_index.insert(document.key, document); - SetType *set = [removed.sortedSet setByAddingObject:document]; - return [[FSTDocumentSet alloc] initWithIndex:std::move(index) set:set]; + SetType set = removed->_sortedSet.insert(document); + return [[FSTDocumentSet alloc] initWithIndex:std::move(index) set:std::move(set)]; } - (instancetype)documentSetByRemovingKey:(const DocumentKey &)key { @@ -178,8 +192,8 @@ - (instancetype)documentSetByRemovingKey:(const DocumentKey &)key { } DocumentMap index = _index.erase(key); - SetType *set = [self.sortedSet setByRemovingObject:doc]; - return [[FSTDocumentSet alloc] initWithIndex:std::move(index) set:set]; + SetType set = _sortedSet.erase(doc); + return [[FSTDocumentSet alloc] initWithIndex:std::move(index) set:std::move(set)]; } @end diff --git a/Firestore/core/src/firebase/firestore/api/query_snapshot.mm b/Firestore/core/src/firebase/firestore/api/query_snapshot.mm index f3f9302d89f..a4b217b1c12 100644 --- a/Firestore/core/src/firebase/firestore/api/query_snapshot.mm +++ b/Firestore/core/src/firebase/firestore/api/query_snapshot.mm @@ -55,7 +55,7 @@ FSTDocumentSet* documentSet = snapshot_.documents(); bool from_cache = metadata_.from_cache(); - for (FSTDocument* document in documentSet.documentEnumerator) { + for (FSTDocument* document : documentSet.documents) { bool has_pending_writes = snapshot_.mutated_keys().contains(document.key); DocumentSnapshot snap(firestore_, document.key, document, from_cache, has_pending_writes); diff --git a/Firestore/core/src/firebase/firestore/core/view_snapshot.mm b/Firestore/core/src/firebase/firestore/core/view_snapshot.mm index 66e58fddd94..772df6806d2 100644 --- a/Firestore/core/src/firebase/firestore/core/view_snapshot.mm +++ b/Firestore/core/src/firebase/firestore/core/view_snapshot.mm @@ -158,7 +158,7 @@ bool from_cache, bool excludes_metadata_changes) { std::vector view_changes; - for (FSTDocument* doc in documents.documentEnumerator) { + for (FSTDocument* doc : documents.documents) { view_changes.emplace_back(doc, DocumentViewChange::Type::kAdded); } diff --git a/Firestore/core/src/firebase/firestore/immutable/sorted_map.h b/Firestore/core/src/firebase/firestore/immutable/sorted_map.h index 7960f6588d3..dc50bbd2bc1 100644 --- a/Firestore/core/src/firebase/firestore/immutable/sorted_map.h +++ b/Firestore/core/src/firebase/firestore/immutable/sorted_map.h @@ -204,7 +204,7 @@ class SortedMap : public impl::SortedMapBase { tree_type result = tree_.erase(key); if (result.empty()) { // Flip back to the array representation for empty arrays. - return SortedMap{}; + return SortedMap{comparator()}; } return SortedMap{std::move(result)}; } diff --git a/Firestore/core/src/firebase/firestore/immutable/sorted_set.h b/Firestore/core/src/firebase/firestore/immutable/sorted_set.h index 0b64a167836..47e3f9cccf5 100644 --- a/Firestore/core/src/firebase/firestore/immutable/sorted_set.h +++ b/Firestore/core/src/firebase/firestore/immutable/sorted_set.h @@ -101,7 +101,7 @@ class SortedSet { return const_iterator{map_.min()}; } - const K& max() const { + const_iterator max() const { return const_iterator{map_.max()}; } From 83d61c722b886757507506c6600b72c088090aa5 Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Sun, 17 Mar 2019 03:02:08 -0700 Subject: [PATCH 02/11] Port FSTDocumentSet to C++ DocumentSet. --- .../firebase/firestore/model/document_set.h | 178 ++++++++++++++++++ .../firebase/firestore/model/document_set.mm | 150 +++++++++++++++ 2 files changed, 328 insertions(+) create mode 100644 Firestore/core/src/firebase/firestore/model/document_set.h create mode 100644 Firestore/core/src/firebase/firestore/model/document_set.mm diff --git a/Firestore/core/src/firebase/firestore/model/document_set.h b/Firestore/core/src/firebase/firestore/model/document_set.h new file mode 100644 index 00000000000..e966d32485b --- /dev/null +++ b/Firestore/core/src/firebase/firestore/model/document_set.h @@ -0,0 +1,178 @@ +/* + * Copyright 2017 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_SET_H_ +#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_DOCUMENT_SET_H_ + +#import + +#include +#include +#include + +#include "Firestore/core/src/firebase/firestore/immutable/sorted_set.h" +#include "Firestore/core/src/firebase/firestore/model/document_key.h" +#include "Firestore/core/src/firebase/firestore/model/document_map.h" + +@class FSTDocument; + +NS_ASSUME_NONNULL_BEGIN + +namespace firebase { +namespace firestore { +namespace model { + +/** + * A C++ comparator that returns less-than, implemented by delegating to + * an NSComparator. + */ +class DocumentSetComparator { + public: + explicit DocumentSetComparator(NSComparator delegate = nil) + : delegate_(delegate) { + } + + bool operator()(FSTDocument* lhs, FSTDocument* rhs) const { + return delegate_(lhs, rhs) == NSOrderedAscending; + } + + private: + NSComparator delegate_; +}; + +/** + * DocumentSet is an immutable (copy-on-write) collection that holds documents + * in order specified by the provided comparator. We always add a document key + * comparator on top of what is provided to guarantee document equality based on + * the key. + */ +class DocumentSet { + public: + /** + * The type of the main collection of documents in an DocumentSet. + * @see sorted_set_. + */ + using SetType = immutable::SortedSet; + + // STL container types + using value_type = FSTDocument*; + using const_iterator = SetType::const_iterator; + + /** + * Creates a new, empty DocumentSet sorted by the given comparator, then by + * keys. + */ + explicit DocumentSet(NSComparator comparator); + + size_t size() const { + return index_.size(); + } + + /** Returns true if the dictionary contains no elements. */ + bool empty() const { + return index_.empty(); + } + + /** Returns true if this set contains a document with the given key. */ + bool ContainsKey(const DocumentKey& key) const; + + SetType::const_iterator begin() const { + return sorted_set_.begin(); + } + SetType::const_iterator end() const { + return sorted_set_.end(); + } + + /** + * Returns the document from this set with the given key if it exists or nil + * if it doesn't. + */ + FSTDocument* _Nullable GetDocument(const DocumentKey& key) const; + + /** + * Returns the first document in the set according to its built in ordering, + * or nil if the set is empty. + */ + FSTDocument* _Nullable GetFirstDocument() const; + + /** + * Returns the last document in the set according to its built in ordering, or + * nil if the set is empty. + */ + FSTDocument* _Nullable GetLastDocument() const; + + /** + * Returns the index of the document with the provided key in the document + * set. Returns NSNotFound if the key is not present. + */ + size_t IndexOf(const DocumentKey& key) const; + + /** + * Returns a copy of the documents in this set as an array. This is O(n) on + * the size of the set. + */ + NSArray* GetArrayValue() const; + + /** + * Returns the documents as a `DocumentMap`. This is O(1) as this leverages + * our internal representation. + */ + const DocumentMap& GetMapValue() const; + + /** Returns a new DocumentSet that contains the given document. */ + DocumentSet insert(FSTDocument* _Nullable document) const; + + /** + * Returns a new DocumentSet that excludes any document associated with + * the given key. + */ + DocumentSet erase(const DocumentKey& key) const; + + friend bool operator==(const DocumentSet& lhs, const DocumentSet& rhs); + + std::string ToString() const; + friend std::ostream& operator<<(std::ostream& os, const DocumentSet& set); + + size_t Hash() const; + + private: + DocumentSet(DocumentMap&& index, SetType&& sorted_set) + : index_(std::move(index)), sorted_set_(std::move(sorted_set)) { + } + + /** + * An index of the documents in the DocumentSet, indexed by document key. + * The index exists to guarantee the uniqueness of document keys in the set + * and to allow lookup and removal of documents by key. + */ + DocumentMap index_; + + /** + * The main collection of documents in the DocumentSet. The documents are + * ordered by a comparator supplied from a query. The SetType collection + * exists in addition to the index to allow ordered traversal of the + * DocumentSet. + */ + SetType sorted_set_; +}; + +} // namespace model +} // namespace firestore +} // namespace firebase + +NS_ASSUME_NONNULL_END + +#endif // FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_DOCUMENT_SET_H_ diff --git a/Firestore/core/src/firebase/firestore/model/document_set.mm b/Firestore/core/src/firebase/firestore/model/document_set.mm new file mode 100644 index 00000000000..565b2cc5534 --- /dev/null +++ b/Firestore/core/src/firebase/firestore/model/document_set.mm @@ -0,0 +1,150 @@ +/* + * Copyright 2017 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_set.h" + +#include +#include + +#import "Firestore/Source/Model/FSTDocument.h" + +#include "Firestore/core/src/firebase/firestore/immutable/sorted_set.h" +#include "Firestore/core/src/firebase/firestore/model/document_key.h" +#include "Firestore/core/src/firebase/firestore/util/objc_compatibility.h" +#include "Firestore/core/src/firebase/firestore/util/range.h" + +NS_ASSUME_NONNULL_BEGIN + +namespace firebase { +namespace firestore { +namespace model { + +namespace objc = util::objc; +using immutable::SortedSet; + +DocumentSet::DocumentSet(NSComparator comparator) + : index_{}, sorted_set_{DocumentSetComparator{comparator}} { +} + +bool operator==(const DocumentSet& lhs, const DocumentSet& rhs) { + if (lhs.size() != rhs.size()) { + return false; + } + + auto left_iter = lhs.sorted_set_.begin(); + auto left_end = lhs.sorted_set_.end(); + + auto right_iter = rhs.sorted_set_.begin(); + auto right_end = rhs.sorted_set_.end(); + + while (left_iter != left_end && right_iter != right_end) { + FSTDocument* left_doc = *left_iter; + FSTDocument* right_doc = *right_iter; + if (![left_doc isEqual:right_doc]) { + return NO; + } + ++left_iter; + ++right_iter; + } + return YES; +} + +std::string DocumentSet::ToString() const { + return util::ToString(sorted_set_); +} + +std::ostream& operator<<(std::ostream& os, const DocumentSet& set) { + return os << set.ToString(); +} + +size_t DocumentSet::Hash() const { + size_t hash = 0; + for (FSTDocument* doc : sorted_set_) { + hash = 31 * hash + [doc hash]; + } + return hash; +} + +bool DocumentSet::ContainsKey(const DocumentKey& key) const { + return index_.underlying_map().find(key) != index_.underlying_map().end(); +} + +FSTDocument* _Nullable DocumentSet::GetDocument(const DocumentKey& key) const { + auto found = index_.underlying_map().find(key); + return found != index_.underlying_map().end() + ? static_cast(found->second) + : nil; +} + +FSTDocument* _Nullable DocumentSet::GetFirstDocument() const { + auto result = sorted_set_.min(); + return result != sorted_set_.end() ? *result : nil; +} + +FSTDocument* _Nullable DocumentSet::GetLastDocument() const { + auto result = sorted_set_.max(); + return result != sorted_set_.end() ? *result : nil; +} + +size_t DocumentSet::IndexOf(const DocumentKey& key) const { + FSTDocument* doc = GetDocument(key); + return doc ? sorted_set_.find_index(doc) : NSNotFound; +} + +NSArray* DocumentSet::GetArrayValue() const { + NSMutableArray* result = + [NSMutableArray arrayWithCapacity:size()]; + for (FSTDocument* doc : sorted_set_) { + [result addObject:doc]; + } + return result; +} + +const DocumentMap& DocumentSet::GetMapValue() const { + return index_; +} + +DocumentSet DocumentSet::insert(FSTDocument* _Nullable document) const { + // TODO(mcg): look into making document nonnull. + if (!document) { + return *this; + } + + // Remove any prior mapping of the document's key before adding, preventing + // sortedSet from accumulating values that aren't in the index. + DocumentSet removed = erase(document.key); + + DocumentMap index = removed.index_.insert(document.key, document); + SetType set = removed.sorted_set_.insert(document); + return {std::move(index), std::move(set)}; +} + +DocumentSet DocumentSet::erase(const DocumentKey& key) const { + FSTDocument* doc = GetDocument(key); + if (!doc) { + return *this; + } + + DocumentMap index = index_.erase(key); + SetType set = sorted_set_.erase(doc); + return {std::move(index), std::move(set)}; +} + +} // namespace model +} // namespace firestore +} // namespace firebase + +NS_ASSUME_NONNULL_END From 5666ce52d9917300ce5f2313c7a43cd28ab2ff51 Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Thu, 21 Mar 2019 14:55:40 -0700 Subject: [PATCH 03/11] Replace implementation of FSTDocumentSet with C++ DocumentSet --- .../Tests/Model/FSTDocumentSetTests.mm | 32 +++-- Firestore/Source/Model/FSTDocumentSet.h | 31 +---- Firestore/Source/Model/FSTDocumentSet.mm | 130 ++++-------------- .../firebase/firestore/immutable/sorted_set.h | 2 +- .../firebase/firestore/model/document_set.h | 2 +- 5 files changed, 57 insertions(+), 140 deletions(-) diff --git a/Firestore/Example/Tests/Model/FSTDocumentSetTests.mm b/Firestore/Example/Tests/Model/FSTDocumentSetTests.mm index c6bb9812329..05a3a50ef68 100644 --- a/Firestore/Example/Tests/Model/FSTDocumentSetTests.mm +++ b/Firestore/Example/Tests/Model/FSTDocumentSetTests.mm @@ -22,10 +22,22 @@ #import "Firestore/Source/Model/FSTDocument.h" +#include "Firestore/core/test/firebase/firestore/testutil/xcgmock.h" + #import "Firestore/Example/Tests/Util/FSTHelpers.h" +using testing::ElementsAre; + NS_ASSUME_NONNULL_BEGIN +std::vector Documents(FSTDocumentSet *documents) { + std::vector result; + for (FSTDocument *doc : documents.documents) { + result.push_back(doc); + } + return result; +} + @interface FSTDocumentSetTests : XCTestCase @end @@ -78,21 +90,21 @@ - (void)testFirstAndLastDocument { - (void)testKeepsDocumentsInTheRightOrder { FSTDocumentSet *set = FSTTestDocSet(_comp, @[ _doc1, _doc2, _doc3 ]); - XCTAssertEqual([self allDocuments:set], (std::vector{_doc3, _doc1, _doc2})); + XC_ASSERT_THAT(Documents(set), ElementsAre(_doc3, _doc1, _doc2)); } - (void)testDeletes { FSTDocumentSet *set = FSTTestDocSet(_comp, @[ _doc1, _doc2, _doc3 ]); FSTDocumentSet *setWithoutDoc1 = [set documentSetByRemovingKey:_doc1.key]; - XCTAssertEqual([self allDocuments:setWithoutDoc1], (std::vector{_doc3, _doc2})); + XC_ASSERT_THAT(Documents(setWithoutDoc1), ElementsAre(_doc3, _doc2)); XCTAssertEqual([setWithoutDoc1 count], 2); // Original remains unchanged - XCTAssertEqual([self allDocuments:set], (std::vector{_doc3, _doc1, _doc2})); + XC_ASSERT_THAT(Documents(set), ElementsAre(_doc3, _doc1, _doc2)); FSTDocumentSet *setWithoutDoc3 = [setWithoutDoc1 documentSetByRemovingKey:_doc3.key]; - XCTAssertEqual([self allDocuments:setWithoutDoc3], (std::vector{_doc2})); + XC_ASSERT_THAT(Documents(setWithoutDoc3), ElementsAre(_doc2)); XCTAssertEqual([setWithoutDoc3 count], 1); } @@ -104,14 +116,14 @@ - (void)testUpdates { set = [set documentSetByAddingDocument:doc2Prime]; XCTAssertEqual([set count], 3); XCTAssertEqualObjects([set documentForKey:doc2Prime.key], doc2Prime); - XCTAssertEqual([self allDocuments:set], (std::vector{_doc3, _doc1, doc2Prime})); + XC_ASSERT_THAT(Documents(set), ElementsAre(_doc3, _doc1, doc2Prime)); } - (void)testAddsDocsWithEqualComparisonValues { FSTDocument *doc4 = FSTTestDoc("docs/4", 0, @{@"sort" : @2}, FSTDocumentStateSynced); FSTDocumentSet *set = FSTTestDocSet(_comp, @[ _doc1, doc4 ]); - XCTAssertEqual([self allDocuments:set], (std::vector{_doc1, doc4})); + XC_ASSERT_THAT(Documents(set), ElementsAre(_doc1, doc4)); } - (void)testIsEqual { @@ -132,14 +144,6 @@ - (void)testIsEqual { XCTAssertNotEqualObjects(set1, sortedSet1); } -- (std::vector)allDocuments:(FSTDocumentSet *)documents { - std::vector result; - for (FSTDocument *doc : documents.documents) { - result.push_back(doc); - } - return result; -} - @end NS_ASSUME_NONNULL_END diff --git a/Firestore/Source/Model/FSTDocumentSet.h b/Firestore/Source/Model/FSTDocumentSet.h index 862f779f824..4da8b8f6282 100644 --- a/Firestore/Source/Model/FSTDocumentSet.h +++ b/Firestore/Source/Model/FSTDocumentSet.h @@ -19,31 +19,13 @@ #include "Firestore/core/src/firebase/firestore/immutable/sorted_set.h" #include "Firestore/core/src/firebase/firestore/model/document_key.h" #include "Firestore/core/src/firebase/firestore/model/document_map.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" @class FSTDocument; -NS_ASSUME_NONNULL_BEGIN - -namespace firebase { -namespace firestore { -namespace model { - -class DocumentSetComparator { - public: - explicit DocumentSetComparator(NSComparator delegate = nil) : delegate_(delegate) { - } - - bool operator()(FSTDocument *lhs, FSTDocument *rhs) const { - return delegate_(lhs, rhs) == NSOrderedAscending; - } +using firebase::firestore::model::DocumentSet; - private: - NSComparator delegate_; -}; - -} // namespace model -} // namespace firestore -} // namespace firebase +NS_ASSUME_NONNULL_BEGIN /** * DocumentSet is an immutable (copy-on-write) collection that holds documents in order specified @@ -55,7 +37,9 @@ class DocumentSetComparator { /** Creates a new, empty FSTDocumentSet sorted by the given comparator, then by keys. */ + (instancetype)documentSetWithComparator:(NSComparator)comparator; -- (instancetype)init __attribute__((unavailable("Use a static constructor instead"))); +- (instancetype)initWithDocumentSet:(DocumentSet &&)documentSet NS_DESIGNATED_INITIALIZER; + +- (instancetype)init NS_UNAVAILABLE; - (NSUInteger)count; @@ -86,8 +70,7 @@ class DocumentSetComparator { */ - (NSUInteger)indexOfKey:(const firebase::firestore::model::DocumentKey &)key; -- (const firebase::firestore::immutable:: - SortedSet &)documents; +- (const DocumentSet &)documents; /** Returns a copy of the documents in this set as an array. This is O(n) on the size of the set. */ - (NSArray *)arrayValue; diff --git a/Firestore/Source/Model/FSTDocumentSet.mm b/Firestore/Source/Model/FSTDocumentSet.mm index 5908541552f..64e2843b085 100644 --- a/Firestore/Source/Model/FSTDocumentSet.mm +++ b/Firestore/Source/Model/FSTDocumentSet.mm @@ -23,177 +23,107 @@ #include "Firestore/core/src/firebase/firestore/immutable/sorted_set.h" #include "Firestore/core/src/firebase/firestore/model/document_key.h" +#include "Firestore/core/src/firebase/firestore/util/delayed_constructor.h" #include "Firestore/core/src/firebase/firestore/util/objc_compatibility.h" -#include "Firestore/core/src/firebase/firestore/util/range.h" +#include "Firestore/core/src/firebase/firestore/util/string_apple.h" namespace objc = firebase::firestore::util::objc; +namespace util = firebase::firestore::util; using firebase::firestore::immutable::SortedSet; using firebase::firestore::model::DocumentMap; using firebase::firestore::model::DocumentKey; -using firebase::firestore::util::range; +using firebase::firestore::util::DelayedConstructor; NS_ASSUME_NONNULL_BEGIN using firebase::firestore::model::DocumentSetComparator; -/** - * The type of the main collection of documents in an FSTDocumentSet. - * @see FSTDocumentSet#sortedSet - */ -using SetType = SortedSet; - -@interface FSTDocumentSet () - -- (instancetype)initWithIndex:(DocumentMap &&)index - set:(SetType &&)sortedSet NS_DESIGNATED_INITIALIZER; - -@end - @implementation FSTDocumentSet { - /** - * The main collection of documents in the FSTDocumentSet. The documents are ordered by a - * comparator supplied from a query. The SetType collection exists in addition to the index to - * allow ordered traversal of the FSTDocumentSet. - */ - SetType _sortedSet; - - /** - * An index of the documents in the FSTDocumentSet, indexed by document key. The index - * exists to guarantee the uniqueness of document keys in the set and to allow lookup and removal - * of documents by key. - */ - DocumentMap _index; + DelayedConstructor _delegate; } + (instancetype)documentSetWithComparator:(NSComparator)comparator { - SetType set{DocumentSetComparator(comparator)}; - return [[FSTDocumentSet alloc] initWithIndex:DocumentMap {} set:std::move(set)]; + DocumentSet wrapped{comparator}; + return [[FSTDocumentSet alloc] initWithDocumentSet:std::move(wrapped)]; } -- (instancetype)initWithIndex:(DocumentMap &&)index set:(SetType &&)sortedSet { +- (instancetype)initWithDocumentSet:(DocumentSet &&)documentSet { self = [super init]; if (self) { - _index = std::move(index); - _sortedSet = std::move(sortedSet); + _delegate.Init(std::move(documentSet)); } return self; } - (BOOL)isEqual:(id)other { - if (other == self) { - return YES; - } - if (![other isMemberOfClass:[FSTDocumentSet class]]) { - return NO; - } + if (other == self) return YES; + if (![other isMemberOfClass:[FSTDocumentSet class]]) return NO; FSTDocumentSet *otherSet = (FSTDocumentSet *)other; - if ([self count] != [otherSet count]) { - return NO; - } - - SetType::const_iterator selfIter = _sortedSet.begin(); - SetType::const_iterator selfEnd = _sortedSet.end(); - - SetType::const_iterator otherIter = otherSet->_sortedSet.begin(); - SetType::const_iterator otherEnd = otherSet->_sortedSet.end(); - - while (selfIter != selfEnd && otherIter != otherEnd) { - FSTDocument *selfDoc = *selfIter; - FSTDocument *otherDoc = *otherIter; - if (![selfDoc isEqual:otherDoc]) { - return NO; - } - ++selfIter; - ++otherIter; - } - return YES; + return *_delegate == *(otherSet->_delegate); } - (NSUInteger)hash { - NSUInteger hash = 0; - for (FSTDocument *doc : _sortedSet) { - hash = 31 * hash + [doc hash]; - } - return hash; + return _delegate->Hash(); } - (NSString *)description { - return objc::Description(_sortedSet); + return util::WrapNSString(_delegate->ToString()); } - (NSUInteger)count { - return _index.size(); + return _delegate->size(); } - (BOOL)isEmpty { - return _index.empty(); + return _delegate->empty(); } - (BOOL)containsKey:(const DocumentKey &)key { - return _index.underlying_map().find(key) != _index.underlying_map().end(); + return _delegate->ContainsKey(key); } - (FSTDocument *_Nullable)documentForKey:(const DocumentKey &)key { - auto found = _index.underlying_map().find(key); - return found != _index.underlying_map().end() ? static_cast(found->second) : nil; + return _delegate->GetDocument(key); } - (FSTDocument *_Nullable)firstDocument { - auto result = _sortedSet.min(); - return result != _sortedSet.end() ? *result : nil; + return _delegate->GetFirstDocument(); } - (FSTDocument *_Nullable)lastDocument { - auto result = _sortedSet.max(); - return result != _sortedSet.end() ? *result : nil; + return _delegate->GetLastDocument(); } - (NSUInteger)indexOfKey:(const DocumentKey &)key { - FSTDocument *doc = [self documentForKey:key]; - return doc ? _sortedSet.find_index(doc) : NSNotFound; + size_t index = _delegate->IndexOf(key); + return index != DocumentSet::npos ? index : NSNotFound; } -- (const SetType &)documents { - return _sortedSet; +- (const DocumentSet &)documents { + return *_delegate; } - (NSArray *)arrayValue { NSMutableArray *result = [NSMutableArray arrayWithCapacity:self.count]; - for (FSTDocument *doc : _sortedSet) { + for (FSTDocument *doc : *_delegate) { [result addObject:doc]; } return result; } - (const DocumentMap &)mapValue { - return _index; + return _delegate->GetMapValue(); } - (instancetype)documentSetByAddingDocument:(FSTDocument *_Nullable)document { - // TODO(mcg): look into making document nonnull. - if (!document) { - return self; - } - - // Remove any prior mapping of the document's key before adding, preventing sortedSet from - // accumulating values that aren't in the index. - FSTDocumentSet *removed = [self documentSetByRemovingKey:document.key]; - - DocumentMap index = removed->_index.insert(document.key, document); - SetType set = removed->_sortedSet.insert(document); - return [[FSTDocumentSet alloc] initWithIndex:std::move(index) set:std::move(set)]; + DocumentSet result = _delegate->insert(document); + return [[FSTDocumentSet alloc] initWithDocumentSet:std::move(result)]; } - (instancetype)documentSetByRemovingKey:(const DocumentKey &)key { - FSTDocument *doc = [self documentForKey:key]; - if (!doc) { - return self; - } - - DocumentMap index = _index.erase(key); - SetType set = _sortedSet.erase(doc); - return [[FSTDocumentSet alloc] initWithIndex:std::move(index) set:std::move(set)]; + DocumentSet result = _delegate->erase(key); + return [[FSTDocumentSet alloc] initWithDocumentSet:std::move(result)]; } @end diff --git a/Firestore/core/src/firebase/firestore/immutable/sorted_set.h b/Firestore/core/src/firebase/firestore/immutable/sorted_set.h index 47e3f9cccf5..b055e914dcf 100644 --- a/Firestore/core/src/firebase/firestore/immutable/sorted_set.h +++ b/Firestore/core/src/firebase/firestore/immutable/sorted_set.h @@ -46,7 +46,7 @@ template , typename V = impl::Empty, typename M = SortedMap> -class SortedSet { +class SortedSet : public impl::SortedMapBase { public: using size_type = typename M::size_type; using value_type = K; diff --git a/Firestore/core/src/firebase/firestore/model/document_set.h b/Firestore/core/src/firebase/firestore/model/document_set.h index e966d32485b..8a600402e18 100644 --- a/Firestore/core/src/firebase/firestore/model/document_set.h +++ b/Firestore/core/src/firebase/firestore/model/document_set.h @@ -59,7 +59,7 @@ class DocumentSetComparator { * comparator on top of what is provided to guarantee document equality based on * the key. */ -class DocumentSet { +class DocumentSet : public immutable::impl::SortedMapBase { public: /** * The type of the main collection of documents in an DocumentSet. From 83c0687501d7006e9dc514ce6a259d603a1ea94c Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Sun, 17 Mar 2019 13:07:45 -0700 Subject: [PATCH 04/11] Update all usages of FSTDocumentSet --- .../Tests/API/FIRQuerySnapshotTests.mm | 7 +- Firestore/Example/Tests/API/FSTAPIHelpers.mm | 17 ++- .../Tests/Core/FSTEventManagerTests.mm | 5 +- .../Tests/Core/FSTQueryListenerTests.mm | 88 +++++++------- .../Example/Tests/Core/FSTViewSnapshotTest.mm | 10 +- Firestore/Example/Tests/Core/FSTViewTests.mm | 79 ++++++++----- .../Example/Tests/Local/FSTLocalStoreTests.mm | 2 +- .../Tests/Model/FSTDocumentSetTests.mm | 111 ++++++++---------- Firestore/Example/Tests/Util/FSTHelpers.h | 7 +- Firestore/Example/Tests/Util/FSTHelpers.mm | 9 +- Firestore/Source/API/FIRDocumentChange.mm | 21 ++-- Firestore/Source/API/FIRDocumentReference.mm | 2 +- .../Source/API/FIRQuerySnapshot+Internal.h | 1 - Firestore/Source/API/FIRQuerySnapshot.mm | 30 ++--- Firestore/Source/Core/FSTEventManager.mm | 4 +- Firestore/Source/Core/FSTFirestoreClient.mm | 2 +- Firestore/Source/Core/FSTSyncEngine.mm | 2 +- Firestore/Source/Core/FSTView.h | 3 +- Firestore/Source/Core/FSTView.mm | 58 +++++---- Firestore/Source/Local/FSTLocalViewChanges.h | 1 - .../firestore/api/document_reference.mm | 6 +- .../firebase/firestore/api/query_snapshot.h | 7 +- .../firebase/firestore/api/query_snapshot.mm | 7 +- .../firebase/firestore/core/view_snapshot.h | 16 +-- .../firebase/firestore/core/view_snapshot.mm | 34 +++--- .../firebase/firestore/model/document_set.h | 7 +- .../firebase/firestore/model/document_set.mm | 10 +- 27 files changed, 281 insertions(+), 265 deletions(-) diff --git a/Firestore/Example/Tests/API/FIRQuerySnapshotTests.mm b/Firestore/Example/Tests/API/FIRQuerySnapshotTests.mm index edec5367b56..9a45e8c388c 100644 --- a/Firestore/Example/Tests/API/FIRQuerySnapshotTests.mm +++ b/Firestore/Example/Tests/API/FIRQuerySnapshotTests.mm @@ -29,15 +29,16 @@ #import "Firestore/Source/API/FIRQuerySnapshot+Internal.h" #import "Firestore/Source/API/FIRSnapshotMetadata+Internal.h" #import "Firestore/Source/Model/FSTDocument.h" -#import "Firestore/Source/Model/FSTDocumentSet.h" #include "Firestore/core/src/firebase/firestore/core/view_snapshot.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" #include "Firestore/core/src/firebase/firestore/util/string_apple.h" namespace util = firebase::firestore::util; using firebase::firestore::core::DocumentViewChange; using firebase::firestore::core::ViewSnapshot; using firebase::firestore::model::DocumentKeySet; +using firebase::firestore::model::DocumentSet; NS_ASSUME_NONNULL_BEGIN @@ -87,8 +88,8 @@ - (void)testIncludeMetadataChanges { FSTDocument *doc2Old = FSTTestDoc("foo/baz", 1, @{@"a" : @"b"}, FSTDocumentStateSynced); FSTDocument *doc2New = FSTTestDoc("foo/baz", 1, @{@"a" : @"c"}, FSTDocumentStateSynced); - FSTDocumentSet *oldDocuments = FSTTestDocSet(FSTDocumentComparatorByKey, @[ doc1Old, doc2Old ]); - FSTDocumentSet *newDocuments = FSTTestDocSet(FSTDocumentComparatorByKey, @[ doc2New, doc2New ]); + DocumentSet oldDocuments = FSTTestDocSet(FSTDocumentComparatorByKey, @[ doc1Old, doc2Old ]); + DocumentSet newDocuments = FSTTestDocSet(FSTDocumentComparatorByKey, @[ doc2New, doc2New ]); std::vector documentChanges{ DocumentViewChange{doc1New, DocumentViewChange::Type::kMetadata}, DocumentViewChange{doc2New, DocumentViewChange::Type::kModified}, diff --git a/Firestore/Example/Tests/API/FSTAPIHelpers.mm b/Firestore/Example/Tests/API/FSTAPIHelpers.mm index bcab27f7bc4..ee9f4d15cb0 100644 --- a/Firestore/Example/Tests/API/FSTAPIHelpers.mm +++ b/Firestore/Example/Tests/API/FSTAPIHelpers.mm @@ -32,9 +32,9 @@ #import "Firestore/Source/API/FIRSnapshotMetadata+Internal.h" #import "Firestore/Source/Core/FSTQuery.h" #import "Firestore/Source/Model/FSTDocument.h" -#import "Firestore/Source/Model/FSTDocumentSet.h" #include "Firestore/core/src/firebase/firestore/core/view_snapshot.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" #include "Firestore/core/src/firebase/firestore/util/string_apple.h" #include "Firestore/core/test/firebase/firestore/testutil/testutil.h" @@ -43,6 +43,7 @@ using firebase::firestore::core::DocumentViewChange; using firebase::firestore::core::ViewSnapshot; using firebase::firestore::model::DocumentKeySet; +using firebase::firestore::model::DocumentSet; NS_ASSUME_NONNULL_BEGIN @@ -98,26 +99,24 @@ bool hasPendingWrites, bool fromCache) { SnapshotMetadata metadata(hasPendingWrites, fromCache); - FSTDocumentSet *oldDocuments = FSTTestDocSet(FSTDocumentComparatorByKey, @[]); + DocumentSet oldDocuments = FSTTestDocSet(FSTDocumentComparatorByKey, @[]); DocumentKeySet mutatedKeys; for (NSString *key in oldDocs) { - oldDocuments = [oldDocuments - documentSetByAddingDocument:FSTTestDoc(util::StringFormat("%s/%s", path, key), 1, - oldDocs[key], - hasPendingWrites ? FSTDocumentStateLocalMutations - : FSTDocumentStateSynced)]; + oldDocuments = oldDocuments.insert( + FSTTestDoc(util::StringFormat("%s/%s", path, key), 1, oldDocs[key], + hasPendingWrites ? FSTDocumentStateLocalMutations : FSTDocumentStateSynced)); if (hasPendingWrites) { const std::string documentKey = util::StringFormat("%s/%s", path, key); mutatedKeys = mutatedKeys.insert(testutil::Key(documentKey)); } } - FSTDocumentSet *newDocuments = oldDocuments; + DocumentSet newDocuments = oldDocuments; std::vector documentChanges; for (NSString *key in docsToAdd) { FSTDocument *docToAdd = FSTTestDoc(util::StringFormat("%s/%s", path, key), 1, docsToAdd[key], hasPendingWrites ? FSTDocumentStateLocalMutations : FSTDocumentStateSynced); - newDocuments = [newDocuments documentSetByAddingDocument:docToAdd]; + newDocuments = newDocuments.insert(docToAdd); documentChanges.emplace_back(docToAdd, DocumentViewChange::Type::kAdded); if (hasPendingWrites) { const std::string documentKey = util::StringFormat("%s/%s", path, key); diff --git a/Firestore/Example/Tests/Core/FSTEventManagerTests.mm b/Firestore/Example/Tests/Core/FSTEventManagerTests.mm index 11152bd5035..98cfe536bc4 100644 --- a/Firestore/Example/Tests/Core/FSTEventManagerTests.mm +++ b/Firestore/Example/Tests/Core/FSTEventManagerTests.mm @@ -23,18 +23,19 @@ #import "Firestore/Source/Core/FSTQuery.h" #import "Firestore/Source/Core/FSTSyncEngine.h" -#import "Firestore/Source/Model/FSTDocumentSet.h" #import "Firestore/Example/Tests/Util/FSTHelpers.h" #include "Firestore/core/src/firebase/firestore/core/view_snapshot.h" #include "Firestore/core/src/firebase/firestore/model/document_key_set.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" #include "Firestore/core/src/firebase/firestore/model/types.h" #include "Firestore/core/src/firebase/firestore/util/statusor.h" using firebase::firestore::core::ViewSnapshot; using firebase::firestore::core::ViewSnapshotHandler; using firebase::firestore::model::DocumentKeySet; +using firebase::firestore::model::DocumentSet; using firebase::firestore::model::OnlineState; using firebase::firestore::util::StatusOr; @@ -106,7 +107,7 @@ - (FSTQueryListener *)queryListenerForQuery:(FSTQuery *)query } - (ViewSnapshot)makeEmptyViewSnapshotWithQuery:(FSTQuery *)query { - FSTDocumentSet *emptyDocs = [FSTDocumentSet documentSetWithComparator:query.comparator]; + DocumentSet emptyDocs{query.comparator}; // sync_state_changed has to be `true` to prevent an assertion about a meaningless view snapshot. return ViewSnapshot{ query, emptyDocs, emptyDocs, {}, DocumentKeySet{}, false, /*sync_state_changed=*/true, false}; diff --git a/Firestore/Example/Tests/Core/FSTQueryListenerTests.mm b/Firestore/Example/Tests/Core/FSTQueryListenerTests.mm index 15ebfcaf995..7516a8aaf05 100644 --- a/Firestore/Example/Tests/Core/FSTQueryListenerTests.mm +++ b/Firestore/Example/Tests/Core/FSTQueryListenerTests.mm @@ -24,13 +24,13 @@ #import "Firestore/Source/Core/FSTQuery.h" #import "Firestore/Source/Core/FSTView.h" #import "Firestore/Source/Model/FSTDocument.h" -#import "Firestore/Source/Model/FSTDocumentSet.h" #import "Firestore/Source/Util/FSTAsyncQueryListener.h" #import "Firestore/Example/Tests/Util/FSTHelpers.h" #include "Firestore/core/include/firebase/firestore/firestore_errors.h" #include "Firestore/core/src/firebase/firestore/core/view_snapshot.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" #include "Firestore/core/src/firebase/firestore/model/types.h" #include "Firestore/core/src/firebase/firestore/remote/remote_event.h" #include "Firestore/core/src/firebase/firestore/util/delayed_constructor.h" @@ -44,6 +44,7 @@ using firebase::firestore::core::ViewSnapshot; using firebase::firestore::core::ViewSnapshotHandler; using firebase::firestore::model::DocumentKeySet; +using firebase::firestore::model::DocumentSet; using firebase::firestore::model::OnlineState; using firebase::firestore::remote::TargetChange; using firebase::firestore::util::DelayedConstructor; @@ -119,15 +120,14 @@ - (void)testRaisesCollectionEvents { XC_ASSERT_THAT(accum[0].document_changes(), ElementsAre(change1, change2)); XC_ASSERT_THAT(accum[1].document_changes(), ElementsAre(change3)); - ViewSnapshot expectedSnap2{ - snap2.query(), - snap2.documents(), - /*old_documents=*/[FSTDocumentSet documentSetWithComparator : snap2.query().comparator], - /*document_changes=*/{ change1, change4 }, - snap2.mutated_keys(), - snap2.from_cache(), - /*sync_state_changed=*/true, - /*excludes_metadata_changes=*/true}; + ViewSnapshot expectedSnap2{snap2.query(), + snap2.documents(), + /*old_documents=*/DocumentSet{snap2.query().comparator}, + /*document_changes=*/{change1, change4}, + snap2.mutated_keys(), + snap2.from_cache(), + /*sync_state_changed=*/true, + /*excludes_metadata_changes=*/true}; XC_ASSERT_THAT(otherAccum, ElementsAre(expectedSnap2)); } @@ -395,15 +395,14 @@ - (void)testWillWaitForSyncIfOnline { DocumentViewChange change1{doc1, DocumentViewChange::Type::kAdded}; DocumentViewChange change2{doc2, DocumentViewChange::Type::kAdded}; - ViewSnapshot expectedSnap{ - snap3.query(), - snap3.documents(), - /*old_documents=*/[FSTDocumentSet documentSetWithComparator : snap3.query().comparator], - /*document_changes=*/{ change1, change2 }, - snap3.mutated_keys(), - /*from_cache=*/false, - /*sync_state_changed=*/true, - /*excludes_metadata_changes=*/true}; + ViewSnapshot expectedSnap{snap3.query(), + snap3.documents(), + /*old_documents=*/DocumentSet{snap3.query().comparator}, + /*document_changes=*/{change1, change2}, + snap3.mutated_keys(), + /*from_cache=*/false, + /*sync_state_changed=*/true, + /*excludes_metadata_changes=*/true}; XC_ASSERT_THAT(events, ElementsAre(expectedSnap)); } @@ -433,15 +432,14 @@ - (void)testWillRaiseInitialEventWhenGoingOffline { DocumentViewChange change1{doc1, DocumentViewChange::Type::kAdded}; DocumentViewChange change2{doc2, DocumentViewChange::Type::kAdded}; - ViewSnapshot expectedSnap1{ - query, - /*documents=*/snap1.documents(), - /*old_documents=*/[FSTDocumentSet documentSetWithComparator : snap1.query().comparator], - /*document_changes=*/{ change1 }, - snap1.mutated_keys(), - /*from_cache=*/true, - /*sync_state_changed=*/true, - /*excludes_metadata_changes=*/true}; + ViewSnapshot expectedSnap1{query, + /*documents=*/snap1.documents(), + /*old_documents=*/DocumentSet{snap1.query().comparator}, + /*document_changes=*/{change1}, + snap1.mutated_keys(), + /*from_cache=*/true, + /*sync_state_changed=*/true, + /*excludes_metadata_changes=*/true}; ViewSnapshot expectedSnap2{query, /*documents=*/snap2.documents(), @@ -469,15 +467,14 @@ - (void)testWillRaiseInitialEventWhenGoingOfflineAndThereAreNoDocs { [listener queryDidChangeViewSnapshot:snap1]; // no event [listener applyChangedOnlineState:OnlineState::Offline]; // event - ViewSnapshot expectedSnap{ - query, - /*documents=*/snap1.documents(), - /*old_documents=*/[FSTDocumentSet documentSetWithComparator : snap1.query().comparator], - /*document_changes=*/{}, - snap1.mutated_keys(), - /*from_cache=*/true, - /*sync_state_changed=*/true, - /*excludes_metadata_changes=*/true}; + ViewSnapshot expectedSnap{query, + /*documents=*/snap1.documents(), + /*old_documents=*/DocumentSet{snap1.query().comparator}, + /*document_changes=*/{}, + snap1.mutated_keys(), + /*from_cache=*/true, + /*sync_state_changed=*/true, + /*excludes_metadata_changes=*/true}; XC_ASSERT_THAT(events, ElementsAre(expectedSnap)); } @@ -495,15 +492,14 @@ - (void)testWillRaiseInitialEventWhenStartingOfflineAndThereAreNoDocs { [listener applyChangedOnlineState:OnlineState::Offline]; // no event [listener queryDidChangeViewSnapshot:snap1]; // event - ViewSnapshot expectedSnap{ - query, - /*documents=*/snap1.documents(), - /*old_documents=*/[FSTDocumentSet documentSetWithComparator : snap1.query().comparator], - /*document_changes=*/{}, - snap1.mutated_keys(), - /*from_cache=*/true, - /*sync_state_changed=*/true, - /*excludes_metadata_changes=*/true}; + ViewSnapshot expectedSnap{query, + /*documents=*/snap1.documents(), + /*old_documents=*/DocumentSet{snap1.query().comparator}, + /*document_changes=*/{}, + snap1.mutated_keys(), + /*from_cache=*/true, + /*sync_state_changed=*/true, + /*excludes_metadata_changes=*/true}; XC_ASSERT_THAT(events, ElementsAre(expectedSnap)); } diff --git a/Firestore/Example/Tests/Core/FSTViewSnapshotTest.mm b/Firestore/Example/Tests/Core/FSTViewSnapshotTest.mm index 6dbdd805b4e..bd13b0ccccb 100644 --- a/Firestore/Example/Tests/Core/FSTViewSnapshotTest.mm +++ b/Firestore/Example/Tests/Core/FSTViewSnapshotTest.mm @@ -20,16 +20,17 @@ #import "Firestore/Source/Core/FSTQuery.h" #import "Firestore/Source/Model/FSTDocument.h" -#import "Firestore/Source/Model/FSTDocumentSet.h" #import "Firestore/Example/Tests/Util/FSTHelpers.h" #include "Firestore/core/src/firebase/firestore/core/view_snapshot.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" using firebase::firestore::core::DocumentViewChange; using firebase::firestore::core::DocumentViewChangeSet; using firebase::firestore::core::ViewSnapshot; using firebase::firestore::model::DocumentKeySet; +using firebase::firestore::model::DocumentSet; NS_ASSUME_NONNULL_BEGIN @@ -100,10 +101,9 @@ - (void)testTrack { - (void)testViewSnapshotConstructor { FSTQuery *query = FSTTestQuery("a"); - FSTDocumentSet *documents = [FSTDocumentSet documentSetWithComparator:FSTDocumentComparatorByKey]; - FSTDocumentSet *oldDocuments = documents; - documents = - [documents documentSetByAddingDocument:FSTTestDoc("c/a", 1, @{}, FSTDocumentStateSynced)]; + DocumentSet documents = DocumentSet{FSTDocumentComparatorByKey}; + DocumentSet oldDocuments = documents; + documents = documents.insert(FSTTestDoc("c/a", 1, @{}, FSTDocumentStateSynced)); std::vector documentChanges{DocumentViewChange{ FSTTestDoc("c/a", 1, @{}, FSTDocumentStateSynced), DocumentViewChange::Type::kAdded}}; diff --git a/Firestore/Example/Tests/Core/FSTViewTests.mm b/Firestore/Example/Tests/Core/FSTViewTests.mm index 2bd80aab592..4245d072e6f 100644 --- a/Firestore/Example/Tests/Core/FSTViewTests.mm +++ b/Firestore/Example/Tests/Core/FSTViewTests.mm @@ -18,18 +18,19 @@ #import +#include #include #include #import "Firestore/Source/API/FIRFirestore+Internal.h" #import "Firestore/Source/Core/FSTQuery.h" #import "Firestore/Source/Model/FSTDocument.h" -#import "Firestore/Source/Model/FSTDocumentSet.h" #import "Firestore/Source/Model/FSTFieldValue.h" #import "Firestore/Example/Tests/Util/FSTHelpers.h" #include "Firestore/core/src/firebase/firestore/core/view_snapshot.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" #include "Firestore/core/src/firebase/firestore/model/resource_path.h" #include "Firestore/core/test/firebase/firestore/testutil/testutil.h" #include "Firestore/core/test/firebase/firestore/testutil/xcgmock.h" @@ -40,10 +41,33 @@ using firebase::firestore::core::ViewSnapshot; using firebase::firestore::model::ResourcePath; using firebase::firestore::model::DocumentKeySet; +using firebase::firestore::model::DocumentSet; using testing::ElementsAre; NS_ASSUME_NONNULL_BEGIN +/** + * A custom matcher that verifies that the subject has the same keys as the given documents without + * verifying that the contents are the same. + */ +MATCHER_P(ContainsDocs, expected, "") { + if (expected.size() != arg.size()) { + return false; + } + for (FSTDocument *doc : expected) { + if (!arg.ContainsKey(doc.key)) { + return false; + } + } + return true; +} + +/** Constructs `ContainsDocs` instances with an initializer list. */ +inline ContainsDocsMatcherP> ContainsDocs( + std::vector docs) { + return ContainsDocsMatcherP>(docs); +} + @interface FSTViewTests : XCTestCase @end @@ -72,7 +96,7 @@ - (void)testAddsDocumentsBasedOnQuery { XCTAssertEqual(snapshot.query(), query); - XCTAssertEqualObjects(snapshot.documents().arrayValue, (@[ doc1, doc2 ])); + XC_ASSERT_THAT(snapshot.documents(), ElementsAre(doc1, doc2)); XCTAssertTrue(( snapshot.document_changes() == @@ -107,7 +131,7 @@ - (void)testRemovesDocuments { XCTAssertEqual(snapshot.query(), query); - XCTAssertEqualObjects(snapshot.documents().arrayValue, (@[ doc1, doc3 ])); + XC_ASSERT_THAT(snapshot.documents(), ElementsAre(doc1, doc3)); XCTAssertTrue(( snapshot.document_changes() == @@ -170,7 +194,7 @@ - (void)testFiltersDocumentsBasedOnQueryWithFilter { XCTAssertEqual(snapshot.query(), query); - XCTAssertEqualObjects(snapshot.documents().arrayValue, (@[ doc1, doc5, doc2 ])); + XC_ASSERT_THAT(snapshot.documents(), ElementsAre(doc1, doc5, doc2)); XCTAssertTrue(( snapshot.document_changes() == @@ -204,7 +228,7 @@ - (void)testUpdatesDocumentsBasedOnQueryWithFilter { XCTAssertEqual(snapshot.query(), query); - XCTAssertEqualObjects(snapshot.documents().arrayValue, (@[ doc1, doc3 ])); + XC_ASSERT_THAT(snapshot.documents(), ElementsAre(doc1, doc3)); FSTDocument *newDoc2 = FSTTestDoc("rooms/eros/messages/2", 1, @{@"sort" : @2}, FSTDocumentStateSynced); @@ -217,7 +241,7 @@ - (void)testUpdatesDocumentsBasedOnQueryWithFilter { XCTAssertEqual(snapshot.query(), query); - XCTAssertEqualObjects(snapshot.documents().arrayValue, (@[ newDoc4, doc1, newDoc2 ])); + XC_ASSERT_THAT(snapshot.documents(), ElementsAre(newDoc4, doc1, newDoc2)); XC_ASSERT_THAT(snapshot.document_changes(), ElementsAre(DocumentViewChange{doc3, DocumentViewChange::Type::kRemoved}, @@ -251,7 +275,7 @@ - (void)testRemovesDocumentsForQueryWithLimit { XCTAssertEqual(snapshot.query(), query); - XCTAssertEqualObjects(snapshot.documents().arrayValue, (@[ doc1, doc2 ])); + XC_ASSERT_THAT(snapshot.documents(), ElementsAre(doc1, doc2)); XCTAssertTrue(( snapshot.document_changes() == @@ -302,7 +326,7 @@ - (void)testDoesntReportChangesForDocumentBeyondLimitOfQuery { XCTAssertEqual(snapshot.query(), query); - XCTAssertEqualObjects(snapshot.documents().arrayValue, (@[ doc1, doc3 ])); + XC_ASSERT_THAT(snapshot.documents(), ElementsAre(doc1, doc3)); XC_ASSERT_THAT(snapshot.document_changes(), ElementsAre(DocumentViewChange{doc2, DocumentViewChange::Type::kRemoved}, @@ -372,13 +396,6 @@ - (void)testResumingQueryCreatesNoLimbos { XCTAssertEqualObjects(change.limboChanges, @[]); } -- (void)assertDocSet:(FSTDocumentSet *)docSet containsDocs:(NSArray *)docs { - XCTAssertEqual(docs.count, docSet.count); - for (FSTDocument *doc in docs) { - XCTAssertTrue([docSet containsKey:doc.key]); - } -} - - (void)testReturnsNeedsRefillOnDeleteInLimitQuery { FSTQuery *query = [[self queryForMessages] queryBySettingLimit:2]; FSTDocument *doc1 = FSTTestDoc("rooms/eros/messages/0", 0, @{}, FSTDocumentStateSynced); @@ -388,7 +405,7 @@ - (void)testReturnsNeedsRefillOnDeleteInLimitQuery { // Start with a full view. FSTViewDocumentChanges *changes = [view computeChangesWithDocuments:FSTTestDocUpdates(@[ doc1, doc2 ])]; - [self assertDocSet:changes.documentSet containsDocs:@[ doc1, doc2 ]]; + XC_ASSERT_THAT(changes.documentSet, ContainsDocs({doc1, doc2})); XCTAssertFalse(changes.needsRefill); XCTAssertEqual(2, changes.changeSet.GetChanges().size()); [view applyChangesToDocuments:changes]; @@ -396,12 +413,12 @@ - (void)testReturnsNeedsRefillOnDeleteInLimitQuery { // Remove one of the docs. changes = [view computeChangesWithDocuments:FSTTestDocUpdates(@[ FSTTestDeletedDoc( "rooms/eros/messages/0", 0, NO) ])]; - [self assertDocSet:changes.documentSet containsDocs:@[ doc2 ]]; + XC_ASSERT_THAT(changes.documentSet, ContainsDocs({doc2})); XCTAssertTrue(changes.needsRefill); XCTAssertEqual(1, changes.changeSet.GetChanges().size()); // Refill it with just the one doc remaining. changes = [view computeChangesWithDocuments:FSTTestDocUpdates(@[ doc2 ]) previousChanges:changes]; - [self assertDocSet:changes.documentSet containsDocs:@[ doc2 ]]; + XC_ASSERT_THAT(changes.documentSet, ContainsDocs({doc2})); XCTAssertFalse(changes.needsRefill); XCTAssertEqual(1, changes.changeSet.GetChanges().size()); [view applyChangesToDocuments:changes]; @@ -424,7 +441,7 @@ - (void)testReturnsNeedsRefillOnReorderInLimitQuery { // Start with a full view. FSTViewDocumentChanges *changes = [view computeChangesWithDocuments:FSTTestDocUpdates(@[ doc1, doc2, doc3 ])]; - [self assertDocSet:changes.documentSet containsDocs:@[ doc1, doc2 ]]; + XC_ASSERT_THAT(changes.documentSet, ContainsDocs({doc1, doc2})); XCTAssertFalse(changes.needsRefill); XCTAssertEqual(2, changes.changeSet.GetChanges().size()); [view applyChangesToDocuments:changes]; @@ -432,13 +449,13 @@ - (void)testReturnsNeedsRefillOnReorderInLimitQuery { // Move one of the docs. doc2 = FSTTestDoc("rooms/eros/messages/1", 1, @{@"order" : @2000}, FSTDocumentStateSynced); changes = [view computeChangesWithDocuments:FSTTestDocUpdates(@[ doc2 ])]; - [self assertDocSet:changes.documentSet containsDocs:@[ doc1, doc2 ]]; + XC_ASSERT_THAT(changes.documentSet, ContainsDocs({doc1, doc2})); XCTAssertTrue(changes.needsRefill); XCTAssertEqual(1, changes.changeSet.GetChanges().size()); // Refill it with all three current docs. changes = [view computeChangesWithDocuments:FSTTestDocUpdates(@[ doc1, doc2, doc3 ]) previousChanges:changes]; - [self assertDocSet:changes.documentSet containsDocs:@[ doc1, doc3 ]]; + XC_ASSERT_THAT(changes.documentSet, ContainsDocs({doc1, doc3})); XCTAssertFalse(changes.needsRefill); XCTAssertEqual(2, changes.changeSet.GetChanges().size()); [view applyChangesToDocuments:changes]; @@ -465,7 +482,7 @@ - (void)testDoesntNeedRefillOnReorderWithinLimit { // Start with a full view. FSTViewDocumentChanges *changes = [view computeChangesWithDocuments:FSTTestDocUpdates(@[ doc1, doc2, doc3, doc4, doc5 ])]; - [self assertDocSet:changes.documentSet containsDocs:@[ doc1, doc2, doc3 ]]; + XC_ASSERT_THAT(changes.documentSet, ContainsDocs({doc1, doc2, doc3})); XCTAssertFalse(changes.needsRefill); XCTAssertEqual(3, changes.changeSet.GetChanges().size()); [view applyChangesToDocuments:changes]; @@ -473,7 +490,7 @@ - (void)testDoesntNeedRefillOnReorderWithinLimit { // Move one of the docs. doc1 = FSTTestDoc("rooms/eros/messages/0", 1, @{@"order" : @3}, FSTDocumentStateSynced); changes = [view computeChangesWithDocuments:FSTTestDocUpdates(@[ doc1 ])]; - [self assertDocSet:changes.documentSet containsDocs:@[ doc2, doc3, doc1 ]]; + XC_ASSERT_THAT(changes.documentSet, ContainsDocs({doc2, doc3, doc1})); XCTAssertFalse(changes.needsRefill); XCTAssertEqual(1, changes.changeSet.GetChanges().size()); [view applyChangesToDocuments:changes]; @@ -500,7 +517,7 @@ - (void)testDoesntNeedRefillOnReorderAfterLimitQuery { // Start with a full view. FSTViewDocumentChanges *changes = [view computeChangesWithDocuments:FSTTestDocUpdates(@[ doc1, doc2, doc3, doc4, doc5 ])]; - [self assertDocSet:changes.documentSet containsDocs:@[ doc1, doc2, doc3 ]]; + XC_ASSERT_THAT(changes.documentSet, ContainsDocs({doc1, doc2, doc3})); XCTAssertFalse(changes.needsRefill); XCTAssertEqual(3, changes.changeSet.GetChanges().size()); [view applyChangesToDocuments:changes]; @@ -508,7 +525,7 @@ - (void)testDoesntNeedRefillOnReorderAfterLimitQuery { // Move one of the docs. doc4 = FSTTestDoc("rooms/eros/messages/3", 1, @{@"order" : @6}, FSTDocumentStateSynced); changes = [view computeChangesWithDocuments:FSTTestDocUpdates(@[ doc4 ])]; - [self assertDocSet:changes.documentSet containsDocs:@[ doc1, doc2, doc3 ]]; + XC_ASSERT_THAT(changes.documentSet, ContainsDocs({doc1, doc2, doc3})); XCTAssertFalse(changes.needsRefill); XCTAssertEqual(0, changes.changeSet.GetChanges().size()); [view applyChangesToDocuments:changes]; @@ -523,7 +540,7 @@ - (void)testDoesntNeedRefillForAdditionAfterTheLimit { // Start with a full view. FSTViewDocumentChanges *changes = [view computeChangesWithDocuments:FSTTestDocUpdates(@[ doc1, doc2 ])]; - [self assertDocSet:changes.documentSet containsDocs:@[ doc1, doc2 ]]; + XC_ASSERT_THAT(changes.documentSet, ContainsDocs({doc1, doc2})); XCTAssertFalse(changes.needsRefill); XCTAssertEqual(2, changes.changeSet.GetChanges().size()); [view applyChangesToDocuments:changes]; @@ -531,7 +548,7 @@ - (void)testDoesntNeedRefillForAdditionAfterTheLimit { // Add a doc that is past the limit. FSTDocument *doc3 = FSTTestDoc("rooms/eros/messages/2", 1, @{}, FSTDocumentStateSynced); changes = [view computeChangesWithDocuments:FSTTestDocUpdates(@[ doc3 ])]; - [self assertDocSet:changes.documentSet containsDocs:@[ doc1, doc2 ]]; + XC_ASSERT_THAT(changes.documentSet, ContainsDocs({doc1, doc2})); XCTAssertFalse(changes.needsRefill); XCTAssertEqual(0, changes.changeSet.GetChanges().size()); [view applyChangesToDocuments:changes]; @@ -545,7 +562,7 @@ - (void)testDoesntNeedRefillForDeletionsWhenNotNearTheLimit { FSTViewDocumentChanges *changes = [view computeChangesWithDocuments:FSTTestDocUpdates(@[ doc1, doc2 ])]; - [self assertDocSet:changes.documentSet containsDocs:@[ doc1, doc2 ]]; + XC_ASSERT_THAT(changes.documentSet, ContainsDocs({doc1, doc2})); XCTAssertFalse(changes.needsRefill); XCTAssertEqual(2, changes.changeSet.GetChanges().size()); [view applyChangesToDocuments:changes]; @@ -553,7 +570,7 @@ - (void)testDoesntNeedRefillForDeletionsWhenNotNearTheLimit { // Remove one of the docs. changes = [view computeChangesWithDocuments:FSTTestDocUpdates(@[ FSTTestDeletedDoc( "rooms/eros/messages/1", 0, NO) ])]; - [self assertDocSet:changes.documentSet containsDocs:@[ doc1 ]]; + XC_ASSERT_THAT(changes.documentSet, ContainsDocs({doc1})); XCTAssertFalse(changes.needsRefill); XCTAssertEqual(1, changes.changeSet.GetChanges().size()); [view applyChangesToDocuments:changes]; @@ -568,7 +585,7 @@ - (void)testHandlesApplyingIrrelevantDocs { // Start with a full view. FSTViewDocumentChanges *changes = [view computeChangesWithDocuments:FSTTestDocUpdates(@[ doc1, doc2 ])]; - [self assertDocSet:changes.documentSet containsDocs:@[ doc1, doc2 ]]; + XC_ASSERT_THAT(changes.documentSet, ContainsDocs({doc1, doc2})); XCTAssertFalse(changes.needsRefill); XCTAssertEqual(2, changes.changeSet.GetChanges().size()); [view applyChangesToDocuments:changes]; @@ -576,7 +593,7 @@ - (void)testHandlesApplyingIrrelevantDocs { // Remove a doc that isn't even in the results. changes = [view computeChangesWithDocuments:FSTTestDocUpdates(@[ FSTTestDeletedDoc( "rooms/eros/messages/2", 0, NO) ])]; - [self assertDocSet:changes.documentSet containsDocs:@[ doc1, doc2 ]]; + XC_ASSERT_THAT(changes.documentSet, ContainsDocs({doc1, doc2})); XCTAssertFalse(changes.needsRefill); XCTAssertEqual(0, changes.changeSet.GetChanges().size()); [view applyChangesToDocuments:changes]; diff --git a/Firestore/Example/Tests/Local/FSTLocalStoreTests.mm b/Firestore/Example/Tests/Local/FSTLocalStoreTests.mm index 15d30c7d6eb..79d906a5d18 100644 --- a/Firestore/Example/Tests/Local/FSTLocalStoreTests.mm +++ b/Firestore/Example/Tests/Local/FSTLocalStoreTests.mm @@ -28,7 +28,6 @@ #import "Firestore/Source/Local/FSTPersistence.h" #import "Firestore/Source/Local/FSTQueryData.h" #import "Firestore/Source/Model/FSTDocument.h" -#import "Firestore/Source/Model/FSTDocumentSet.h" #import "Firestore/Source/Model/FSTMutation.h" #import "Firestore/Source/Model/FSTMutationBatch.h" #import "Firestore/Source/Util/FSTClasses.h" @@ -40,6 +39,7 @@ #include "Firestore/core/src/firebase/firestore/auth/user.h" #include "Firestore/core/src/firebase/firestore/model/document_map.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" #include "Firestore/core/src/firebase/firestore/remote/remote_event.h" #include "Firestore/core/src/firebase/firestore/remote/watch_change.h" #include "Firestore/core/src/firebase/firestore/util/status.h" diff --git a/Firestore/Example/Tests/Model/FSTDocumentSetTests.mm b/Firestore/Example/Tests/Model/FSTDocumentSetTests.mm index 05a3a50ef68..edb2334f9b4 100644 --- a/Firestore/Example/Tests/Model/FSTDocumentSetTests.mm +++ b/Firestore/Example/Tests/Model/FSTDocumentSetTests.mm @@ -14,30 +14,22 @@ * limitations under the License. */ -#import "Firestore/Source/Model/FSTDocumentSet.h" - #import #include +#import "Firestore/Example/Tests/Util/FSTHelpers.h" #import "Firestore/Source/Model/FSTDocument.h" +// TODO(wilhuff) move to first include once this test filename matches +#include "Firestore/core/src/firebase/firestore/model/document_set.h" #include "Firestore/core/test/firebase/firestore/testutil/xcgmock.h" -#import "Firestore/Example/Tests/Util/FSTHelpers.h" - +using firebase::firestore::model::DocumentSet; using testing::ElementsAre; NS_ASSUME_NONNULL_BEGIN -std::vector Documents(FSTDocumentSet *documents) { - std::vector result; - for (FSTDocument *doc : documents.documents) { - result.push_back(doc); - } - return result; -} - @interface FSTDocumentSetTests : XCTestCase @end @@ -58,90 +50,91 @@ - (void)setUp { } - (void)testCount { - XCTAssertEqual([FSTTestDocSet(_comp, @[]) count], 0); - XCTAssertEqual([FSTTestDocSet(_comp, @[ _doc1, _doc2, _doc3 ]) count], 3); + XCTAssertEqual(FSTTestDocSet(_comp, @[]).size(), 0); + XCTAssertEqual(FSTTestDocSet(_comp, @[ _doc1, _doc2, _doc3 ]).size(), 3); } - (void)testHasKey { - FSTDocumentSet *set = FSTTestDocSet(_comp, @[ _doc1, _doc2 ]); + DocumentSet set = FSTTestDocSet(_comp, @[ _doc1, _doc2 ]); - XCTAssertTrue([set containsKey:_doc1.key]); - XCTAssertTrue([set containsKey:_doc2.key]); - XCTAssertFalse([set containsKey:_doc3.key]); + XCTAssertTrue(set.ContainsKey(_doc1.key)); + XCTAssertTrue(set.ContainsKey(_doc2.key)); + XCTAssertFalse(set.ContainsKey(_doc3.key)); } - (void)testDocumentForKey { - FSTDocumentSet *set = FSTTestDocSet(_comp, @[ _doc1, _doc2 ]); + DocumentSet set = FSTTestDocSet(_comp, @[ _doc1, _doc2 ]); - XCTAssertEqualObjects([set documentForKey:_doc1.key], _doc1); - XCTAssertEqualObjects([set documentForKey:_doc2.key], _doc2); - XCTAssertNil([set documentForKey:_doc3.key]); + XCTAssertEqualObjects(set.GetDocument(_doc1.key), _doc1); + XCTAssertEqualObjects(set.GetDocument(_doc2.key), _doc2); + XCTAssertNil(set.GetDocument(_doc3.key)); } - (void)testFirstAndLastDocument { - FSTDocumentSet *set = FSTTestDocSet(_comp, @[]); - XCTAssertNil([set firstDocument]); - XCTAssertNil([set lastDocument]); + DocumentSet set = FSTTestDocSet(_comp, @[]); + XCTAssertNil(set.GetFirstDocument()); + XCTAssertNil(set.GetLastDocument()); set = FSTTestDocSet(_comp, @[ _doc1, _doc2, _doc3 ]); - XCTAssertEqualObjects([set firstDocument], _doc3); - XCTAssertEqualObjects([set lastDocument], _doc2); + XCTAssertEqualObjects(set.GetFirstDocument(), _doc3); + XCTAssertEqualObjects(set.GetLastDocument(), _doc2); } - (void)testKeepsDocumentsInTheRightOrder { - FSTDocumentSet *set = FSTTestDocSet(_comp, @[ _doc1, _doc2, _doc3 ]); - XC_ASSERT_THAT(Documents(set), ElementsAre(_doc3, _doc1, _doc2)); + DocumentSet set = FSTTestDocSet(_comp, @[ _doc1, _doc2, _doc3 ]); + XC_ASSERT_THAT(set, ElementsAre(_doc3, _doc1, _doc2)); } - (void)testDeletes { - FSTDocumentSet *set = FSTTestDocSet(_comp, @[ _doc1, _doc2, _doc3 ]); + DocumentSet set = FSTTestDocSet(_comp, @[ _doc1, _doc2, _doc3 ]); - FSTDocumentSet *setWithoutDoc1 = [set documentSetByRemovingKey:_doc1.key]; - XC_ASSERT_THAT(Documents(setWithoutDoc1), ElementsAre(_doc3, _doc2)); - XCTAssertEqual([setWithoutDoc1 count], 2); + DocumentSet setWithoutDoc1 = set.erase(_doc1.key); + XC_ASSERT_THAT(setWithoutDoc1, ElementsAre(_doc3, _doc2)); + XCTAssertEqual(setWithoutDoc1.size(), 2); // Original remains unchanged - XC_ASSERT_THAT(Documents(set), ElementsAre(_doc3, _doc1, _doc2)); + XC_ASSERT_THAT(set, ElementsAre(_doc3, _doc1, _doc2)); - FSTDocumentSet *setWithoutDoc3 = [setWithoutDoc1 documentSetByRemovingKey:_doc3.key]; - XC_ASSERT_THAT(Documents(setWithoutDoc3), ElementsAre(_doc2)); - XCTAssertEqual([setWithoutDoc3 count], 1); + DocumentSet setWithoutDoc3 = setWithoutDoc1.erase(_doc3.key); + XC_ASSERT_THAT(setWithoutDoc3, ElementsAre(_doc2)); + XCTAssertEqual(setWithoutDoc3.size(), 1); } - (void)testUpdates { - FSTDocumentSet *set = FSTTestDocSet(_comp, @[ _doc1, _doc2, _doc3 ]); + DocumentSet set = FSTTestDocSet(_comp, @[ _doc1, _doc2, _doc3 ]); FSTDocument *doc2Prime = FSTTestDoc("docs/2", 0, @{@"sort" : @9}, FSTDocumentStateSynced); - set = [set documentSetByAddingDocument:doc2Prime]; - XCTAssertEqual([set count], 3); - XCTAssertEqualObjects([set documentForKey:doc2Prime.key], doc2Prime); - XC_ASSERT_THAT(Documents(set), ElementsAre(_doc3, _doc1, doc2Prime)); + set = set.insert(doc2Prime); + XCTAssertEqual(set.size(), 3); + XCTAssertEqualObjects(set.GetDocument(doc2Prime.key), doc2Prime); + XC_ASSERT_THAT(set, ElementsAre(_doc3, _doc1, doc2Prime)); } - (void)testAddsDocsWithEqualComparisonValues { FSTDocument *doc4 = FSTTestDoc("docs/4", 0, @{@"sort" : @2}, FSTDocumentStateSynced); - FSTDocumentSet *set = FSTTestDocSet(_comp, @[ _doc1, doc4 ]); - XC_ASSERT_THAT(Documents(set), ElementsAre(_doc1, doc4)); + DocumentSet set = FSTTestDocSet(_comp, @[ _doc1, doc4 ]); + XC_ASSERT_THAT(set, ElementsAre(_doc1, doc4)); } - (void)testIsEqual { - FSTDocumentSet *set1 = FSTTestDocSet(FSTDocumentComparatorByKey, @[ _doc1, _doc2, _doc3 ]); - FSTDocumentSet *set2 = FSTTestDocSet(FSTDocumentComparatorByKey, @[ _doc1, _doc2, _doc3 ]); - XCTAssertEqualObjects(set1, set1); - XCTAssertEqualObjects(set1, set2); - XCTAssertNotEqualObjects(set1, nil); - - FSTDocumentSet *sortedSet1 = FSTTestDocSet(_comp, @[ _doc1, _doc2, _doc3 ]); - FSTDocumentSet *sortedSet2 = FSTTestDocSet(_comp, @[ _doc1, _doc2, _doc3 ]); - XCTAssertEqualObjects(sortedSet1, sortedSet1); - XCTAssertEqualObjects(sortedSet1, sortedSet2); - XCTAssertNotEqualObjects(sortedSet1, nil); - - FSTDocumentSet *shortSet = FSTTestDocSet(FSTDocumentComparatorByKey, @[ _doc1, _doc2 ]); - XCTAssertNotEqualObjects(set1, shortSet); - XCTAssertNotEqualObjects(set1, sortedSet1); + DocumentSet empty{FSTDocumentComparatorByKey}; + DocumentSet set1 = FSTTestDocSet(FSTDocumentComparatorByKey, @[ _doc1, _doc2, _doc3 ]); + DocumentSet set2 = FSTTestDocSet(FSTDocumentComparatorByKey, @[ _doc1, _doc2, _doc3 ]); + XCTAssertEqual(set1, set1); + XCTAssertEqual(set1, set2); + XCTAssertNotEqual(set1, empty); + + DocumentSet sortedSet1 = FSTTestDocSet(_comp, @[ _doc1, _doc2, _doc3 ]); + DocumentSet sortedSet2 = FSTTestDocSet(_comp, @[ _doc1, _doc2, _doc3 ]); + XCTAssertEqual(sortedSet1, sortedSet1); + XCTAssertEqual(sortedSet1, sortedSet2); + XCTAssertNotEqual(sortedSet1, empty); + + DocumentSet shortSet = FSTTestDocSet(FSTDocumentComparatorByKey, @[ _doc1, _doc2 ]); + XCTAssertNotEqual(set1, shortSet); + XCTAssertNotEqual(set1, sortedSet1); } @end diff --git a/Firestore/Example/Tests/Util/FSTHelpers.h b/Firestore/Example/Tests/Util/FSTHelpers.h index 8a485f2dca8..a56b9b7eac1 100644 --- a/Firestore/Example/Tests/Util/FSTHelpers.h +++ b/Firestore/Example/Tests/Util/FSTHelpers.h @@ -24,6 +24,7 @@ #include "Firestore/core/src/firebase/firestore/core/view_snapshot.h" #include "Firestore/core/src/firebase/firestore/model/document_map.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" #include "Firestore/core/src/firebase/firestore/model/field_path.h" #include "Firestore/core/src/firebase/firestore/model/field_value.h" #include "Firestore/core/src/firebase/firestore/model/resource_path.h" @@ -37,7 +38,6 @@ @class FSTDeletedDocument; @class FSTDocument; @class FSTDocumentKeyReference; -@class FSTDocumentSet; @class FSTFieldValue; @class FSTFilter; @class FSTLocalViewChanges; @@ -275,10 +275,11 @@ FSTSortOrder *FSTTestOrderBy(const absl::string_view field, NSString *direction) NSComparator FSTTestDocComparator(const absl::string_view fieldPath); /** - * Creates a FSTDocumentSet based on the given comparator, initially containing the given + * Creates a DocumentSet based on the given comparator, initially containing the given * documents. */ -FSTDocumentSet *FSTTestDocSet(NSComparator comp, NSArray *docs); +firebase::firestore::model::DocumentSet FSTTestDocSet(NSComparator comp, + NSArray *docs); /** Computes changes to the view with the docs and then applies them and returns the snapshot. */ absl::optional FSTTestApplyChanges( diff --git a/Firestore/Example/Tests/Util/FSTHelpers.mm b/Firestore/Example/Tests/Util/FSTHelpers.mm index 12facfbfcc2..db77d439c9e 100644 --- a/Firestore/Example/Tests/Util/FSTHelpers.mm +++ b/Firestore/Example/Tests/Util/FSTHelpers.mm @@ -32,7 +32,6 @@ #import "Firestore/Source/Local/FSTLocalViewChanges.h" #import "Firestore/Source/Local/FSTQueryData.h" #import "Firestore/Source/Model/FSTDocument.h" -#import "Firestore/Source/Model/FSTDocumentSet.h" #import "Firestore/Source/Model/FSTFieldValue.h" #import "Firestore/Source/Model/FSTMutation.h" @@ -40,6 +39,7 @@ #include "Firestore/core/src/firebase/firestore/model/database_id.h" #include "Firestore/core/src/firebase/firestore/model/document_key.h" #include "Firestore/core/src/firebase/firestore/model/document_key_set.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" #include "Firestore/core/src/firebase/firestore/model/field_mask.h" #include "Firestore/core/src/firebase/firestore/model/field_transform.h" #include "Firestore/core/src/firebase/firestore/model/field_value.h" @@ -59,6 +59,7 @@ using firebase::firestore::model::DatabaseId; using firebase::firestore::model::DocumentKey; using firebase::firestore::model::DocumentKeySet; +using firebase::firestore::model::DocumentSet; using firebase::firestore::model::FieldMask; using firebase::firestore::model::FieldPath; using firebase::firestore::model::FieldTransform; @@ -236,10 +237,10 @@ NSComparator FSTTestDocComparator(const absl::string_view fieldPath) { return [query comparator]; } -FSTDocumentSet *FSTTestDocSet(NSComparator comp, NSArray *docs) { - FSTDocumentSet *docSet = [FSTDocumentSet documentSetWithComparator:comp]; +DocumentSet FSTTestDocSet(NSComparator comp, NSArray *docs) { + DocumentSet docSet{comp}; for (FSTDocument *doc in docs) { - docSet = [docSet documentSetByAddingDocument:doc]; + docSet = docSet.insert(doc); } return docSet; } diff --git a/Firestore/Source/API/FIRDocumentChange.mm b/Firestore/Source/API/FIRDocumentChange.mm index ffcf645cfb8..1a5bccd20bb 100644 --- a/Firestore/Source/API/FIRDocumentChange.mm +++ b/Firestore/Source/API/FIRDocumentChange.mm @@ -20,14 +20,15 @@ #import "Firestore/Source/API/FIRFirestore+Internal.h" #import "Firestore/Source/Core/FSTQuery.h" #import "Firestore/Source/Model/FSTDocument.h" -#import "Firestore/Source/Model/FSTDocumentSet.h" #include "Firestore/core/src/firebase/firestore/core/view_snapshot.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" #include "Firestore/core/src/firebase/firestore/util/hard_assert.h" using firebase::firestore::api::Firestore; using firebase::firestore::core::DocumentViewChange; using firebase::firestore::core::ViewSnapshot; +using firebase::firestore::model::DocumentSet; NS_ASSUME_NONNULL_BEGIN @@ -59,7 +60,7 @@ + (FIRDocumentChangeType)documentChangeTypeForChange:(const DocumentViewChange & + (NSArray *)documentChangesForSnapshot:(const ViewSnapshot &)snapshot includeMetadataChanges:(bool)includeMetadataChanges firestore:(Firestore *)firestore { - if (snapshot.old_documents().isEmpty) { + if (snapshot.old_documents().empty()) { // Special case the first snapshot because index calculation is easy and fast. Also all changes // on the first snapshot are adds so there are also no metadata-only changes to filter out. FSTDocument *_Nullable lastDocument = nil; @@ -86,7 +87,7 @@ + (FIRDocumentChangeType)documentChangeTypeForChange:(const DocumentViewChange & } else { // A DocumentSet that is updated incrementally as changes are applied to use to lookup the index // of a document. - FSTDocumentSet *indexTracker = snapshot.old_documents(); + DocumentSet indexTracker = snapshot.old_documents(); NSMutableArray *changes = [NSMutableArray array]; for (const DocumentViewChange &change : snapshot.document_changes()) { if (!includeMetadataChanges && change.type() == DocumentViewChange::Type::kMetadata) { @@ -100,16 +101,16 @@ + (FIRDocumentChangeType)documentChangeTypeForChange:(const DocumentViewChange & fromCache:snapshot.from_cache() hasPendingWrites:snapshot.mutated_keys().contains(change.document().key)]; - NSUInteger oldIndex = NSNotFound; - NSUInteger newIndex = NSNotFound; + size_t oldIndex = DocumentSet::npos; + size_t newIndex = DocumentSet::npos; if (change.type() != DocumentViewChange::Type::kAdded) { - oldIndex = [indexTracker indexOfKey:change.document().key]; - HARD_ASSERT(oldIndex != NSNotFound, "Index for document not found"); - indexTracker = [indexTracker documentSetByRemovingKey:change.document().key]; + oldIndex = indexTracker.IndexOf(change.document().key); + HARD_ASSERT(oldIndex != DocumentSet::npos, "Index for document not found"); + indexTracker = indexTracker.erase(change.document().key); } if (change.type() != DocumentViewChange::Type::kRemoved) { - indexTracker = [indexTracker documentSetByAddingDocument:change.document()]; - newIndex = [indexTracker indexOfKey:change.document().key]; + indexTracker = indexTracker.insert(change.document()); + newIndex = indexTracker.IndexOf(change.document().key); } [FIRDocumentChange documentChangeTypeForChange:change]; FIRDocumentChangeType type = [FIRDocumentChange documentChangeTypeForChange:change]; diff --git a/Firestore/Source/API/FIRDocumentReference.mm b/Firestore/Source/API/FIRDocumentReference.mm index ef5cce388bf..ac494db2e1d 100644 --- a/Firestore/Source/API/FIRDocumentReference.mm +++ b/Firestore/Source/API/FIRDocumentReference.mm @@ -29,13 +29,13 @@ #import "Firestore/Source/API/FSTUserDataConverter.h" #import "Firestore/Source/Core/FSTEventManager.h" #import "Firestore/Source/Core/FSTQuery.h" -#import "Firestore/Source/Model/FSTDocumentSet.h" #import "Firestore/Source/Model/FSTFieldValue.h" #import "Firestore/Source/Util/FSTUsageValidation.h" #include "Firestore/core/src/firebase/firestore/api/document_reference.h" #include "Firestore/core/src/firebase/firestore/api/document_snapshot.h" #include "Firestore/core/src/firebase/firestore/model/document_key.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" #include "Firestore/core/src/firebase/firestore/model/precondition.h" #include "Firestore/core/src/firebase/firestore/model/resource_path.h" #include "Firestore/core/src/firebase/firestore/util/error_apple.h" diff --git a/Firestore/Source/API/FIRQuerySnapshot+Internal.h b/Firestore/Source/API/FIRQuerySnapshot+Internal.h index 37c953d7d33..96589d1a3dd 100644 --- a/Firestore/Source/API/FIRQuerySnapshot+Internal.h +++ b/Firestore/Source/API/FIRQuerySnapshot+Internal.h @@ -23,7 +23,6 @@ @class FIRFirestore; @class FIRSnapshotMetadata; -@class FSTDocumentSet; @class FSTQuery; using firebase::firestore::api::Firestore; diff --git a/Firestore/Source/API/FIRQuerySnapshot.mm b/Firestore/Source/API/FIRQuerySnapshot.mm index 837055612d3..52a4810575c 100644 --- a/Firestore/Source/API/FIRQuerySnapshot.mm +++ b/Firestore/Source/API/FIRQuerySnapshot.mm @@ -26,19 +26,21 @@ #import "Firestore/Source/API/FIRSnapshotMetadata+Internal.h" #import "Firestore/Source/Core/FSTQuery.h" #import "Firestore/Source/Model/FSTDocument.h" -#import "Firestore/Source/Model/FSTDocumentSet.h" #import "Firestore/Source/Util/FSTUsageValidation.h" #include "Firestore/core/src/firebase/firestore/core/view_snapshot.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" +#include "Firestore/core/src/firebase/firestore/util/delayed_constructor.h" using firebase::firestore::api::Firestore; using firebase::firestore::api::QuerySnapshot; using firebase::firestore::core::ViewSnapshot; +using firebase::firestore::util::DelayedConstructor; NS_ASSUME_NONNULL_BEGIN @implementation FIRQuerySnapshot { - QuerySnapshot _snapshot; + DelayedConstructor _snapshot; FIRSnapshotMetadata *_cached_metadata; @@ -52,7 +54,7 @@ @implementation FIRQuerySnapshot { - (instancetype)initWithSnapshot:(QuerySnapshot &&)snapshot { if (self = [super init]) { - _snapshot = std::move(snapshot); + _snapshot.Init(std::move(snapshot)); } return self; } @@ -70,21 +72,21 @@ - (BOOL)isEqual:(nullable id)other { if (![other isKindOfClass:[FIRQuerySnapshot class]]) return NO; FIRQuerySnapshot *otherSnapshot = other; - return _snapshot == otherSnapshot->_snapshot; + return *_snapshot == *(otherSnapshot->_snapshot); } - (NSUInteger)hash { - return _snapshot.Hash(); + return _snapshot->Hash(); } - (FIRQuery *)query { - FIRFirestore *firestore = [FIRFirestore recoverFromFirestore:_snapshot.firestore()]; - return [FIRQuery referenceWithQuery:_snapshot.internal_query() firestore:firestore]; + FIRFirestore *firestore = [FIRFirestore recoverFromFirestore:_snapshot->firestore()]; + return [FIRQuery referenceWithQuery:_snapshot->internal_query() firestore:firestore]; } - (FIRSnapshotMetadata *)metadata { if (!_cached_metadata) { - _cached_metadata = [[FIRSnapshotMetadata alloc] initWithMetadata:_snapshot.metadata()]; + _cached_metadata = [[FIRSnapshotMetadata alloc] initWithMetadata:_snapshot->metadata()]; } return _cached_metadata; } @@ -92,20 +94,20 @@ - (FIRSnapshotMetadata *)metadata { @dynamic empty; - (BOOL)isEmpty { - return _snapshot.empty(); + return _snapshot->empty(); } // This property is exposed as an NSInteger instead of an NSUInteger since (as of Xcode 8.1) // Swift bridges NSUInteger as UInt, and we want to avoid forcing Swift users to cast their ints // where we can. See cr/146959032 for additional context. - (NSInteger)count { - return static_cast(_snapshot.size()); + return static_cast(_snapshot->size()); } - (NSArray *)documents { if (!_documents) { NSMutableArray *result = [NSMutableArray array]; - _snapshot.ForEachDocument([&result](DocumentSnapshot snapshot) { + _snapshot->ForEachDocument([&result](DocumentSnapshot snapshot) { [result addObject:[[FIRQueryDocumentSnapshot alloc] initWithSnapshot:std::move(snapshot)]]; }); @@ -120,16 +122,16 @@ - (NSInteger)count { - (NSArray *)documentChangesWithIncludeMetadataChanges: (BOOL)includeMetadataChanges { - if (includeMetadataChanges && _snapshot.view_snapshot().excludes_metadata_changes()) { + if (includeMetadataChanges && _snapshot->view_snapshot().excludes_metadata_changes()) { FSTThrowInvalidArgument( @"To include metadata changes with your document changes, you must call " @"addSnapshotListener(includeMetadataChanges: true)."); } if (!_documentChanges || _documentChangesIncludeMetadataChanges != includeMetadataChanges) { - _documentChanges = [FIRDocumentChange documentChangesForSnapshot:_snapshot.view_snapshot() + _documentChanges = [FIRDocumentChange documentChangesForSnapshot:_snapshot->view_snapshot() includeMetadataChanges:includeMetadataChanges - firestore:_snapshot.firestore()]; + firestore:_snapshot->firestore()]; _documentChangesIncludeMetadataChanges = includeMetadataChanges; } return _documentChanges; diff --git a/Firestore/Source/Core/FSTEventManager.mm b/Firestore/Source/Core/FSTEventManager.mm index 384dbb2910b..df69f05b243 100644 --- a/Firestore/Source/Core/FSTEventManager.mm +++ b/Firestore/Source/Core/FSTEventManager.mm @@ -21,8 +21,8 @@ #import "Firestore/Source/Core/FSTQuery.h" #import "Firestore/Source/Core/FSTSyncEngine.h" -#import "Firestore/Source/Model/FSTDocumentSet.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" #include "Firestore/core/src/firebase/firestore/util/error_apple.h" #include "Firestore/core/src/firebase/firestore/util/hard_assert.h" #include "Firestore/core/src/firebase/firestore/util/status.h" @@ -216,7 +216,7 @@ - (BOOL)shouldRaiseInitialEventForSnapshot:(const ViewSnapshot &)snapshot } // Raise data from cache if we have any documents or we are offline - return !snapshot.documents().isEmpty || onlineState == OnlineState::Offline; + return !snapshot.documents().empty() || onlineState == OnlineState::Offline; } - (BOOL)shouldRaiseEventForSnapshot:(const ViewSnapshot &)snapshot { diff --git a/Firestore/Source/Core/FSTFirestoreClient.mm b/Firestore/Source/Core/FSTFirestoreClient.mm index 35d593bf318..76af1cd5d17 100644 --- a/Firestore/Source/Core/FSTFirestoreClient.mm +++ b/Firestore/Source/Core/FSTFirestoreClient.mm @@ -39,13 +39,13 @@ #import "Firestore/Source/Local/FSTLocalStore.h" #import "Firestore/Source/Local/FSTMemoryPersistence.h" #import "Firestore/Source/Model/FSTDocument.h" -#import "Firestore/Source/Model/FSTDocumentSet.h" #import "Firestore/Source/Remote/FSTSerializerBeta.h" #import "Firestore/Source/Util/FSTClasses.h" #include "Firestore/core/src/firebase/firestore/auth/credentials_provider.h" #include "Firestore/core/src/firebase/firestore/core/database_info.h" #include "Firestore/core/src/firebase/firestore/model/database_id.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" #include "Firestore/core/src/firebase/firestore/remote/datastore.h" #include "Firestore/core/src/firebase/firestore/remote/remote_store.h" #include "Firestore/core/src/firebase/firestore/util/async_queue.h" diff --git a/Firestore/Source/Core/FSTSyncEngine.mm b/Firestore/Source/Core/FSTSyncEngine.mm index 728933b425d..b00982fea36 100644 --- a/Firestore/Source/Core/FSTSyncEngine.mm +++ b/Firestore/Source/Core/FSTSyncEngine.mm @@ -31,7 +31,6 @@ #import "Firestore/Source/Local/FSTLocalWriteResult.h" #import "Firestore/Source/Local/FSTQueryData.h" #import "Firestore/Source/Model/FSTDocument.h" -#import "Firestore/Source/Model/FSTDocumentSet.h" #import "Firestore/Source/Model/FSTMutationBatch.h" #include "Firestore/core/src/firebase/firestore/auth/user.h" @@ -41,6 +40,7 @@ #include "Firestore/core/src/firebase/firestore/local/reference_set.h" #include "Firestore/core/src/firebase/firestore/model/document_key.h" #include "Firestore/core/src/firebase/firestore/model/document_map.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" #include "Firestore/core/src/firebase/firestore/model/snapshot_version.h" #include "Firestore/core/src/firebase/firestore/remote/remote_event.h" #include "Firestore/core/src/firebase/firestore/util/error_apple.h" diff --git a/Firestore/Source/Core/FSTView.h b/Firestore/Source/Core/FSTView.h index f6048f9d7ca..aed87321503 100644 --- a/Firestore/Source/Core/FSTView.h +++ b/Firestore/Source/Core/FSTView.h @@ -34,7 +34,6 @@ class TargetChange; } // namespace firestore } // namespace firebase -@class FSTDocumentSet; @class FSTQuery; NS_ASSUME_NONNULL_BEGIN @@ -49,7 +48,7 @@ NS_ASSUME_NONNULL_BEGIN - (const firebase::firestore::model::DocumentKeySet &)mutatedKeys; /** The new set of docs that should be in the view. */ -@property(nonatomic, strong, readonly) FSTDocumentSet *documentSet; +- (const firebase::firestore::model::DocumentSet &)documentSet; /** The diff of this these docs with the previous set of docs. */ - (const firebase::firestore::core::DocumentViewChangeSet &)changeSet; diff --git a/Firestore/Source/Core/FSTView.mm b/Firestore/Source/Core/FSTView.mm index 687dddfe4d9..a0687767706 100644 --- a/Firestore/Source/Core/FSTView.mm +++ b/Firestore/Source/Core/FSTView.mm @@ -22,12 +22,14 @@ #import "Firestore/Source/Core/FSTQuery.h" #import "Firestore/Source/Model/FSTDocument.h" -#import "Firestore/Source/Model/FSTDocumentSet.h" #import "Firestore/Source/Model/FSTFieldValue.h" #include "Firestore/core/src/firebase/firestore/core/view_snapshot.h" #include "Firestore/core/src/firebase/firestore/model/document_key.h" +#include "Firestore/core/src/firebase/firestore/model/document_key_set.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" #include "Firestore/core/src/firebase/firestore/remote/remote_event.h" +#include "Firestore/core/src/firebase/firestore/util/delayed_constructor.h" #include "Firestore/core/src/firebase/firestore/util/hard_assert.h" using firebase::firestore::core::DocumentViewChange; @@ -36,9 +38,11 @@ using firebase::firestore::core::ViewSnapshot; using firebase::firestore::model::DocumentKey; using firebase::firestore::model::DocumentKeySet; +using firebase::firestore::model::DocumentSet; using firebase::firestore::model::MaybeDocumentMap; using firebase::firestore::model::OnlineState; using firebase::firestore::remote::TargetChange; +using firebase::firestore::util::DelayedConstructor; NS_ASSUME_NONNULL_BEGIN @@ -68,7 +72,7 @@ int GetDocumentViewChangeTypePosition(DocumentViewChange::Type changeType) { /** The result of applying a set of doc changes to a view. */ @interface FSTViewDocumentChanges () -- (instancetype)initWithDocumentSet:(FSTDocumentSet *)documentSet +- (instancetype)initWithDocumentSet:(DocumentSet)documentSet changeSet:(DocumentViewChangeSet &&)changeSet needsRefill:(BOOL)needsRefill mutatedKeys:(DocumentKeySet)mutatedKeys NS_DESIGNATED_INITIALIZER; @@ -76,17 +80,18 @@ - (instancetype)initWithDocumentSet:(FSTDocumentSet *)documentSet @end @implementation FSTViewDocumentChanges { + DelayedConstructor _documentSet; DocumentKeySet _mutatedKeys; DocumentViewChangeSet _changeSet; } -- (instancetype)initWithDocumentSet:(FSTDocumentSet *)documentSet +- (instancetype)initWithDocumentSet:(DocumentSet)documentSet changeSet:(DocumentViewChangeSet &&)changeSet needsRefill:(BOOL)needsRefill mutatedKeys:(DocumentKeySet)mutatedKeys { self = [super init]; if (self) { - _documentSet = documentSet; + _documentSet.Init(std::move(documentSet)); _changeSet = std::move(changeSet); _needsRefill = needsRefill; _mutatedKeys = std::move(mutatedKeys); @@ -98,6 +103,10 @@ - (instancetype)initWithDocumentSet:(FSTDocumentSet *)documentSet return _mutatedKeys; } +- (const firebase::firestore::model::DocumentSet &)documentSet { + return *_documentSet; +} + - (const firebase::firestore::core::DocumentViewChangeSet &)changeSet { return _changeSet; } @@ -208,11 +217,11 @@ @interface FSTView () */ @property(nonatomic, assign, getter=isCurrent) BOOL current; -@property(nonatomic, strong) FSTDocumentSet *documentSet; - @end @implementation FSTView { + DelayedConstructor _documentSet; + /** Documents included in the remote target. */ DocumentKeySet _syncedDocuments; @@ -227,7 +236,7 @@ - (instancetype)initWithQuery:(FSTQuery *)query remoteDocuments:(DocumentKeySet) self = [super init]; if (self) { _query = query; - _documentSet = [FSTDocumentSet documentSetWithComparator:query.comparator]; + _documentSet.Init(query.comparator); _syncedDocuments = std::move(remoteDocuments); } return self; @@ -248,11 +257,11 @@ - (FSTViewDocumentChanges *)computeChangesWithDocuments:(const MaybeDocumentMap if (previousChanges) { changeSet = previousChanges.changeSet; } - FSTDocumentSet *oldDocumentSet = previousChanges ? previousChanges.documentSet : self.documentSet; + DocumentSet oldDocumentSet = previousChanges ? previousChanges.documentSet : *_documentSet; DocumentKeySet newMutatedKeys = previousChanges ? previousChanges.mutatedKeys : _mutatedKeys; DocumentKeySet oldMutatedKeys = _mutatedKeys; - FSTDocumentSet *newDocumentSet = oldDocumentSet; + DocumentSet newDocumentSet = oldDocumentSet; BOOL needsRefill = NO; // Track the last doc in a (full) limit. This is necessary, because some update (a delete, or an @@ -264,14 +273,15 @@ - (FSTViewDocumentChanges *)computeChangesWithDocuments:(const MaybeDocumentMap // Note that this should never get used in a refill (when previousChanges is set), because there // will only be adds -- no deletes or updates. FSTDocument *_Nullable lastDocInLimit = - (self.query.limit && oldDocumentSet.count == self.query.limit) ? oldDocumentSet.lastDocument - : nil; + (self.query.limit && oldDocumentSet.size() == self.query.limit) + ? oldDocumentSet.GetLastDocument() + : nil; for (const auto &kv : docChanges) { const DocumentKey &key = kv.first; FSTMaybeDocument *maybeNewDoc = kv.second; - FSTDocument *_Nullable oldDoc = [oldDocumentSet documentForKey:key]; + FSTDocument *_Nullable oldDoc = oldDocumentSet.GetDocument(key); FSTDocument *_Nullable newDoc = nil; if ([maybeNewDoc isKindOfClass:[FSTDocument class]]) { newDoc = (FSTDocument *)maybeNewDoc; @@ -328,23 +338,23 @@ - (FSTViewDocumentChanges *)computeChangesWithDocuments:(const MaybeDocumentMap if (changeApplied) { if (newDoc) { - newDocumentSet = [newDocumentSet documentSetByAddingDocument:newDoc]; + newDocumentSet = newDocumentSet.insert(newDoc); if (newDoc.hasLocalMutations) { newMutatedKeys = newMutatedKeys.insert(key); } else { newMutatedKeys = newMutatedKeys.erase(key); } } else { - newDocumentSet = [newDocumentSet documentSetByRemovingKey:key]; + newDocumentSet = newDocumentSet.erase(key); newMutatedKeys = newMutatedKeys.erase(key); } } } if (self.query.limit) { - for (long i = newDocumentSet.count - self.query.limit; i > 0; --i) { - FSTDocument *oldDoc = [newDocumentSet lastDocument]; - newDocumentSet = [newDocumentSet documentSetByRemovingKey:oldDoc.key]; + for (long i = newDocumentSet.size() - self.query.limit; i > 0; --i) { + FSTDocument *oldDoc = newDocumentSet.GetLastDocument(); + newDocumentSet = newDocumentSet.erase(oldDoc.key); newMutatedKeys = newMutatedKeys.erase(oldDoc.key); changeSet.AddChange(DocumentViewChange{oldDoc, DocumentViewChange::Type::kRemoved}); } @@ -353,7 +363,7 @@ - (FSTViewDocumentChanges *)computeChangesWithDocuments:(const MaybeDocumentMap HARD_ASSERT(!needsRefill || !previousChanges, "View was refilled using docs that themselves needed refilling."); - return [[FSTViewDocumentChanges alloc] initWithDocumentSet:newDocumentSet + return [[FSTViewDocumentChanges alloc] initWithDocumentSet:std::move(newDocumentSet) changeSet:std::move(changeSet) needsRefill:needsRefill mutatedKeys:newMutatedKeys]; @@ -377,8 +387,8 @@ - (FSTViewChange *)applyChangesToDocuments:(FSTViewDocumentChanges *)docChanges targetChange:(const absl::optional &)targetChange { HARD_ASSERT(!docChanges.needsRefill, "Cannot apply changes that need a refill"); - FSTDocumentSet *oldDocuments = self.documentSet; - self.documentSet = docChanges.documentSet; + DocumentSet oldDocuments = *_documentSet; + *_documentSet = docChanges.documentSet; _mutatedKeys = docChanges.mutatedKeys; // Sort changes based on type and query comparator. @@ -424,7 +434,7 @@ - (FSTViewChange *)applyChangedOnlineState:(OnlineState)onlineState { // that sets `current` back to YES once the client is back online. self.current = NO; return [self applyChangesToDocuments:[[FSTViewDocumentChanges alloc] - initWithDocumentSet:self.documentSet + initWithDocumentSet:*_documentSet changeSet:DocumentViewChangeSet {} needsRefill:NO mutatedKeys:_mutatedKeys]]; @@ -443,14 +453,14 @@ - (BOOL)shouldBeLimboDocumentKey:(const DocumentKey &)key { return NO; } // The local store doesn't think it's a result, so it shouldn't be in limbo. - if (![self.documentSet containsKey:key]) { + if (!_documentSet->ContainsKey(key)) { return NO; } // If there are local changes to the doc, they might explain why the server doesn't know that it's // part of the query. So don't put it in limbo. // TODO(klimt): Ideally, we would only consider changes that might actually affect this specific // query. - if ([self.documentSet documentForKey:key].hasLocalMutations) { + if (_documentSet->GetDocument(key).hasLocalMutations) { return NO; } // Everything else is in limbo. @@ -489,7 +499,7 @@ - (void)applyTargetChange:(const absl::optional &)maybeTargetChang // TODO(klimt): Do this incrementally so that it's not quadratic when updating many documents. DocumentKeySet oldLimboDocuments = std::move(_limboDocuments); _limboDocuments = DocumentKeySet{}; - for (FSTDocument *doc : self.documentSet.documents) { + for (FSTDocument *doc : *_documentSet) { if ([self shouldBeLimboDocumentKey:doc.key]) { _limboDocuments = _limboDocuments.insert(doc.key); } diff --git a/Firestore/Source/Local/FSTLocalViewChanges.h b/Firestore/Source/Local/FSTLocalViewChanges.h index 47651a0a272..66ec8631194 100644 --- a/Firestore/Source/Local/FSTLocalViewChanges.h +++ b/Firestore/Source/Local/FSTLocalViewChanges.h @@ -20,7 +20,6 @@ #include "Firestore/core/src/firebase/firestore/model/document_key_set.h" #include "Firestore/core/src/firebase/firestore/model/types.h" -@class FSTDocumentSet; @class FSTMutation; @class FSTQuery; diff --git a/Firestore/core/src/firebase/firestore/api/document_reference.mm b/Firestore/core/src/firebase/firestore/api/document_reference.mm index dc883d05cf0..75f568e79d7 100644 --- a/Firestore/core/src/firebase/firestore/api/document_reference.mm +++ b/Firestore/core/src/firebase/firestore/api/document_reference.mm @@ -24,13 +24,13 @@ #import "Firestore/Source/Core/FSTEventManager.h" #import "Firestore/Source/Core/FSTFirestoreClient.h" #import "Firestore/Source/Core/FSTQuery.h" -#import "Firestore/Source/Model/FSTDocumentSet.h" #import "Firestore/Source/Model/FSTMutation.h" #import "Firestore/Source/Util/FSTAsyncQueryListener.h" #import "Firestore/Source/Util/FSTUsageValidation.h" #include "Firestore/core/src/firebase/firestore/core/view_snapshot.h" #include "Firestore/core/src/firebase/firestore/model/document_key.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" #include "Firestore/core/src/firebase/firestore/model/precondition.h" #include "Firestore/core/src/firebase/firestore/model/resource_path.h" #include "Firestore/core/src/firebase/firestore/util/error_apple.h" @@ -191,9 +191,9 @@ } const ViewSnapshot& snapshot = maybe_snapshot.ValueOrDie(); - HARD_ASSERT(snapshot.documents().count <= 1, + HARD_ASSERT(snapshot.documents().size() <= 1, "Too many documents returned on a document query"); - FSTDocument* document = [snapshot.documents() documentForKey:key]; + FSTDocument* document = snapshot.documents().GetDocument(key); bool has_pending_writes = document diff --git a/Firestore/core/src/firebase/firestore/api/query_snapshot.h b/Firestore/core/src/firebase/firestore/api/query_snapshot.h index 006e8421a91..50b7ed24802 100644 --- a/Firestore/core/src/firebase/firestore/api/query_snapshot.h +++ b/Firestore/core/src/firebase/firestore/api/query_snapshot.h @@ -26,11 +26,10 @@ #include #include -#import "Firestore/Source/Model/FSTDocumentSet.h" - #include "Firestore/core/src/firebase/firestore/api/document_snapshot.h" #include "Firestore/core/src/firebase/firestore/api/snapshot_metadata.h" #include "Firestore/core/src/firebase/firestore/core/view_snapshot.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" NS_ASSUME_NONNULL_BEGIN @@ -63,12 +62,12 @@ class QuerySnapshot { * Indicates whether this `QuerySnapshot` is empty (contains no documents). */ bool empty() const { - return static_cast(snapshot_.documents().isEmpty); + return snapshot_.documents().empty(); } /** The count of documents in this `QuerySnapshot`. */ size_t size() const { - return static_cast(snapshot_.documents().count); + return snapshot_.documents().size(); } Firestore* firestore() const { diff --git a/Firestore/core/src/firebase/firestore/api/query_snapshot.mm b/Firestore/core/src/firebase/firestore/api/query_snapshot.mm index a4b217b1c12..7992cc15fba 100644 --- a/Firestore/core/src/firebase/firestore/api/query_snapshot.mm +++ b/Firestore/core/src/firebase/firestore/api/query_snapshot.mm @@ -24,10 +24,10 @@ #import "Firestore/Source/API/FIRQuery+Internal.h" #import "Firestore/Source/Core/FSTQuery.h" #import "Firestore/Source/Model/FSTDocument.h" -#import "Firestore/Source/Model/FSTDocumentSet.h" #import "Firestore/Source/Util/FSTUsageValidation.h" #include "Firestore/core/src/firebase/firestore/core/view_snapshot.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" #include "Firestore/core/src/firebase/firestore/util/objc_compatibility.h" NS_ASSUME_NONNULL_BEGIN @@ -39,6 +39,7 @@ namespace objc = util::objc; using api::Firestore; using core::ViewSnapshot; +using model::DocumentSet; bool operator==(const QuerySnapshot& lhs, const QuerySnapshot& rhs) { return lhs.firestore_ == rhs.firestore_ && @@ -52,10 +53,10 @@ void QuerySnapshot::ForEachDocument( const std::function& callback) const { - FSTDocumentSet* documentSet = snapshot_.documents(); + DocumentSet documentSet = snapshot_.documents(); bool from_cache = metadata_.from_cache(); - for (FSTDocument* document : documentSet.documents) { + for (FSTDocument* document : documentSet) { bool has_pending_writes = snapshot_.mutated_keys().contains(document.key); DocumentSnapshot snap(firestore_, document.key, document, from_cache, has_pending_writes); diff --git a/Firestore/core/src/firebase/firestore/core/view_snapshot.h b/Firestore/core/src/firebase/firestore/core/view_snapshot.h index 502f4afb4e7..a4d9619d6f9 100644 --- a/Firestore/core/src/firebase/firestore/core/view_snapshot.h +++ b/Firestore/core/src/firebase/firestore/core/view_snapshot.h @@ -30,13 +30,13 @@ #include "Firestore/core/src/firebase/firestore/immutable/sorted_map.h" #include "Firestore/core/src/firebase/firestore/model/document_key.h" #include "Firestore/core/src/firebase/firestore/model/document_key_set.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" #include "Firestore/core/src/firebase/firestore/util/statusor.h" NS_ASSUME_NONNULL_BEGIN @class FSTDocument; @class FSTQuery; -@class FSTDocumentSet; namespace firebase { namespace firestore { @@ -114,8 +114,8 @@ class ViewSnapshot { ViewSnapshot() = default; ViewSnapshot(FSTQuery* query, - FSTDocumentSet* documents, - FSTDocumentSet* old_documents, + model::DocumentSet documents, + model::DocumentSet old_documents, std::vector document_changes, model::DocumentKeySet mutated_keys, bool from_cache, @@ -127,7 +127,7 @@ class ViewSnapshot { * added. */ static ViewSnapshot FromInitialDocuments(FSTQuery* query, - FSTDocumentSet* documents, + model::DocumentSet documents, model::DocumentKeySet mutated_keys, bool from_cache, bool excludes_metadata_changes); @@ -138,12 +138,12 @@ class ViewSnapshot { } /** The documents currently known to be results of the query. */ - FSTDocumentSet* documents() const { + const model::DocumentSet& documents() const { return documents_; } /** The documents of the last snapshot. */ - FSTDocumentSet* old_documents() const { + const model::DocumentSet& old_documents() const { return old_documents_; } @@ -184,8 +184,8 @@ class ViewSnapshot { private: FSTQuery* query_ = nil; - FSTDocumentSet* documents_ = nil; - FSTDocumentSet* old_documents_ = nil; + model::DocumentSet documents_; + model::DocumentSet old_documents_; std::vector document_changes_; model::DocumentKeySet mutated_keys_; diff --git a/Firestore/core/src/firebase/firestore/core/view_snapshot.mm b/Firestore/core/src/firebase/firestore/core/view_snapshot.mm index 772df6806d2..6663063298c 100644 --- a/Firestore/core/src/firebase/firestore/core/view_snapshot.mm +++ b/Firestore/core/src/firebase/firestore/core/view_snapshot.mm @@ -20,8 +20,8 @@ #import "Firestore/Source/Core/FSTQuery.h" #import "Firestore/Source/Model/FSTDocument.h" -#import "Firestore/Source/Model/FSTDocumentSet.h" +#include "Firestore/core/src/firebase/firestore/model/document_set.h" #include "Firestore/core/src/firebase/firestore/util/hashing.h" #include "Firestore/core/src/firebase/firestore/util/objc_compatibility.h" #include "Firestore/core/src/firebase/firestore/util/string_format.h" @@ -34,6 +34,7 @@ namespace objc = util::objc; using model::DocumentKey; using model::DocumentKeySet; +using model::DocumentSet; using util::StringFormat; // DocumentViewChange @@ -134,16 +135,16 @@ // ViewSnapshot ViewSnapshot::ViewSnapshot(FSTQuery* query, - FSTDocumentSet* documents, - FSTDocumentSet* old_documents, + DocumentSet documents, + DocumentSet old_documents, std::vector document_changes, model::DocumentKeySet mutated_keys, bool from_cache, bool sync_state_changed, bool excludes_metadata_changes) : query_{query}, - documents_{documents}, - old_documents_{old_documents}, + documents_{std::move(documents)}, + old_documents_{std::move(old_documents)}, document_changes_{std::move(document_changes)}, mutated_keys_{std::move(mutated_keys)}, from_cache_{from_cache}, @@ -153,21 +154,20 @@ ViewSnapshot ViewSnapshot::FromInitialDocuments( FSTQuery* query, - FSTDocumentSet* documents, + DocumentSet documents, DocumentKeySet mutated_keys, bool from_cache, bool excludes_metadata_changes) { std::vector view_changes; - for (FSTDocument* doc : documents.documents) { + for (FSTDocument* doc : documents) { view_changes.emplace_back(doc, DocumentViewChange::Type::kAdded); } - return ViewSnapshot{ - query, documents, - /*old_documents=*/ - [FSTDocumentSet documentSetWithComparator:query.comparator], - std::move(view_changes), std::move(mutated_keys), from_cache, - /*sync_state_changed=*/true, excludes_metadata_changes}; + return ViewSnapshot{query, documents, + /*old_documents=*/ + DocumentSet{query.comparator}, std::move(view_changes), + std::move(mutated_keys), from_cache, + /*sync_state_changed=*/true, excludes_metadata_changes}; } std::string ViewSnapshot::ToString() const { @@ -175,7 +175,7 @@ "", - query(), documents(), old_documents(), + query(), documents_.ToString(), old_documents_.ToString(), objc::Description(document_changes()), from_cache(), mutated_keys().size(), sync_state_changed(), excludes_metadata_changes()); } @@ -189,15 +189,15 @@ // straightforward way to compute its hash value. Since `ViewSnapshot` is // currently not stored in any dictionaries, this has no side effects. - return util::Hash([query() hash], [documents() hash], [old_documents() hash], + return util::Hash([query() hash], documents(), old_documents(), document_changes(), from_cache(), sync_state_changed(), excludes_metadata_changes()); } bool operator==(const ViewSnapshot& lhs, const ViewSnapshot& rhs) { return objc::Equals(lhs.query(), rhs.query()) && - objc::Equals(lhs.documents(), rhs.documents()) && - objc::Equals(lhs.old_documents(), rhs.old_documents()) && + lhs.documents() == rhs.documents() && + lhs.old_documents() == rhs.old_documents() && lhs.document_changes() == rhs.document_changes() && lhs.from_cache() == rhs.from_cache() && lhs.mutated_keys() == rhs.mutated_keys() && diff --git a/Firestore/core/src/firebase/firestore/model/document_set.h b/Firestore/core/src/firebase/firestore/model/document_set.h index 8a600402e18..eb0005bd77f 100644 --- a/Firestore/core/src/firebase/firestore/model/document_set.h +++ b/Firestore/core/src/firebase/firestore/model/document_set.h @@ -22,10 +22,12 @@ #include #include #include +#include #include "Firestore/core/src/firebase/firestore/immutable/sorted_set.h" #include "Firestore/core/src/firebase/firestore/model/document_key.h" #include "Firestore/core/src/firebase/firestore/model/document_map.h" +#include "Firestore/core/src/firebase/firestore/util/comparison.h" @class FSTDocument; @@ -59,7 +61,8 @@ class DocumentSetComparator { * comparator on top of what is provided to guarantee document equality based on * the key. */ -class DocumentSet : public immutable::impl::SortedMapBase { +class DocumentSet : public immutable::impl::SortedMapBase, + public util::Equatable { public: /** * The type of the main collection of documents in an DocumentSet. @@ -124,7 +127,7 @@ class DocumentSet : public immutable::impl::SortedMapBase { * Returns a copy of the documents in this set as an array. This is O(n) on * the size of the set. */ - NSArray* GetArrayValue() const; + std::vector ToVector() const; /** * Returns the documents as a `DocumentMap`. This is O(1) as this leverages diff --git a/Firestore/core/src/firebase/firestore/model/document_set.mm b/Firestore/core/src/firebase/firestore/model/document_set.mm index 565b2cc5534..ff67bfcdf9b 100644 --- a/Firestore/core/src/firebase/firestore/model/document_set.mm +++ b/Firestore/core/src/firebase/firestore/model/document_set.mm @@ -24,7 +24,6 @@ #include "Firestore/core/src/firebase/firestore/immutable/sorted_set.h" #include "Firestore/core/src/firebase/firestore/model/document_key.h" #include "Firestore/core/src/firebase/firestore/util/objc_compatibility.h" -#include "Firestore/core/src/firebase/firestore/util/range.h" NS_ASSUME_NONNULL_BEGIN @@ -104,13 +103,8 @@ return doc ? sorted_set_.find_index(doc) : NSNotFound; } -NSArray* DocumentSet::GetArrayValue() const { - NSMutableArray* result = - [NSMutableArray arrayWithCapacity:size()]; - for (FSTDocument* doc : sorted_set_) { - [result addObject:doc]; - } - return result; +std::vector DocumentSet::ToVector() const { + return {sorted_set_.begin(), sorted_set_.end()}; } const DocumentMap& DocumentSet::GetMapValue() const { From 614132feb29f1e655ba0fbcc80abb74bd88d3185 Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Thu, 21 Mar 2019 15:25:06 -0700 Subject: [PATCH 05/11] Remove FSTDocumentSet --- Firestore/Source/Model/FSTDocumentSet.h | 91 ------------ Firestore/Source/Model/FSTDocumentSet.mm | 131 ------------------ .../firebase/firestore/model/document_set.h | 12 -- .../firebase/firestore/model/document_set.mm | 8 -- 4 files changed, 242 deletions(-) delete mode 100644 Firestore/Source/Model/FSTDocumentSet.h delete mode 100644 Firestore/Source/Model/FSTDocumentSet.mm diff --git a/Firestore/Source/Model/FSTDocumentSet.h b/Firestore/Source/Model/FSTDocumentSet.h deleted file mode 100644 index 4da8b8f6282..00000000000 --- a/Firestore/Source/Model/FSTDocumentSet.h +++ /dev/null @@ -1,91 +0,0 @@ -/* - * Copyright 2017 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. - */ - -#import - -#include "Firestore/core/src/firebase/firestore/immutable/sorted_set.h" -#include "Firestore/core/src/firebase/firestore/model/document_key.h" -#include "Firestore/core/src/firebase/firestore/model/document_map.h" -#include "Firestore/core/src/firebase/firestore/model/document_set.h" - -@class FSTDocument; - -using firebase::firestore::model::DocumentSet; - -NS_ASSUME_NONNULL_BEGIN - -/** - * DocumentSet is an immutable (copy-on-write) collection that holds documents in order specified - * by the provided comparator. We always add a document key comparator on top of what is provided - * to guarantee document equality based on the key. - */ -@interface FSTDocumentSet : NSObject - -/** Creates a new, empty FSTDocumentSet sorted by the given comparator, then by keys. */ -+ (instancetype)documentSetWithComparator:(NSComparator)comparator; - -- (instancetype)initWithDocumentSet:(DocumentSet &&)documentSet NS_DESIGNATED_INITIALIZER; - -- (instancetype)init NS_UNAVAILABLE; - -- (NSUInteger)count; - -/** Returns true if the dictionary contains no elements. */ -- (BOOL)isEmpty; - -/** Returns YES if this set contains a document with the given key. */ -- (BOOL)containsKey:(const firebase::firestore::model::DocumentKey &)key; - -/** Returns the document from this set with the given key if it exists or nil if it doesn't. */ -- (FSTDocument *_Nullable)documentForKey:(const firebase::firestore::model::DocumentKey &)key; - -/** - * Returns the first document in the set according to its built in ordering, or nil if the set - * is empty. - */ -- (FSTDocument *_Nullable)firstDocument; - -/** - * Returns the last document in the set according to its built in ordering, or nil if the set - * is empty. - */ -- (FSTDocument *_Nullable)lastDocument; - -/** - * Returns the index of the document with the provided key in the document set. Returns NSNotFound - * if the key is not present. - */ -- (NSUInteger)indexOfKey:(const firebase::firestore::model::DocumentKey &)key; - -- (const DocumentSet &)documents; - -/** Returns a copy of the documents in this set as an array. This is O(n) on the size of the set. */ -- (NSArray *)arrayValue; - -/** - * Returns the documents as a `DocumentMap`. This is O(1) as this leverages - * our internal representation. - */ -- (const firebase::firestore::model::DocumentMap &)mapValue; - -/** Returns a new FSTDocumentSet that contains the given document. */ -- (instancetype)documentSetByAddingDocument:(FSTDocument *_Nullable)document; - -/** Returns a new FSTDocumentSet that excludes any document associated with the given key. */ -- (instancetype)documentSetByRemovingKey:(const firebase::firestore::model::DocumentKey &)key; -@end - -NS_ASSUME_NONNULL_END diff --git a/Firestore/Source/Model/FSTDocumentSet.mm b/Firestore/Source/Model/FSTDocumentSet.mm deleted file mode 100644 index 64e2843b085..00000000000 --- a/Firestore/Source/Model/FSTDocumentSet.mm +++ /dev/null @@ -1,131 +0,0 @@ -/* - * Copyright 2017 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 - -#import "Firestore/Source/Model/FSTDocumentSet.h" - -#import "Firestore/Source/Model/FSTDocument.h" -#import "Firestore/third_party/Immutable/FSTImmutableSortedSet.h" - -#include "Firestore/core/src/firebase/firestore/immutable/sorted_set.h" -#include "Firestore/core/src/firebase/firestore/model/document_key.h" -#include "Firestore/core/src/firebase/firestore/util/delayed_constructor.h" -#include "Firestore/core/src/firebase/firestore/util/objc_compatibility.h" -#include "Firestore/core/src/firebase/firestore/util/string_apple.h" - -namespace objc = firebase::firestore::util::objc; -namespace util = firebase::firestore::util; -using firebase::firestore::immutable::SortedSet; -using firebase::firestore::model::DocumentMap; -using firebase::firestore::model::DocumentKey; -using firebase::firestore::util::DelayedConstructor; - -NS_ASSUME_NONNULL_BEGIN - -using firebase::firestore::model::DocumentSetComparator; - -@implementation FSTDocumentSet { - DelayedConstructor _delegate; -} - -+ (instancetype)documentSetWithComparator:(NSComparator)comparator { - DocumentSet wrapped{comparator}; - return [[FSTDocumentSet alloc] initWithDocumentSet:std::move(wrapped)]; -} - -- (instancetype)initWithDocumentSet:(DocumentSet &&)documentSet { - self = [super init]; - if (self) { - _delegate.Init(std::move(documentSet)); - } - return self; -} - -- (BOOL)isEqual:(id)other { - if (other == self) return YES; - if (![other isMemberOfClass:[FSTDocumentSet class]]) return NO; - - FSTDocumentSet *otherSet = (FSTDocumentSet *)other; - return *_delegate == *(otherSet->_delegate); -} - -- (NSUInteger)hash { - return _delegate->Hash(); -} - -- (NSString *)description { - return util::WrapNSString(_delegate->ToString()); -} - -- (NSUInteger)count { - return _delegate->size(); -} - -- (BOOL)isEmpty { - return _delegate->empty(); -} - -- (BOOL)containsKey:(const DocumentKey &)key { - return _delegate->ContainsKey(key); -} - -- (FSTDocument *_Nullable)documentForKey:(const DocumentKey &)key { - return _delegate->GetDocument(key); -} - -- (FSTDocument *_Nullable)firstDocument { - return _delegate->GetFirstDocument(); -} - -- (FSTDocument *_Nullable)lastDocument { - return _delegate->GetLastDocument(); -} - -- (NSUInteger)indexOfKey:(const DocumentKey &)key { - size_t index = _delegate->IndexOf(key); - return index != DocumentSet::npos ? index : NSNotFound; -} - -- (const DocumentSet &)documents { - return *_delegate; -} - -- (NSArray *)arrayValue { - NSMutableArray *result = [NSMutableArray arrayWithCapacity:self.count]; - for (FSTDocument *doc : *_delegate) { - [result addObject:doc]; - } - return result; -} - -- (const DocumentMap &)mapValue { - return _delegate->GetMapValue(); -} - -- (instancetype)documentSetByAddingDocument:(FSTDocument *_Nullable)document { - DocumentSet result = _delegate->insert(document); - return [[FSTDocumentSet alloc] initWithDocumentSet:std::move(result)]; -} - -- (instancetype)documentSetByRemovingKey:(const DocumentKey &)key { - DocumentSet result = _delegate->erase(key); - return [[FSTDocumentSet alloc] initWithDocumentSet:std::move(result)]; -} - -@end - -NS_ASSUME_NONNULL_END diff --git a/Firestore/core/src/firebase/firestore/model/document_set.h b/Firestore/core/src/firebase/firestore/model/document_set.h index eb0005bd77f..3502dbd2f8d 100644 --- a/Firestore/core/src/firebase/firestore/model/document_set.h +++ b/Firestore/core/src/firebase/firestore/model/document_set.h @@ -123,18 +123,6 @@ class DocumentSet : public immutable::impl::SortedMapBase, */ size_t IndexOf(const DocumentKey& key) const; - /** - * Returns a copy of the documents in this set as an array. This is O(n) on - * the size of the set. - */ - std::vector ToVector() const; - - /** - * Returns the documents as a `DocumentMap`. This is O(1) as this leverages - * our internal representation. - */ - const DocumentMap& GetMapValue() const; - /** Returns a new DocumentSet that contains the given document. */ DocumentSet insert(FSTDocument* _Nullable document) const; diff --git a/Firestore/core/src/firebase/firestore/model/document_set.mm b/Firestore/core/src/firebase/firestore/model/document_set.mm index ff67bfcdf9b..41dfd95d8ad 100644 --- a/Firestore/core/src/firebase/firestore/model/document_set.mm +++ b/Firestore/core/src/firebase/firestore/model/document_set.mm @@ -103,14 +103,6 @@ return doc ? sorted_set_.find_index(doc) : NSNotFound; } -std::vector DocumentSet::ToVector() const { - return {sorted_set_.begin(), sorted_set_.end()}; -} - -const DocumentMap& DocumentSet::GetMapValue() const { - return index_; -} - DocumentSet DocumentSet::insert(FSTDocument* _Nullable document) const { // TODO(mcg): look into making document nonnull. if (!document) { From f46866c407197f9619c340abc5244a8b725e47b1 Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Fri, 22 Mar 2019 16:49:49 -0700 Subject: [PATCH 06/11] Add Objective-C++ guard --- Firestore/core/src/firebase/firestore/model/document_set.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Firestore/core/src/firebase/firestore/model/document_set.h b/Firestore/core/src/firebase/firestore/model/document_set.h index 3502dbd2f8d..7365ecdb369 100644 --- a/Firestore/core/src/firebase/firestore/model/document_set.h +++ b/Firestore/core/src/firebase/firestore/model/document_set.h @@ -17,6 +17,10 @@ #ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_DOCUMENT_SET_H_ #define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_DOCUMENT_SET_H_ +#if !defined(__OBJC__) +#error "This header only supports Objective-C++" +#endif // !defined(__OBJC__) + #import #include From 3f45e4451b86435718583e19fc46dd9260906459 Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Fri, 22 Mar 2019 16:50:21 -0700 Subject: [PATCH 07/11] Use absl::c_equal instead of hand-rolling it. --- .../firebase/firestore/model/document_set.mm | 25 ++++--------------- 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/Firestore/core/src/firebase/firestore/model/document_set.mm b/Firestore/core/src/firebase/firestore/model/document_set.mm index 41dfd95d8ad..d27056fa4f0 100644 --- a/Firestore/core/src/firebase/firestore/model/document_set.mm +++ b/Firestore/core/src/firebase/firestore/model/document_set.mm @@ -24,6 +24,7 @@ #include "Firestore/core/src/firebase/firestore/immutable/sorted_set.h" #include "Firestore/core/src/firebase/firestore/model/document_key.h" #include "Firestore/core/src/firebase/firestore/util/objc_compatibility.h" +#include "absl/algorithm/container.h" NS_ASSUME_NONNULL_BEGIN @@ -39,26 +40,10 @@ } bool operator==(const DocumentSet& lhs, const DocumentSet& rhs) { - if (lhs.size() != rhs.size()) { - return false; - } - - auto left_iter = lhs.sorted_set_.begin(); - auto left_end = lhs.sorted_set_.end(); - - auto right_iter = rhs.sorted_set_.begin(); - auto right_end = rhs.sorted_set_.end(); - - while (left_iter != left_end && right_iter != right_end) { - FSTDocument* left_doc = *left_iter; - FSTDocument* right_doc = *right_iter; - if (![left_doc isEqual:right_doc]) { - return NO; - } - ++left_iter; - ++right_iter; - } - return YES; + return absl::c_equal(lhs.sorted_set_, rhs.sorted_set_, + [](FSTDocument* left_doc, FSTDocument* right_doc) { + return [left_doc isEqual:right_doc]; + }); } std::string DocumentSet::ToString() const { From c4fbcfe6bb4beb1411bb409d4c1f010666e4b0a9 Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Sat, 23 Mar 2019 07:08:57 -0700 Subject: [PATCH 08/11] Split SortedContainer out of SortedMapBase ... and use it --- .../firestore/immutable/array_sorted_map.h | 2 +- .../firebase/firestore/immutable/llrb_node.h | 2 +- ...sorted_map_base.cc => sorted_container.cc} | 6 +-- .../{sorted_map_base.h => sorted_container.h} | 39 ++++++++++++------- .../firebase/firestore/immutable/sorted_map.h | 4 +- .../firebase/firestore/immutable/sorted_set.h | 4 +- .../firestore/immutable/tree_sorted_map.h | 2 +- .../firebase/firestore/model/document_set.h | 3 +- .../firestore/immutable/sorted_map_test.cc | 2 - .../firestore/immutable/sorted_set_test.cc | 5 +-- 10 files changed, 37 insertions(+), 32 deletions(-) rename Firestore/core/src/firebase/firestore/immutable/{sorted_map_base.cc => sorted_container.cc} (89%) rename Firestore/core/src/firebase/firestore/immutable/{sorted_map_base.h => sorted_container.h} (77%) diff --git a/Firestore/core/src/firebase/firestore/immutable/array_sorted_map.h b/Firestore/core/src/firebase/firestore/immutable/array_sorted_map.h index a88c1bdf7ff..1a7076d0e8d 100644 --- a/Firestore/core/src/firebase/firestore/immutable/array_sorted_map.h +++ b/Firestore/core/src/firebase/firestore/immutable/array_sorted_map.h @@ -27,7 +27,7 @@ #include "Firestore/core/src/firebase/firestore/immutable/keys_view.h" #include "Firestore/core/src/firebase/firestore/immutable/map_entry.h" -#include "Firestore/core/src/firebase/firestore/immutable/sorted_map_base.h" +#include "Firestore/core/src/firebase/firestore/immutable/sorted_container.h" #include "Firestore/core/src/firebase/firestore/util/comparison.h" #include "Firestore/core/src/firebase/firestore/util/hard_assert.h" #include "Firestore/core/src/firebase/firestore/util/range.h" diff --git a/Firestore/core/src/firebase/firestore/immutable/llrb_node.h b/Firestore/core/src/firebase/firestore/immutable/llrb_node.h index 80c2d865455..0046580364e 100644 --- a/Firestore/core/src/firebase/firestore/immutable/llrb_node.h +++ b/Firestore/core/src/firebase/firestore/immutable/llrb_node.h @@ -21,7 +21,7 @@ #include #include "Firestore/core/src/firebase/firestore/immutable/llrb_node_iterator.h" -#include "Firestore/core/src/firebase/firestore/immutable/sorted_map_base.h" +#include "Firestore/core/src/firebase/firestore/immutable/sorted_container.h" namespace firebase { namespace firestore { diff --git a/Firestore/core/src/firebase/firestore/immutable/sorted_map_base.cc b/Firestore/core/src/firebase/firestore/immutable/sorted_container.cc similarity index 89% rename from Firestore/core/src/firebase/firestore/immutable/sorted_map_base.cc rename to Firestore/core/src/firebase/firestore/immutable/sorted_container.cc index 954bdb9cd73..637270d3f71 100644 --- a/Firestore/core/src/firebase/firestore/immutable/sorted_map_base.cc +++ b/Firestore/core/src/firebase/firestore/immutable/sorted_container.cc @@ -14,18 +14,16 @@ * limitations under the License. */ -#include "Firestore/core/src/firebase/firestore/immutable/sorted_map_base.h" +#include "Firestore/core/src/firebase/firestore/immutable/sorted_container.h" namespace firebase { namespace firestore { namespace immutable { -namespace impl { // Define external storage for constants: +constexpr SortedContainer::size_type SortedContainer::npos; constexpr SortedMapBase::size_type SortedMapBase::kFixedSize; -constexpr SortedMapBase::size_type SortedMapBase::npos; -} // namespace impl } // namespace immutable } // namespace firestore } // namespace firebase diff --git a/Firestore/core/src/firebase/firestore/immutable/sorted_map_base.h b/Firestore/core/src/firebase/firestore/immutable/sorted_container.h similarity index 77% rename from Firestore/core/src/firebase/firestore/immutable/sorted_map_base.h rename to Firestore/core/src/firebase/firestore/immutable/sorted_container.h index a19bd778830..1cf5eb79c5c 100644 --- a/Firestore/core/src/firebase/firestore/immutable/sorted_map_base.h +++ b/Firestore/core/src/firebase/firestore/immutable/sorted_container.h @@ -14,25 +14,25 @@ * limitations under the License. */ -#ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_IMMUTABLE_SORTED_MAP_BASE_H_ -#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_IMMUTABLE_SORTED_MAP_BASE_H_ +#ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_IMMUTABLE_SORTED_CONTAINER_H_ +#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_IMMUTABLE_SORTED_CONTAINER_H_ #include namespace firebase { namespace firestore { namespace immutable { -namespace impl { /** - * A base class for implementing sorted maps, containing types and constants - * that don't depend upon the template parameters to the main class. + * A base class for implementing immutable sorted containers, containing types + * and constants that don't depend upon the template parameters to the main + * class. * * Note that this exists as a base class rather than as just a namespace in * order to make it possible for users of the SortedMap classes to avoid needing * to declare storage for each instantiation of the template. */ -class SortedMapBase { +class SortedContainer { public: /** * The type of size() methods on immutable collections. Note: @@ -42,6 +42,23 @@ class SortedMapBase { */ using size_type = uint32_t; + /** + * A sentinel return value that indicates not found. Functionally similar to + * std::string::npos. + */ + static constexpr size_type npos = static_cast(-1); +}; + +/** + * A base class for implementing sorted maps, containing types and constants + * that don't depend upon the template parameters to the main class. + * + * Note that this exists as a base class rather than as just a namespace in + * order to make it possible for users of the SortedMap classes to avoid needing + * to declare storage for each instantiation of the template. + */ +class SortedMapBase : public SortedContainer { + public: /** * The maximum size of an ArraySortedMap. * @@ -52,19 +69,11 @@ class SortedMapBase { * inserting and lookups. Feel free to empirically determine this constant, * but don't expect much gain in real world performance. */ - // TODO(wilhuff): actually use this for switching implementations. static constexpr size_type kFixedSize = 25; - - /** - * A sentinel return value that indicates not found. Functionally similar to - * std::string::npos. - */ - static constexpr size_type npos = static_cast(-1); }; -} // namespace impl } // namespace immutable } // namespace firestore } // namespace firebase -#endif // FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_IMMUTABLE_SORTED_MAP_BASE_H_ +#endif // FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_IMMUTABLE_SORTED_CONTAINER_H_ diff --git a/Firestore/core/src/firebase/firestore/immutable/sorted_map.h b/Firestore/core/src/firebase/firestore/immutable/sorted_map.h index dc50bbd2bc1..f3c998bd80e 100644 --- a/Firestore/core/src/firebase/firestore/immutable/sorted_map.h +++ b/Firestore/core/src/firebase/firestore/immutable/sorted_map.h @@ -21,7 +21,7 @@ #include "Firestore/core/src/firebase/firestore/immutable/array_sorted_map.h" #include "Firestore/core/src/firebase/firestore/immutable/keys_view.h" -#include "Firestore/core/src/firebase/firestore/immutable/sorted_map_base.h" +#include "Firestore/core/src/firebase/firestore/immutable/sorted_container.h" #include "Firestore/core/src/firebase/firestore/immutable/sorted_map_iterator.h" #include "Firestore/core/src/firebase/firestore/immutable/tree_sorted_map.h" #include "Firestore/core/src/firebase/firestore/util/comparison.h" @@ -36,7 +36,7 @@ namespace immutable { * has methods to efficiently create new maps that are mutations of it. */ template > -class SortedMap : public impl::SortedMapBase { +class SortedMap : public SortedMapBase { public: using key_type = K; using mapped_type = V; diff --git a/Firestore/core/src/firebase/firestore/immutable/sorted_set.h b/Firestore/core/src/firebase/firestore/immutable/sorted_set.h index b055e914dcf..36b6e16ef03 100644 --- a/Firestore/core/src/firebase/firestore/immutable/sorted_set.h +++ b/Firestore/core/src/firebase/firestore/immutable/sorted_set.h @@ -20,8 +20,8 @@ #include #include +#include "Firestore/core/src/firebase/firestore/immutable/sorted_container.h" #include "Firestore/core/src/firebase/firestore/immutable/sorted_map.h" -#include "Firestore/core/src/firebase/firestore/immutable/sorted_map_base.h" #include "Firestore/core/src/firebase/firestore/util/comparison.h" #include "Firestore/core/src/firebase/firestore/util/hard_assert.h" #include "Firestore/core/src/firebase/firestore/util/hashing.h" @@ -46,7 +46,7 @@ template , typename V = impl::Empty, typename M = SortedMap> -class SortedSet : public impl::SortedMapBase { +class SortedSet : public SortedContainer { public: using size_type = typename M::size_type; using value_type = K; diff --git a/Firestore/core/src/firebase/firestore/immutable/tree_sorted_map.h b/Firestore/core/src/firebase/firestore/immutable/tree_sorted_map.h index 9fd51c33643..9a1635c2902 100644 --- a/Firestore/core/src/firebase/firestore/immutable/tree_sorted_map.h +++ b/Firestore/core/src/firebase/firestore/immutable/tree_sorted_map.h @@ -26,7 +26,7 @@ #include "Firestore/core/src/firebase/firestore/immutable/keys_view.h" #include "Firestore/core/src/firebase/firestore/immutable/llrb_node.h" #include "Firestore/core/src/firebase/firestore/immutable/map_entry.h" -#include "Firestore/core/src/firebase/firestore/immutable/sorted_map_base.h" +#include "Firestore/core/src/firebase/firestore/immutable/sorted_container.h" #include "Firestore/core/src/firebase/firestore/util/comparator_holder.h" #include "Firestore/core/src/firebase/firestore/util/comparison.h" diff --git a/Firestore/core/src/firebase/firestore/model/document_set.h b/Firestore/core/src/firebase/firestore/model/document_set.h index 7365ecdb369..e069fc65c82 100644 --- a/Firestore/core/src/firebase/firestore/model/document_set.h +++ b/Firestore/core/src/firebase/firestore/model/document_set.h @@ -28,6 +28,7 @@ #include #include +#include "Firestore/core/src/firebase/firestore/immutable/sorted_container.h" #include "Firestore/core/src/firebase/firestore/immutable/sorted_set.h" #include "Firestore/core/src/firebase/firestore/model/document_key.h" #include "Firestore/core/src/firebase/firestore/model/document_map.h" @@ -65,7 +66,7 @@ class DocumentSetComparator { * comparator on top of what is provided to guarantee document equality based on * the key. */ -class DocumentSet : public immutable::impl::SortedMapBase, +class DocumentSet : public immutable::SortedContainer, public util::Equatable { public: /** diff --git a/Firestore/core/test/firebase/firestore/immutable/sorted_map_test.cc b/Firestore/core/test/firebase/firestore/immutable/sorted_map_test.cc index acd06429a47..2c26c45b403 100644 --- a/Firestore/core/test/firebase/firestore/immutable/sorted_map_test.cc +++ b/Firestore/core/test/firebase/firestore/immutable/sorted_map_test.cc @@ -36,8 +36,6 @@ #include "Firestore/core/test/firebase/firestore/immutable/testing.h" #include "gtest/gtest.h" -using firebase::firestore::immutable::impl::SortedMapBase; - namespace firebase { namespace firestore { namespace immutable { diff --git a/Firestore/core/test/firebase/firestore/immutable/sorted_set_test.cc b/Firestore/core/test/firebase/firestore/immutable/sorted_set_test.cc index a4b337cb166..3239fdafe8b 100644 --- a/Firestore/core/test/firebase/firestore/immutable/sorted_set_test.cc +++ b/Firestore/core/test/firebase/firestore/immutable/sorted_set_test.cc @@ -21,13 +21,12 @@ #include "Firestore/core/test/firebase/firestore/immutable/testing.h" -using firebase::firestore::immutable::impl::SortedMapBase; -using SizeType = SortedMapBase::size_type; - namespace firebase { namespace firestore { namespace immutable { +using SizeType = SortedContainer::size_type; + template SortedSet ToSet(const std::vector& container) { SortedSet result; From 114fe059438965d1359f74d9b189244a43273fcb Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Fri, 22 Mar 2019 18:12:09 -0700 Subject: [PATCH 09/11] Remove = default constructors that aren't true. These are implicitly deleted because DocumentSet is a member and isn't default constructible. --- Firestore/core/src/firebase/firestore/api/query_snapshot.h | 2 -- Firestore/core/src/firebase/firestore/core/view_snapshot.h | 2 -- 2 files changed, 4 deletions(-) diff --git a/Firestore/core/src/firebase/firestore/api/query_snapshot.h b/Firestore/core/src/firebase/firestore/api/query_snapshot.h index 50b7ed24802..7c5f49dde79 100644 --- a/Firestore/core/src/firebase/firestore/api/query_snapshot.h +++ b/Firestore/core/src/firebase/firestore/api/query_snapshot.h @@ -44,8 +44,6 @@ namespace api { */ class QuerySnapshot { public: - QuerySnapshot() = default; - QuerySnapshot(Firestore* firestore, FSTQuery* query, core::ViewSnapshot&& snapshot, diff --git a/Firestore/core/src/firebase/firestore/core/view_snapshot.h b/Firestore/core/src/firebase/firestore/core/view_snapshot.h index a4d9619d6f9..279ea706952 100644 --- a/Firestore/core/src/firebase/firestore/core/view_snapshot.h +++ b/Firestore/core/src/firebase/firestore/core/view_snapshot.h @@ -111,8 +111,6 @@ using ViewSnapshotHandler = */ class ViewSnapshot { public: - ViewSnapshot() = default; - ViewSnapshot(FSTQuery* query, model::DocumentSet documents, model::DocumentSet old_documents, From 03847dc2965838d1757987fb1c31a0a522aa771e Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Fri, 22 Mar 2019 17:52:28 -0700 Subject: [PATCH 10/11] Fix NSNotFound -> npos --- Firestore/core/src/firebase/firestore/model/document_set.h | 2 +- Firestore/core/src/firebase/firestore/model/document_set.mm | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Firestore/core/src/firebase/firestore/model/document_set.h b/Firestore/core/src/firebase/firestore/model/document_set.h index e069fc65c82..003f1050a3c 100644 --- a/Firestore/core/src/firebase/firestore/model/document_set.h +++ b/Firestore/core/src/firebase/firestore/model/document_set.h @@ -124,7 +124,7 @@ class DocumentSet : public immutable::SortedContainer, /** * Returns the index of the document with the provided key in the document - * set. Returns NSNotFound if the key is not present. + * set. Returns `npos` if the key is not present. */ size_t IndexOf(const DocumentKey& key) const; diff --git a/Firestore/core/src/firebase/firestore/model/document_set.mm b/Firestore/core/src/firebase/firestore/model/document_set.mm index d27056fa4f0..47cb3d263dd 100644 --- a/Firestore/core/src/firebase/firestore/model/document_set.mm +++ b/Firestore/core/src/firebase/firestore/model/document_set.mm @@ -85,7 +85,7 @@ size_t DocumentSet::IndexOf(const DocumentKey& key) const { FSTDocument* doc = GetDocument(key); - return doc ? sorted_set_.find_index(doc) : NSNotFound; + return doc ? sorted_set_.find_index(doc) : npos; } DocumentSet DocumentSet::insert(FSTDocument* _Nullable document) const { From 9d86b35e4171dfcb319d8b4ce7b82d20568c0501 Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Fri, 22 Mar 2019 18:12:57 -0700 Subject: [PATCH 11/11] Fix usage of FSTQuery.limit --- Firestore/Source/Core/FSTView.mm | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Firestore/Source/Core/FSTView.mm b/Firestore/Source/Core/FSTView.mm index a0687767706..079c547ad27 100644 --- a/Firestore/Source/Core/FSTView.mm +++ b/Firestore/Source/Core/FSTView.mm @@ -273,7 +273,7 @@ - (FSTViewDocumentChanges *)computeChangesWithDocuments:(const MaybeDocumentMap // Note that this should never get used in a refill (when previousChanges is set), because there // will only be adds -- no deletes or updates. FSTDocument *_Nullable lastDocInLimit = - (self.query.limit && oldDocumentSet.size() == self.query.limit) + (self.query.limit != NSNotFound && oldDocumentSet.size() == self.query.limit) ? oldDocumentSet.GetLastDocument() : nil; @@ -351,8 +351,8 @@ - (FSTViewDocumentChanges *)computeChangesWithDocuments:(const MaybeDocumentMap } } - if (self.query.limit) { - for (long i = newDocumentSet.size() - self.query.limit; i > 0; --i) { + if (self.query.limit != NSNotFound && newDocumentSet.size() > self.query.limit) { + for (size_t i = newDocumentSet.size() - self.query.limit; i > 0; --i) { FSTDocument *oldDoc = newDocumentSet.GetLastDocument(); newDocumentSet = newDocumentSet.erase(oldDoc.key); newMutatedKeys = newMutatedKeys.erase(oldDoc.key);