Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

gRPC: replace Objective-C implementation with the new C++ implementation #1968

Merged
merged 32 commits into from Oct 26, 2018

Conversation

Projects
None yet
6 participants
@var-const
Copy link
Collaborator

commented Oct 18, 2018

Most of the diff comes from deleted files. I split the PR into meaningful commits (the first five) which can be treated as separate PRs (most of them cannot be merged on their own).

var-const added some commits Oct 18, 2018

gRPC: add support for SSL disabled to `GrpcConnection`.
Unfortunately, there currently is no way to verify this change actually
works.
gRPC: make gRPC calls using the C++ implementation:
* make `FSTRemoteStore` create C++ streams instead of their Objective-C
  counterparts;
* port firebase/firebase-js-sdk#1041: streams
  are never recreated, only restarted.
* make `FSTDatastore` delegate server calls to the C++ implementation.
C++: port `MockWatchStream` and `MockWriteStream` to C++.
`FSTMockDatastore` is still in place, because `Datastore` is not fully
ported yet.
test_credentials_->use_insecure_channel = true;
// TODO(varconst): hostname if necessary.
HostConfig& test_config = (*config_by_host_)[host];
test_config.use_insecure_channel = true;

This comment has been minimized.

Copy link
@rsgowman

rsgowman Oct 19, 2018

Collaborator

Perfectly fine, though you could simplify lines 242-243 to just (*config_by_host_)[host].use_insecure_channel = true; Optional.

This comment has been minimized.

Copy link
@var-const

var-const Oct 22, 2018

Author Collaborator

I don't feel strongly about it, but since it's optional, I'd prefer to leave as is. It mirrors similar code for the test certificate, and I feel there's a bit too much going for a one-liner (pointer dereference + map access + property access + assignment).

@@ -81,7 +81,6 @@
5492E07A202154D600B64F25 /* FIRTypeTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E071202154D600B64F25 /* FIRTypeTests.mm */; };
5492E07F202154EC00B64F25 /* FSTTransactionTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E07B202154EB00B64F25 /* FSTTransactionTests.mm */; };
5492E080202154EC00B64F25 /* FSTSmokeTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E07C202154EB00B64F25 /* FSTSmokeTests.mm */; };
5492E081202154EC00B64F25 /* FSTStreamTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E07D202154EB00B64F25 /* FSTStreamTests.mm */; };

This comment has been minimized.

Copy link
@rsgowman

rsgowman Oct 19, 2018

Collaborator

There's a lot of unexpected changes to this file. Is that on purpose?

This comment has been minimized.

Copy link
@var-const

var-const Oct 22, 2018

Author Collaborator

The changes are on purpose:

  • FSTStreamTests was deleted. Most of the tests it contained are covered by unit tests, and integration tests are more effort to set up, so my intuition is not to create a C++ version of this file.
  • this PR intends to delete all references to gRPC Objective-C client, so it removes the dependency on related pods (GRPCClient is the Objective-C client, ProtoRPC and RxLibrary are its dependencies).
  • As far as I can tell, directly depending upon gRPC-Core pod is unnecessary, because gRPC-C++ has a dependency on it.

Let me know if I overlooked any changes.

This comment has been minimized.

Copy link
@zxu123

zxu123 Oct 22, 2018

Collaborator

Actually FSTStreamTests is cross platform i.e. the same test cases are also in Android native client code. However, they are not based on public API. I guess it is OK to remove them for now. But my 1 cent is that integration tests and unit tests are compliment each other (we may want to add one one day).

using firebase::firestore::remote::Datastore;
using firebase::firestore::remote::GrpcConnection;
using firebase::firestore::remote::WatchStream;
using firebase::firestore::remote::WriteStream;

This comment has been minimized.

Copy link
@rsgowman

rsgowman Oct 19, 2018

Collaborator

I'm mildly surprised to see using declarations here. My preference would be for them to either:
a) be at the top of the file, with the rest of them
b) be inside the functions where they're actually used, i.e.

// not here
// using foo::bar;
int baz() {
  // here (though probably only makes sense if foo::bar isn't used elsewhere in the file)
  using foo::bar;
  ...
}

I presume you're doing this because these only really apply to @implementation FSTDatastore? I can buy that, though I suspect we'll just end up with multiple using declarations for the same symbol; some at the top, and some here.

This comment has been minimized.

Copy link
@var-const

var-const Oct 22, 2018

Author Collaborator

Thanks for spotting!

