Skip to content

merge master into firestore-api-changes #1095

New issue

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

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

Already on GitHub? Sign in to your account

Merged
merged 366 commits into from
Apr 13, 2018

Conversation

rsgowman
Copy link
Member

No description provided.

rsgowman and others added 30 commits February 12, 2018 13:20
Incomplete, and what does exist in still slightly vague. It's expected
that this will change.
* update FieldValue of type Reference

* address change

* fix bad path string literal in test

* use ReferenceValue and qualified name
* implement SnapshotVersion and test

* update project

* implement MaybeDocument and test

* move snapshot-version from core to model

* fix a bug

* implement Document and test

* implement NoDocument

* adding type tag and fix style

* fix a few bugs, discovered after merging and test run.

* add assert to check FieldValue type and more test for comparision.

* address changes

* allow moving FieldValue to construct Document.

* address changes

* add document tests to project

* use std::less convention

* make Type::Unknown static initializer
* Implement schema versions

* Style fixes

* newlines, copyrights, assumptions

* Fix nullability

* Raw ptr -> shared_ptr

* kVersionTableGlobal -> kVersionGlobalTable

* Drop utils, move into static methods

* Drop extra include

* Add a few more comments

* Move version constant into migrations file

* formatting?

* Fix comment

* Split add and update queryData

* Work on adding targetCount

* More work on count

* Using shared_ptr

* Implement count for query cache

* use quotes

* Add cast

* Styling

* Revert year bump in copyright

* Add adversarial key to migration test

* Add comment

* Fix style
Cleaning up implicit retain for the RTDB and Storage
* Build (grpc's) nanopb with -DPB_FIELD_16BIT

We require (at least) 16 bit fields. (By default, nanopb uses 8 bit
fields, ie allowing up to 256 field tags.)

Also note that this patch adds this to grpc's nanopb, rather than to our
nanopb. We'll need to eventually either:
a) we instruct grpc to use our nanopb
b) we rely on grpc's nanopb instead of using our own.

(^ marked as a TODO for now.)

* Add some dependant protos

Imported from protobuf. Nanopb requires these to be present (though
anything using libprotobuf does not, as these are already built into
that.)

* Add generated nanopb protos based off of newly added proto definitions

* Build the nanopb protos

* Serialize and deserialize null
Add code markup to a few paths for consistency.
* replacing Auth/FSTUser by C++ auth implementation

* address changes
Basically a port of firebase/firebase-js-sdk@a1e346f and firebase/firebase-js-sdk@fce4168

* Introduces a DelayedCallback helper class in FSTDispatchQueue to encapsulate delayed callback logic.
* Adds cancellation support.
* Updates the idle timer in FSTStream to use new cancellation support.
* Adds a FSTTimerId enum for identifying delayed operations on the queue and uses it to identify our existing backoff and idle timers.
* Added containsDelayedCallback: and runDelayedCallbacksUntil: methods to FSTDispatchQueue which can be used from tests to check for the presence of a callback or to schedule them to run early.
* Removes FSTTestDispatchQueue and changes idle tests to use new test methods.
In order to use different cflags for abseil, this patch splits it out
into a subspec within the pod.

The cmake side of things "just works" since Firestore/CMakeLists.txt
includes abseil before setting our compiler flags.
absl includes code like this:
```
void fn(std::initializer_list<absl::string_view> pieces) {
  ...
  for (const absl::string_view piece : pieces) total_size += piece.size();
```

clang objects, suggesting that a reference should be used instead, i.e.:
```
  for (const absl::string_view& piece : pieces) total_size += piece.size();
```

But:
a) we don't want to touch absl code
b) string_views are cheap to copy (and absl recommends copying
string_views rather than taking references as it may result in smaller
code)
c) some brief, naive benchmarking suggests there's no significant
different in this case (i.e. (b) is correct.)

Note that -Wrange-loop-analysis is already exlicitly enabled in our
cmake build.
* replacing Auth/FSTUser by C++ auth implementation

* address changes

* replacing FSTGetTokenResult by C++ Token implementation

* address changes

* address changes

* fix another const& v.s. dispatch bug

* fix more const& v.s. dispatch bug  zxu123 committed

* fix

* passing by value in callback
* Fix b/73167987: Upon receiving a permanent write error when we had additional
  pendingWrites to send, we were restarting the stream with a new delegate and
  then immediately setting the delegate to nil, causing the stream to ignore
  all GRPC events for the stream.