return self.open;
}
void Stop() override {
WatchStream::Stop();

This comment has been minimized.

Copy link
@rsgowman

rsgowman Oct 19, 2018

Collaborator

There's a bit of an asymmetry here; it might be ok; I haven't investigated:

The new Stop() method calls super::Stop(), but the new Start() method does not call super::Start().
The old stop/start methods don't invoke anything in the parent class.

No real action required; just ensure that this is intentional. (Also for the WriteStream below)

This comment has been minimized.

Copy link
@var-const

var-const Oct 22, 2018

Author Collaborator

I'm following Android on this. I presume the intention is that omitting Start skips auth/attempts to actually establish a connection. I think Stop is actually unnecessary too, I mostly left it in for consistency with Android.


/** Properties implemented in FSTDatastore that are nonpublic. */
@property(nonatomic, strong, readonly) FSTDispatchQueue *workerDispatchQueue;
@property(nonatomic, assign, readonly) CredentialsProvider *credentials;

@end

@implementation FSTMockDatastore
@implementation FSTMockDatastore {
std::shared_ptr<firebase::firestore::remote::MockWatchStream> _watchStream;

This comment has been minimized.

Copy link
@rsgowman

rsgowman Oct 19, 2018

Collaborator

You've used a lot of fully qualified names throughout this (non-header) file. You might consider just usinging a few extra names to cut down on them a bit. Optional.

@@ -129,15 +129,15 @@ class Stream : public GrpcStreamObserver,
*
* When start returns, `IsStarted` will return true.
*/
void Start();
virtual /*only virtual for tests*/ void Start();

This comment has been minimized.

Copy link
@rsgowman

rsgowman Oct 19, 2018

Collaborator

Another approach would be to leave as non-virtual, and adjust the behaviour via the ctor params. (Take a look at go/totw/labs/virtual-for-testing for details.) Probably not worth it here; this is fine.

This comment has been minimized.

Copy link
@var-const

var-const Oct 22, 2018

Author Collaborator

Yeah, I'd rather avoid it:

  • this approach is more similar to other platforms, where every method is implicitly virtual by default;
  • I'd prefer to minimize how much Stream knows about tests; to me, modifying the argument list/behavior seems like a more intrusive approach.

This comment has been minimized.

Copy link
@wilhuff

wilhuff Oct 23, 2018

Member

The /*only virtual for tests*/ decoration seems like overkill. Maybe just add a class-level comment about how this isn't intended to be subclassed generally but contains virtual methods for testing?

This comment has been minimized.

Copy link
@var-const

var-const Oct 23, 2018

Author Collaborator

Hmm, Stream is intended as a base class; anyway, made a class comment that all public virtual methods are for tests.

@@ -111,9 +111,6 @@ endfunction()
#
# Libprotobuf Objective-C also includes the well-known protos so they must be
# omitted here.
#
# The Objective-C client also uses the generated gRPC stubs for the Firestore
# service.

This comment has been minimized.

Copy link
@rsgowman

rsgowman Oct 19, 2018

Collaborator

If we've removed the objc grpc stubs, don't we now need the cpp ones?

This comment has been minimized.

Copy link
@var-const

var-const Oct 22, 2018

Author Collaborator

FSTDatastore uses the generated service to make the backend calls (example). In C++ Datastore, I'm using gRPC objects directly. I chose that approach primarily because it was more straightforward; I can look into code generation as a follow-up.

This comment has been minimized.

Copy link
@wilhuff

wilhuff Oct 23, 2018

Member

If we can do this without generated code that's better. The generated code is essentially dead-weight given the way we're doing our own serialization so let's avoid it.

@rsgowman rsgowman assigned var-const and unassigned rsgowman Oct 22, 2018

var-const added some commits Oct 22, 2018

@var-const

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 22, 2018

@rsgowman Rich, I added some commits to in order to load the SSL certificate, PTAL (link to diff.

test_credentials_->use_insecure_channel = true;
// TODO(varconst): hostname if necessary.
HostConfig& test_config = (*config_by_host_)[host];
test_config.use_insecure_channel = true;

This comment has been minimized.

Copy link
@var-const

var-const Oct 22, 2018

Author Collaborator

I don't feel strongly about it, but since it's optional, I'd prefer to leave as is. It mirrors similar code for the test certificate, and I feel there's a bit too much going for a one-liner (pointer dereference + map access + property access + assignment).

@@ -81,7 +81,6 @@
5492E07A202154D600B64F25 /* FIRTypeTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E071202154D600B64F25 /* FIRTypeTests.mm */; };
5492E07F202154EC00B64F25 /* FSTTransactionTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E07B202154EB00B64F25 /* FSTTransactionTests.mm */; };
5492E080202154EC00B64F25 /* FSTSmokeTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E07C202154EB00B64F25 /* FSTSmokeTests.mm */; };
5492E081202154EC00B64F25 /* FSTStreamTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E07D202154EB00B64F25 /* FSTStreamTests.mm */; };

This comment has been minimized.

Copy link
@var-const

var-const Oct 22, 2018

Author Collaborator

The changes are on purpose:

  • FSTStreamTests was deleted. Most of the tests it contained are covered by unit tests, and integration tests are more effort to set up, so my intuition is not to create a C++ version of this file.
  • this PR intends to delete all references to gRPC Objective-C client, so it removes the dependency on related pods (GRPCClient is the Objective-C client, ProtoRPC and RxLibrary are its dependencies).
  • As far as I can tell, directly depending upon gRPC-Core pod is unnecessary, because gRPC-C++ has a dependency on it.

Let me know if I overlooked any changes.

@@ -129,15 +129,15 @@ class Stream : public GrpcStreamObserver,
*
* When start returns, `IsStarted` will return true.
*/
void Start();
virtual /*only virtual for tests*/ void Start();

This comment has been minimized.

Copy link
@var-const

var-const Oct 22, 2018

Author Collaborator

Yeah, I'd rather avoid it:

  • this approach is more similar to other platforms, where every method is implicitly virtual by default;
  • I'd prefer to minimize how much Stream knows about tests; to me, modifying the argument list/behavior seems like a more intrusive approach.
@@ -111,9 +111,6 @@ endfunction()
#
# Libprotobuf Objective-C also includes the well-known protos so they must be
# omitted here.
#
# The Objective-C client also uses the generated gRPC stubs for the Firestore
# service.

This comment has been minimized.

Copy link
@var-const

var-const Oct 22, 2018

Author Collaborator

FSTDatastore uses the generated service to make the backend calls (example). In C++ Datastore, I'm using gRPC objects directly. I chose that approach primarily because it was more straightforward; I can look into code generation as a follow-up.

using firebase::firestore::remote::Datastore;
using firebase::firestore::remote::GrpcConnection;
using firebase::firestore::remote::WatchStream;
using firebase::firestore::remote::WriteStream;

This comment has been minimized.

Copy link
@var-const

var-const Oct 22, 2018

Author Collaborator

Thanks for spotting!

return self.open;
}
void Stop() override {
WatchStream::Stop();

This comment has been minimized.

Copy link
@var-const

var-const Oct 22, 2018

Author Collaborator

I'm following Android on this. I presume the intention is that omitting Start skips auth/attempts to actually establish a connection. I think Stop is actually unnecessary too, I mostly left it in for consistency with Android.

util::MakeString(defaultSettings.host),
util::MakeString(certsPath), "test_cert_2");
//GrpcConnection::UseTestCertificate(
// util::MakeString(defaultSettings.host),

This comment has been minimized.

Copy link
@var-const

var-const Oct 22, 2018

Author Collaborator

This is accidental, reverted in a later commit.

@var-const var-const assigned rsgowman and unassigned var-const Oct 22, 2018

var-const added some commits Oct 22, 2018

@zxu123

zxu123 approved these changes Oct 22, 2018