* Fix b/73382103: We were attempting to gracefully teardown the write stream (i.e.
  send an empty WriteRequest) even when the stream was already failed due
  to an error. This caused no harm other than log pollution, but I fixed it.
* Use -[GRPCCall setResponseDispatchQueue] to dispatch GRPC callbacks directly onto the Firestore worker queue. This saves a double-dispatch and simplifies our logic.
* Add stricter assertions regarding stream state now that dispatch queue / callback filter race conditions are eliminated.
* [En|De]codeUnsignedVarint -> [En|De]codeVarint

The 'unsigned' portion was misleading, as these varints work with both
signed and unsigned integers. (The 'signed' varints also work with both
signed and unsigned integers, but use zig-zag encoding so that negative
numbers are encoded more efficiently. Note that 'signed' varints aren't
used in the Value proto, so won't appear in the serializer class for at
least the short term.)

Added some docstrings to help disambiguate this.
- FSTTimestamp is now FIRTimestamp, under Firestore/Source/{Public,API}. This is a temporary solution; eventually, FIRTimestamp is supposed to live somewhere under Firebase;
- move most internal Timestamp methods to the public header (the only exception is ISOString).
Xcode has starting warning about us implicitly retaining self references
within blocks. This commit fixes it by explicitly mentioning self. No
real changes are introduced here; this is effectively just making
implicit behaviour explicit.
zxu123 and others added 22 commits April 10, 2018 16:28
* port FieldMask to C++

* address changes

* address changes

* fix test

* address change

* Port transform operations (FSTTransformOperation, FSTServerTimestampTransform) to C++

* address changes

* address changes

* address changes

* implement `FieldTransform` in C++

* port `FieldTransform`

* make `fieldTransforms` shared inside `context`

* address changes

* fix lint

* address changes
Remove a duplicate macro definition and clean up an extra const clang-tidy complains about.
* remove deprecated data message callback
* Fix the issue that swizzling is not setup in recommended data message callback for message tracking.
operationInProgress is accessed from both the main thread and from
libdispatch on some other thread. Make it atomic to avoid a data race.
Also reorder assertion checks to only access operationInProgress
after making sure the function is running on the queue.

Tested: ran unit tests using old and new versions under Thread
Sanitizer, verified that TSan reports a data race for the old version,
but finds no issues with the new version.
…files

exposing FIRAppEnvironmentUtil functionalities
Example run: `cmake -DWITH_ASAN=ON ..` (from Firestore build folder)
* Add a .clang-tidy configuration for Firestore C++

* Fix clang-tidy warnings

  * typedef -> using
  * const ref + rvalue ref -> pass by value
  * NULL -> nullptr
  * remove useless default initializations
  * remove useless const value-type parameter declarations (definitions
    can still use them)
  * use auto instead of repeating types in a cast

* Fix typos

* Address use of static method through instance warnings

* Address use after move warnings
* port FieldMask to C++

* address changes

* address changes

* fix test

* address change

* Port transform operations (FSTTransformOperation, FSTServerTimestampTransform) to C++

* address changes

* address changes

* address changes

* implement `FieldTransform` in C++

* port `FieldTransform`

* make `fieldTransforms` shared inside `context`

* Implement Precondition in C++ w/o test yet

* add unit test for `Precondition`

* port `Precondition`

* address changes

* address changes

* fix bugs for integration test

* address changes

* fix lint

* address changes

* address changes
Getting warning message from xcode so we need to define it to remove warning.
@rsgowman
Copy link
Member Author

Note: 6cdbc99 previously merged master, but it looks like I squashed it when merging, so git lost track of which commits from master were properly merged. As a result, a naive merging of master into firestore-api-changes results in approx 10^999 conflicts. So this PR, first reverts that commit, and then merges master.

Copy link
Contributor

@wilhuff wilhuff left a comment

Choose a reason for hiding this comment

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

LGTM

(verified by checking this out and then diffing against master)

@rsgowman rsgowman merged commit 6fed632 into firestore-api-changes Apr 13, 2018
@rsgowman
Copy link
Member Author

Yeah, that's how I did my final verification of this PR too. :)

@rsgowman rsgowman deleted the rsgowman/fix_firestore-api-changes branch April 13, 2018 20:51
@firebase firebase locked and limited conversation to collaborators Nov 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.