@@ -81,7 +81,6 @@
5492E07A202154D600B64F25 /* FIRTypeTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E071202154D600B64F25 /* FIRTypeTests.mm */; };
5492E07F202154EC00B64F25 /* FSTTransactionTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E07B202154EB00B64F25 /* FSTTransactionTests.mm */; };
5492E080202154EC00B64F25 /* FSTSmokeTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E07C202154EB00B64F25 /* FSTSmokeTests.mm */; };
5492E081202154EC00B64F25 /* FSTStreamTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E07D202154EB00B64F25 /* FSTStreamTests.mm */; };

This comment has been minimized.

Copy link
@zxu123

zxu123 Oct 22, 2018

Collaborator

Actually FSTStreamTests is cross platform i.e. the same test cases are also in Android native client code. However, they are not based on public API. I guess it is OK to remove them for now. But my 1 cent is that integration tests and unit tests are compliment each other (we may want to add one one day).

@var-const var-const assigned wilhuff and unassigned var-const Oct 24, 2018

[self.activeTargets removeAllObjects];
self.delegate = nil;
}
NSDictionary<FSTBoxedTargetID *, FSTQueryData *> *ActiveTargets() {

This comment has been minimized.

Copy link
@wilhuff

wilhuff Oct 25, 2018

Member

Can this method be const?

This comment has been minimized.

Copy link
@var-const

var-const Oct 25, 2018

Author Collaborator

Done.

* Returns the number of mutations that have been sent to the backend but not retrieved via
* nextSentWrite yet.
*/
int SentMutationsCount() const {

This comment has been minimized.

Copy link
@wilhuff

wilhuff Oct 25, 2018

Member

This could be sent_mutations_count to indicate that it's a cheap property.

This comment has been minimized.

Copy link
@var-const

var-const Oct 25, 2018

Author Collaborator

Done.

@@ -16,158 +16,16 @@

#import <Foundation/Foundation.h>

#import "Firestore/Source/Util/FSTDispatchQueue.h"
#import "Firestore/Source/Core/FSTTypes.h"

This comment has been minimized.

Copy link
@wilhuff

wilhuff Oct 25, 2018

Member

What's in Core/FSTTypes.h that's not in Firestore/core/src/firebase/firestore/model/types.h

This comment has been minimized.

Copy link
@var-const

var-const Oct 25, 2018

Author Collaborator

Done.

// Deliberately never deleted.
test_credentials_ = new TestCredentials{};
config_by_host_ = new ConfigByHost{};

This comment has been minimized.

Copy link
@wilhuff

wilhuff Oct 25, 2018

Member

You can avoid this leak (and simplify the code using this) by using a magic static.

Add a private method something like this:

HostConfig* ConfigForHost(const std::string& hostname, bool create) {
  static std::unordered_map<std::string, HostConfig> config_by_host;

  if (create) {
    return &config_by_host[hostname];
  } else {
    auto iter = config_by_host->find(host);
    return iter == config_by_host->end() ? nullptr : &iter->second;
  }
}

There's probably an alternative version that uses insert with a hint instead rather than using the indexing operation.

Alternatively, if you don't like the boolean for creation, do something like this:

ConfigByHost& Config() {
  static std::unordered_map<std::string, HostConfig> config_by_host;
  return config_by_host;
}

This comment has been minimized.

Copy link
@var-const

var-const Oct 25, 2018

Author Collaborator

Done (using the second approach).

The reason I did it that way is because of the style guide. However, in this case I'm quite confident config_by_host won't be accessed after it's destructed (which would be a more serious bug anyway).

This comment has been minimized.

Copy link
@wilhuff

wilhuff Oct 25, 2018

Member

Another approach would be to import google3/util/gtl/no_destructor.h into our util package and use that. However, I think this is already fine.

std::string target_name;
bool use_insecure_channel = false;
};
static TestCredentials* test_credentials_;
using ConfigByHost = std::unordered_map<std::string, HostConfig>;

This comment has been minimized.

Copy link
@wilhuff

wilhuff Oct 25, 2018

Member

Could these details be entirely private (and with static linkage) in the implementation file?

std::shared_ptr<grpc::ChannelCredentials> CreateSslCredentials(
const Path& certificate_path) {
grpc::SslCredentialsOptions options;
options.pem_root_certs = LoadCertificate(certificate_path);

This comment has been minimized.

Copy link
@wilhuff

wilhuff Oct 25, 2018

Member

It seems like means we're going to load this certificate every time we create a channel. Is that how this worked in the Objective-C client we're replacing?

This comment has been minimized.

Copy link
@var-const

var-const Oct 25, 2018

Author Collaborator

No, gRPC Objective-C client stores the certificate in a static variable.

I'm totally fine with doing it that way. My reasoning was this: channel is created infrequently. It's created once for every new GrpcConnection object, whose lifetime is tied to FSTDatastore (so IIUC, it will only be recreated if the whole Firestore instance is recreated). It may also be recreated if it gets into GRPC_CHANNEL_SHUTDOWN state. I can't find any docs on the state; from digging through the code, it seems like it's very rare and seems to be mostly in case of gRPC bugs. On the other hand, certificate is approximately 260 KB to hold in memory -- not huge, but not tiny. All in all, it's a tradeoff. What do you think? I'm fine either way.

This comment has been minimized.

Copy link
@wilhuff

wilhuff Oct 25, 2018

Member

The ssl credential options object lives for the life of the channel anyway, so the bytes are already stuck in RAM. Keeping them separately is just wasteful. If we retain anything between attempts it should be std::shared_ptr<grpc::ChannelCredentials>, but you're right--we don't recreate these often often enough to make this worthwhile.

@wilhuff wilhuff assigned var-const and unassigned wilhuff Oct 25, 2018

@wilhuff

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

Basically LGTM, though I think we should try to avoid the memory leak. People will see it in Instruments.

var-const added some commits Oct 25, 2018

@var-const var-const assigned wilhuff and unassigned var-const Oct 25, 2018

@wilhuff
Copy link
Member

left a comment

LGTM

std::shared_ptr<grpc::ChannelCredentials> CreateSslCredentials(
const Path& certificate_path) {
grpc::SslCredentialsOptions options;
options.pem_root_certs = LoadCertificate(certificate_path);

This comment has been minimized.

Copy link
@wilhuff

wilhuff Oct 25, 2018

Member

The ssl credential options object lives for the life of the channel anyway, so the bytes are already stuck in RAM. Keeping them separately is just wasteful. If we retain anything between attempts it should be std::shared_ptr<grpc::ChannelCredentials>, but you're right--we don't recreate these often often enough to make this worthwhile.

@var-const var-const assigned var-const and unassigned wilhuff Oct 26, 2018

@var-const var-const merged commit a514bd9 into master Oct 26, 2018

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details

wilhuff added a commit that referenced this pull request Oct 31, 2018

rsgowman added a commit that referenced this pull request Nov 1, 2018

style.sh && merge master (#2023)
* Update versions for Release 5.11.0

* Update Firestore Generated Protos (#1972)

* Remove cruft from project file (#1975)

* Lock reads and writes to session map

* Changed FDL to use compatible __typeof__() operator. (#1982)

typeof() is not supported in some C language dialects which causes
issues when integrating FDL in projects that do use non-GNU compiler modes.

For example, this causes problems when building in some versions of Unity
firebase/quickstart-unity#228

* Changed FDL to use compatible __typeof__() operator. (#1982)

typeof() is not supported in some C language dialects which causes
issues when integrating FDL in projects that do use non-GNU compiler modes.

For example, this causes problems when building in some versions of Unity
firebase/quickstart-unity#228

* Bump DynamicLinks patch version

* Add FDL to release manifest

* Bump GoogleUtilities patch version to deliver fix for #1964 (#1987)

* Wrap diagnostics notification in collection flag check. (#1979)

* Wrap diagnostics notification in collection flag check.

Some of the diagnostics notifications were missed and not covered by
the data collection flag.

* Remove redundant notification call, move Core diagnostics API call.

* Removed configure with no options test.

* Queue Storage Tasks on separate queue (#1981)

* Increase timeout for testDispatchAfterDelay (#1988)

* Update Storage Changelog (#1989)

* Add missing FirebaseStorage release note (#1990)

* Allow semicolon in file name (#1991)

*  Remove unnecessary notification flag (#1993)

* Remove unnecessary notification flag.

This was added when the Google pod could configure Firebase but the
Google pod is deprecated and can only work with Firebase 3.X. These
flags and conditional checks can be safely removed.

* Resolve issues from commit split.

* Style fixes.

* Reduce singleton usage in FIRApp tests. (#1995)

* Reduce singleton usage in FIRApp tests.

There have been some issues while creating new tests of conflicts with
mocks of classes and instances, this should alleviate some of those
conflicts in the future.

* Remove bad style changes.

* Use default app name flag instead of local variable.

* Comply with c99 standard (#1992)

* Trigger travis for Firestore podspec changes

* Use C99-compatible __typeof__ instead of typeof (#1985)

`typeof` is only defined if you compile with GNU extensions, while
`__typeof__` is always available.

This is the Firestore equivalent of #1982.

Note that Firestore won't yet build in this mode because among other
things the Objective-C gRPC still uses `typeof`. Once we eliminate that
dependency this might become possible.

* Adding AppCode Diff (#1996)

* Remove warning (#1999)

* Add InAppMessaging to Carthage template (#2006)

* Restore SafariServices framework (#2002)

* SafariServices not available on tvOS and not used on osx

* Force Firestore to conform to C99 and C++11. (#2001)

Note that c++0x is how Xcode spells c++11.

Also fix an issue where we were accidentally using a C++14 feature.

* Changing the internal testing repo (#2003)

* Clean up test. The issue has already been fixed and we are now running integration test with actual server instead of hexa. (#2007)

* gRPC: replace Objective-C implementation with the new C++ implementation (#1968)

* add support for SSL disabled to `GrpcConnection` (unfortunately, there
  currently is no way to verify this change actually works);
* make gRPC calls using the C++ implementation:
* make `FSTRemoteStore` create C++ streams instead of their Objective-C
  counterparts;
* port firebase/firebase-js-sdk#1041: streams
  are now never recreated, only restarted;
* make `FSTDatastore` delegate server calls to the C++ implementation;
* port `MockWatchStream` and `MockWriteStream` to C++ (`FSTMockDatastore` is
  still in place, because `Datastore` is not fully ported yet);
* no longer generate Objective-C gRPC service definitions from protos;
* remove all references to Objective-C gRPC client;
* check in gRPC root certificates file and load it at runtime (the check-in part
  is temporary until gRPC-C++.podspec provides the certificate). This makes SSL
  work.

* Add component system documentation.

* Fixed markdown layout issue.

* Remove trailing whitespaces.

* Add table of contents.

* Remove extra parentheses from TOC.

* Renamed SDKs to frameworks and products for accuracy.

* Updated the Carthage installation instructions (#2012)

* Add Rome instructions (#2014)

* Attempt to fix frequent Auth Unit Test flake on OSX (#2017)

* Increase timeouts in attempt to eliminate travis flakes (#2016)

* Don't rely on test always being in foreground (#2021)

* Fix log overflow in continuous fuzz testing (#2020)

Prevent generating too many "Unrecognized selector" console messages that eventually make Travis kill the job due to log exceeding limits.

* style.sh

wilhuff added a commit that referenced this pull request Nov 1, 2018

wilhuff added a commit that referenced this pull request Nov 1, 2018

bstpierr added a commit that referenced this pull request Nov 9, 2018

Merge branch 'master' into bs-auth-cleanup
* master: (128 commits)
  Add a nanopb string (#1839)
  Update Auth samples to sync with internal changes (#2056)
  Add clang-format installation instructions (#2057)
  Delete Firestore public C++ API (#2050)
  Porting Multi-Tab Structural Changes (#2049)
  Remove Mutation tombstones (#2052)
  Release 5.12.0 (#2051)
  Fix analyze errors (#2047)
  Isolate Firestore nanopb messages in C++ namespaces (#2046)
  Fix auth multi app support (#2043)
  Add missing nanopb flag (#2042)
  Delete deprecated files (#2038)
  Address travis timeout flakes (#2033)
  Fix tablet layout for in-app messaging (#2032)
  Held Write Acks Changelog (#2037)
  Remove Held Write Acks (#2029)
  Move #2034 into 5.12.0 (#2036)
  Partially revert "Update CHANGELOG for Firestore v0.14.0 (#2025)" (#2031)
  Revert "gRPC: replace Objective-C implementation with the new C++ implementation (#1968)" (#2030)
  Update CHANGELOG.md for #2034 (#2035)
  ...

var-const added a commit that referenced this pull request Nov 13, 2018

wilhuff added a commit that referenced this pull request Nov 13, 2018

var-const added a commit that referenced this pull request Nov 13, 2018

wilhuff added a commit that referenced this pull request Nov 13, 2018

gRPC: replace Objective-C implementation with the new C++ implementat…
…ion (#2068)

* Revert "Revert "gRPC: replace Objective-C implementation with the new C++ implementation (#1968)" (#2030)"

This reverts commit ea567dc.

* gRPC: fix bugs found during testing (#2039)

All in `GrpcStream`:
* make sure there are no pending completions when "finish" operation is enqueued;
* always finish gRPC calls;
* when connectivity changes, reset not just the calls, but the underlying channel as well.

* Revert "Partially revert "Update CHANGELOG for Firestore v0.14.0 (#2025)" (#2031)"

This reverts commit b2437b8.

* Update changelog for v0.15.0.

* gRPC: add gRPC wrapper classes to CMake build (#2015)

* add a C++ equivalent of `FIRFirestoreVersionString`. This eliminates the only Objective-C dependency of `grpc_connection.mm` and allows making it a pure C++ file;
* open-source `binary_to_array.py` script and use it in the build to embed `roots.pem` certificate file from gRPC into the Firestore binary. The embedded char array is then used to load certificate at runtime.

* Revert local change -- do not enable sanitizers by default

rsgowman added a commit that referenced this pull request Nov 27, 2018

merge master + namespace fixup (#2117)
* Allow for custom domains in the FDL iOS SDK

Allow for custo domains to be whitelisted by the info.plist file.

* Fix style. Run ./scripts/style.sh

* style

* Update versions for Release 5.11.0

* Update Firestore Generated Protos (#1972)

* Remove cruft from project file (#1975)

* Lock reads and writes to session map

* Changed FDL to use compatible __typeof__() operator. (#1982)

typeof() is not supported in some C language dialects which causes
issues when integrating FDL in projects that do use non-GNU compiler modes.

For example, this causes problems when building in some versions of Unity
firebase/quickstart-unity#228

* Changed FDL to use compatible __typeof__() operator. (#1982)

typeof() is not supported in some C language dialects which causes
issues when integrating FDL in projects that do use non-GNU compiler modes.

For example, this causes problems when building in some versions of Unity
firebase/quickstart-unity#228

* Bump DynamicLinks patch version

* Add FDL to release manifest

* Bump GoogleUtilities patch version to deliver fix for #1964 (#1987)

* Wrap diagnostics notification in collection flag check. (#1979)

* Wrap diagnostics notification in collection flag check.

Some of the diagnostics notifications were missed and not covered by
the data collection flag.

* Remove redundant notification call, move Core diagnostics API call.

* Removed configure with no options test.

* Queue Storage Tasks on separate queue (#1981)

* Increase timeout for testDispatchAfterDelay (#1988)

* Update Storage Changelog (#1989)

* Add missing FirebaseStorage release note (#1990)

* Allow semicolon in file name (#1991)

*  Remove unnecessary notification flag (#1993)

* Remove unnecessary notification flag.

This was added when the Google pod could configure Firebase but the
Google pod is deprecated and can only work with Firebase 3.X. These
flags and conditional checks can be safely removed.

* Resolve issues from commit split.

* Style fixes.

* Reduce singleton usage in FIRApp tests. (#1995)

* Reduce singleton usage in FIRApp tests.

There have been some issues while creating new tests of conflicts with
mocks of classes and instances, this should alleviate some of those
conflicts in the future.

* Remove bad style changes.

* Use default app name flag instead of local variable.

* Comply with c99 standard (#1992)

* Trigger travis for Firestore podspec changes

* Use C99-compatible __typeof__ instead of typeof (#1985)

`typeof` is only defined if you compile with GNU extensions, while
`__typeof__` is always available.

This is the Firestore equivalent of #1982.

Note that Firestore won't yet build in this mode because among other
things the Objective-C gRPC still uses `typeof`. Once we eliminate that
dependency this might become possible.

* Adding AppCode Diff (#1996)

* Remove warning (#1999)

* Add InAppMessaging to Carthage template (#2006)

* Restore SafariServices framework (#2002)

* SafariServices not available on tvOS and not used on osx

* Force Firestore to conform to C99 and C++11. (#2001)

Note that c++0x is how Xcode spells c++11.

Also fix an issue where we were accidentally using a C++14 feature.

* Changing the internal testing repo (#2003)

* Clean up test. The issue has already been fixed and we are now running integration test with actual server instead of hexa. (#2007)

* gRPC: replace Objective-C implementation with the new C++ implementation (#1968)

* add support for SSL disabled to `GrpcConnection` (unfortunately, there
  currently is no way to verify this change actually works);
* make gRPC calls using the C++ implementation:
* make `FSTRemoteStore` create C++ streams instead of their Objective-C
  counterparts;
* port firebase/firebase-js-sdk#1041: streams
  are now never recreated, only restarted;
* make `FSTDatastore` delegate server calls to the C++ implementation;
* port `MockWatchStream` and `MockWriteStream` to C++ (`FSTMockDatastore` is
  still in place, because `Datastore` is not fully ported yet);
* no longer generate Objective-C gRPC service definitions from protos;
* remove all references to Objective-C gRPC client;
* check in gRPC root certificates file and load it at runtime (the check-in part
  is temporary until gRPC-C++.podspec provides the certificate). This makes SSL
  work.

* Add component system documentation.

* Fixed markdown layout issue.

* Remove trailing whitespaces.

* Add table of contents.

* Remove extra parentheses from TOC.

* Renamed SDKs to frameworks and products for accuracy.

* Updated the Carthage installation instructions (#2012)

* Add Rome instructions (#2014)

* Attempt to fix frequent Auth Unit Test flake on OSX (#2017)

* Increase timeouts in attempt to eliminate travis flakes (#2016)

* Don't rely on test always being in foreground (#2021)

* Fix log overflow in continuous fuzz testing (#2020)

Prevent generating too many "Unrecognized selector" console messages that eventually make Travis kill the job due to log exceeding limits.

* Assign the default app before posting notifications. (#2024)

Although SDKs being configured should access the app through the
dictionary being passed in (and soon the `FIRCoreConfigurable`
protocol), the default app should be assigned before notifying SDKs
that Core is ready.

* Update CHANGELOG for Firestore v0.14.0 (#2025)

* M37 Core release notes (#2027)

* Add changelog to GoogleUtilities

* Fix static analysis warning in Core. (#2034)

Explicitly check for `nil` instead of using the `!` operator on the pointer.

* Update CHANGELOG.md for #2034 (#2035)

* Revert "gRPC: replace Objective-C implementation with the new C++ implementation (#1968)" (#2030)

This reverts commit a514bd9.

* Partially revert "Update CHANGELOG for Firestore v0.14.0 (#2025)" (#2031)

This removes the changelog entry that describes our migration to
gRPC-C++.

* Move #2034 into 5.12.0 (#2036)

* Remove Held Write Acks (#2029)

* Drop acknowledged mutations in schema migration (#1818)

Drop acknowledged mutations in schema migration as part of removing held write acks.

Port of firebase/firebase-js-sdk#1149

* Change removeMutationBatch to remove a single batch (#1955)

* Update Firestore Generated Protos

*  Adding UnknownDocument and hasCommittedMutations (#1971)

* Removing held write batch handling from LocalStore (#1997)

* View changes for held write acks (#2004)

* Held Write Acks Unit Test Fixes (#2008)

* Held Write Ack Removal Spec Tests (#2018)

* Fix integration test

* Held Write Acks Changelog (#2037)

To be submitted when main PR goes in

* Fix tablet layout for in-app messaging (#2032)

* Rename fine-tuning methods to reflect size class rather than orientation

* Modify existing constraints to de-portrait tablet layout, need a card width for tablet now

* Further constraint tinkering, add identifiers for better auto layout debugging

* More constraints for "landscape" appearance in tablet. Looks good for every case except Thin Image, which blows up the height of the message card

* Fix fine tune layout methods to correctly fine tune for compact height and width

* Update layout constraint identifiers to match titles in document outline for easier debugging

* Revert superfluous method name changes for layout fine tuning methods

* Address travis timeout flakes (#2033)

*  Delete deprecated files (#2038)

* Add missing nanopb flag (#2042)

* Fix auth multi app support (#2043)

* Isolate Firestore nanopb messages in C++ namespaces (#2046)

* Add C++ namespaces and use C++ linkage for nanopb-generated sources.
* Regenerate nanopb sources as C++
* Add generated C++ nanopb sources to the podspec

* Fix analyze errors (#2047)

* Release 5.12.0 (#2051)

* Update versions for Release 5.12.0
* Add missing nanopb flag
* Isolate Firestore nanopb messages in C++ namespaces (#2046)
* Add C++ namespaces and use C++ linkage for nanopb-generated sources.
* Regenerate nanopb sources as C++
* Add generated C++ nanopb sources to the podspec

* Remove Mutation tombstones (#2052)

* Remove Mutation tombstones

* Review comments

* Porting Multi-Tab Structural Changes (#2049)

* Delete Firestore public C++ API (#2050)

* Add firebase_cpp_sdk 5.4.0

... to the Firestore CMake build.

* Move C++ public API headers to cpp/include

* Add Firebase C++ API tests.

* Add support for building on Linux

* Add clang-format configuration for Firestore/cpp

* Add windows build support

* Review feedback

* Revert build changes to support Firebase C++ integration.

It turns out the public Firebase C++ SDK doesn't include enough to
actually build another component of that SDK externally so these changes
don't really help.

Eventually, once Firebase C++ is open source we'll integrate with that
to actually test Firestore's public C++ API.

* Delete Firestore/cpp and public C++ artifacts

It's not yet feasible to build these components externally so there's no
use in keeping them here.

* Add clang-format installation instructions (#2057)

* Update Auth samples to sync with internal changes (#2056)

* Add a nanopb string (#1839)

* Add Equatable and Comparable
* Add nanopb String

* FIRApp is already configured. Remove duplicate configure call. This Fixes unit test failure.

* Invalidate non-background url sessions

* Run style

* Update abseil to master@{2018-11-06} (#2058)

* Update abseil-cpp to a new upstream

Update to 7990fd459e9339467814ddb95000c87cb1e4d945 master@{2018-11-06}

* Re-apply -Wcomma fix

* Update add_subdirectory for Abseil

* Remove references to Firestore/Port (which longer exists)

* Fix ABSL_HAVE_THREAD_LOCAL when targeting iOS 8 in Xcode 10

* Clean up abseil warnings

* Clean up a Firestore format string warning.

* Exclude third_party/abseil-cpp from whitespace checks

* Port code review changes from google3

* Amend changelog

* Minor edits: change domain names in test app plist, fix warnings.

* Fix warning.

* Fix style.

* Adding Numeric Add Proto message (#2064)

* Adding Numeric Add Proto message

* Remove trailing whitespace

* Adding base_writes proto field (#2066)

* Update the GULObjectSwizzler to handle NSProxy objects (#2053)

* GoogleUtilities 5.3.5 (#2067)

* gRPC: replace Objective-C implementation with the new C++ implementation (#2068)

* Revert "Revert "gRPC: replace Objective-C implementation with the new C++ implementation (#1968)" (#2030)"

This reverts commit ea567dc.

* gRPC: fix bugs found during testing (#2039)

All in `GrpcStream`:
* make sure there are no pending completions when "finish" operation is enqueued;
* always finish gRPC calls;
* when connectivity changes, reset not just the calls, but the underlying channel as well.

* Revert "Partially revert "Update CHANGELOG for Firestore v0.14.0 (#2025)" (#2031)"

This reverts commit b2437b8.

* Update changelog for v0.15.0.

* gRPC: add gRPC wrapper classes to CMake build (#2015)

* add a C++ equivalent of `FIRFirestoreVersionString`. This eliminates the only Objective-C dependency of `grpc_connection.mm` and allows making it a pure C++ file;
* open-source `binary_to_array.py` script and use it in the build to embed `roots.pem` certificate file from gRPC into the Firestore binary. The embedded char array is then used to load certificate at runtime.

* Revert local change -- do not enable sanitizers by default

* Fix `string_view` referring to an out-of-scope string (#2074)

* Allow setting the domainURIPrefix for custom domain names/paths when creating dynamic links (#2071)

* Add support for creating DL with custom domain names/paths. The new domainURIPrefix parameter requires either a. a valid FDL domain name created in the Firebase console or b. a custom domain name or c. a custom domain name with path registered for DL. All domainURIPrefixes need to start with a valid (https) scheme.

* Fix style.

* style

* Fix typo in DEPRECATED_MSG_ATTRIBUTE, other nits.

* Fix styling.

* Check incoming domainURIPrefix for validity using NSURL and check for an https scheme.

* Release manifest for 5.13.0 (#2076)

* Renaming manifest (#2077)

* Release manifest for 5.13.0

* Add warning for deprecated API to indicate that the passed in domain name's scheme will deduced as https (#2078)

* Add support for creating DL with custom domain names/paths. The new domainURIPrefix parameter requires either a. a valid FDL domain name created in the Firebase console or b. a custom domain name or c. a custom domain name with path registered for DL. All domainURIPrefixes need to start with a valid (https) scheme.

* Fix style.

* style

* Fix typo in DEPRECATED_MSG_ATTRIBUTE, other nits.

* Fix styling.

* Check incoming domainURIPrefix for validity using NSURL and check for an https scheme.

* Add extra checks for deprecated domain parameter being incorrectly called with a scheme. Also fix some nits.

* Log a warning for the deprecated API stating that the scheme for the supplied domain will be deduced as https.

* Update CHANGELOG for M38.

* Update changelog for M38

* Update CHANGELOG for M38 release.

* Capitalize HTTPS.

* Update CHANGELOG to include PR for removal of deprecated files.

* Capitalize HTTPS.

* Fix GoogleUtilities nullability regressions (#2079)

* Remove `adjustsFontForContentSizeCategory` (unsupported in iOS 10.0) from labels in card layout Storyboard (#2083)

* Add pod spec lint testing for Xcode 9 (#2084)

* FirebaseAnalyticsInterop is cross-platform (#2088)

* C++: eliminate `FSTDispatchQueue` and replace it with `AsyncQueue` (#2062)

* replace all references to `FSTDispatchQueue` and `FSTTimerID` with their C++ equivalents;
* remove `FSTDispatchQueue` class and related tests;
* in method argument names, replace `workerDispatchQueue` with just `workerQueue`, more similar to other platforms;
* move `Executor` and derived classes out of `internal` namespace.

* Database Interop (#1865)

* Register Database with Interop

+ Add AuthInterop dependency
+ Have Database register with Interop component container
+ Add weak dependency on Auth
~ Cleaned up imports

* Use Interop for Auth token fetching

+ Use macro to get auth component
~ Change `getTokenForcingRefresh:withCallback:` call to use Interop component version of Auth
~ Clean up imports

* Implement necessary protocols

+ Added FIRComponentRegistrant and FIRDatabaseNilProtocol to FIRDatabase

* Squash of my previous local commits for Database Interop

Register Database with Interop:

  + Add AuthInterop dependency
  + Have Database register with Interop component container
  + Add weak dependency on Auth
  ~ Cleaned up imports

Use Interop for Auth token fetching:

  + Use macro to get auth component
  ~ Change `getTokenForcingRefresh:withCallback:` call to use Interop component version of Auth
  ~ Clean up imports

Implement necessary protocols:

  + Added FIRComponentRegistrant and FIRDatabaseNilProtocol to FIRDatabase

Clean up Database tests:

  - Removed all unnecessary header files
  ~ Ran style.sh across all the tests

Cleaned Up project file:

  Some header removals were missed in the last commit.

Sorted some test files

Fix Database Fake App Tests:

  + Added container property
  + Added an Auth fake in the container
  + Added the Shared utils files to the necessary targets
  + Added a missing XCTest reference in a test that didn't compile for me

* Revert "Squash of my previous local commits for Database Interop"

This reverts commit c0d0421.

* Cleaned Up project file

Some header removals were missed in the last commit.

* Sorted some test files

* Fix Database Fake App Tests

+ Added container property
+ Added an Auth fake in the container
+ Added the Shared utils files to the necessary targets
+ Added a missing XCTest reference in a test that didn't compile for me

* PR Feedback Changes

+ Created new FIRDatabaseComponent class to encapsulate instance creation
+ Updated FAuthTokenProvider to only use an auth instance rather than an app.

* PR Feedback Follow-Up

- Removed FIRDatabase registration code

* pod deintegrate

* Move instance management

Somes tests may still be out of date.

* Fix Auth integration test

* pod deintegrate -- again

* PR Feedback

* PR Feedback

Added the FIRComponentLifecycleMaintainer protocol to FIRDatabaseComponent

* Update Firebase/Database/Api/FIRDatabaseComponent.m

Missed some small changes

* Get integration tests compiling

* Fix some tests

* Fixed more tests

Component needs to be cacheable in order to get the appWIllBeDeleted callback.

* Update target memberships

* Revert project file changes

* Add FIRAuthInteropFake for broken targets

* PR feedback

* Spacing and Style

* Moved `instances` to ivar

* Simplify FIRDatabaseDictionary

It's now keyed on the URL with the app being implied since each app will get its own `FIRDatabaseComponent`

* Revert "Database Interop (#1865)" (#2093)

This reverts commit 4090195.

* Revert premature api changes (#2097)

* Revert "Add warning for deprecated API to indicate that the passed in domain name's scheme will deduced as https (#2078)"

This reverts commit ceb8392.

* Revert "Allow setting the domainURIPrefix for custom domain names/paths when creating dynamic links (#2071)"

This reverts commit d917bac.

* Revert "Minor edits: change domain names in test app plist, fix warnings."

This reverts commit 15f5d46.

* Revert "Fix style. Run ./scripts/style.sh"

This reverts commit 0e16e6c.

* Revert "Allow for custom domains in the FDL iOS SDK"

This reverts commit 5e33153.

* Version is still 3.2.0

* Add test for verify iOS client (#2096)

* Release 5.13.0 (#2101)

* Update versions for Release 5.13.0

* Revert premature api changes (#2097)

* Revert "Add warning for deprecated API to indicate that the passed in domain name's scheme will deduced as https (#2078)"

This reverts commit ceb8392.

* Revert "Allow setting the domainURIPrefix for custom domain names/paths when creating dynamic links (#2071)"

This reverts commit d917bac.

* Revert "Minor edits: change domain names in test app plist, fix warnings."

This reverts commit 15f5d46.

* Revert "Fix style. Run ./scripts/style.sh"

This reverts commit 0e16e6c.

* Revert "Allow for custom domains in the FDL iOS SDK"

This reverts commit 5e33153.

* Version is still 3.2.0

* Pin gRPC-C++ version to exactly 0.0.5 (#2105)

While gRPC-C++ is in its 0.* versions, any new version could potentially contain breaking changes. Make sure we only upgrade at our own pace.

* Database Interop Rollforward (#2095)

* Register Database with Interop

+ Add AuthInterop dependency
+ Have Database register with Interop component container
+ Add weak dependency on Auth
~ Cleaned up imports

* Use Interop for Auth token fetching

+ Use macro to get auth component
~ Change `getTokenForcingRefresh:withCallback:` call to use Interop component version of Auth
~ Clean up imports

* Implement necessary protocols

+ Added FIRComponentRegistrant and FIRDatabaseNilProtocol to FIRDatabase

* Squash of my previous local commits for Database Interop

Register Database with Interop:

  + Add AuthInterop dependency
  + Have Database register with Interop component container
  + Add weak dependency on Auth
  ~ Cleaned up imports

Use Interop for Auth token fetching:

  + Use macro to get auth component
  ~ Change `getTokenForcingRefresh:withCallback:` call to use Interop component version of Auth
  ~ Clean up imports

Implement necessary protocols:

  + Added FIRComponentRegistrant and FIRDatabaseNilProtocol to FIRDatabase

Clean up Database tests:

  - Removed all unnecessary header files
  ~ Ran style.sh across all the tests

Cleaned Up project file:

  Some header removals were missed in the last commit.

Sorted some test files

Fix Database Fake App Tests:

  + Added container property
  + Added an Auth fake in the container
  + Added the Shared utils files to the necessary targets
  + Added a missing XCTest reference in a test that didn't compile for me

* Revert "Squash of my previous local commits for Database Interop"

This reverts commit c0d0421.

* Cleaned Up project file

Some header removals were missed in the last commit.

* Sorted some test files

* Fix Database Fake App Tests

+ Added container property
+ Added an Auth fake in the container
+ Added the Shared utils files to the necessary targets
+ Added a missing XCTest reference in a test that didn't compile for me

* PR Feedback Changes

+ Created new FIRDatabaseComponent class to encapsulate instance creation
+ Updated FAuthTokenProvider to only use an auth instance rather than an app.

* PR Feedback Follow-Up

- Removed FIRDatabase registration code

* pod deintegrate

* Move instance management

Somes tests may still be out of date.

* Fix Auth integration test

* pod deintegrate -- again

* PR Feedback

* PR Feedback

Added the FIRComponentLifecycleMaintainer protocol to FIRDatabaseComponent

* Update Firebase/Database/Api/FIRDatabaseComponent.m

Missed some small changes

* Get integration tests compiling

* Fix some tests

* Fixed more tests

Component needs to be cacheable in order to get the appWIllBeDeleted callback.

* Update target memberships

* Revert project file changes

* Add FIRAuthInteropFake for broken targets

* PR feedback

* Spacing and Style

* Moved `instances` to ivar

* Simplify FIRDatabaseDictionary

It's now keyed on the URL with the app being implied since each app will get its own `FIRDatabaseComponent`

* Fix Database Integration Tests

Have the FakeApp cache DB instances
Use the Full host/URL path to cache DB instances
Simplified shared scheme

```
Test Suite 'Database_IntegrationTests_iOS.xctest' passed at 2018-11-20 07:09:30.520.
	 Executed 326 tests, with 0 failures (0 unexpected) in 66.251 (66.528) seconds
Test Suite 'All tests' passed at 2018-11-20 07:09:30.522.
	 Executed 326 tests, with 0 failures (0 unexpected) in 66.251 (66.530) seconds
```

* Clarify defaultApp use

* Prefer roots.pem from gRPC-C++, but fall back to Firestore bundled ones if necessary (#2106)

gRPC-C++ versions up to and including 0.0.3, and also 0.0.5, don't bundle `roots.pem` in the podspec. gRPC-C++ 0.0.4 and, presumably, the currently not-yet-released 0.0.6 do. Firestore currently also bundles this file under the same bundle name, which leads to build errors when both Firestore and gRPC-C++ try to add the file into the build (only shows during archiving).

For transition, this PR:
* renames the Firestore bundle to avoid naming clash;
* changes the loading code so that it first tries to load certificates bundled with gRPC-C++ (versions 0.0.4 and 0.0.6+), but falls back to those bundled with Firestore if necessary.

At a later point, Firestore should be changed to not bundle the certificates altogether.

* Add Firebase Source to Header Search Path (#2114)

This resolves the annoying Xcode errors that claim `FIRAppInternal.h` can't be found.

It also makes it *much* easier to "Jump to definition" now.

* Implement PatchMutation::ApplyToLocalView (#1973)

* Test namespacing fixup

Since the nanopb protos are now namespaced, we need to account for that
in our serializer_test.cc file.

@paulb777 paulb777 deleted the varconst/grpc-integr82 branch Apr 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.