Migrate CPP SDK from FieldValue to Protobuf #577

schmidt-sebastian · 2021-07-26T21:12:28Z

This changes the internals of the Firestore C++ to use the updated code on iOS, which drops model::FieldValue in favor of using Protobuf directly.

I did not port the UserDataReader/UserDataWriter refactor, but we can do that as a follow up.

This will not build until the next iOS Release.

var-const · 2021-07-28T00:49:17Z

firestore/src/main/field_value_main.cc

 }

 size_t FieldValueInternal::blob_size() const {
  SIMPLE_HARD_ASSERT(type_ == Type::kBlob);
-  return absl::get<model::FieldValue>(value_).blob_value().size();
+  auto value =


Nit: ditto.

var-const · 2021-07-28T00:49:20Z

firestore/src/main/field_value_main.cc

 }

 const uint8_t* FieldValueInternal::blob_value() const {
  SIMPLE_HARD_ASSERT(type_ == Type::kBlob);
-  return absl::get<model::FieldValue>(value_).blob_value().data();
+  auto value =


Nit: s/auto/auto*/.

var-const · 2021-07-28T00:52:59Z

firestore/src/main/field_value_main.cc

-FieldValueInternal::FieldValueInternal(bool value)
-    : type_{Type::kBoolean}, value_{model::FieldValue::FromBoolean(value)} {}
+FieldValueInternal::FieldValueInternal(bool value) : type_{Type::kBoolean} {
+  auto proto = absl::get<SharedMessage<google_firestore_v1_Value>>(value_);


An alias for SharedMessage<google_firestore_v1_Value> would help reduce some of this verbosity -- something like

using ProtoValueT = SharedMessage<google_firestore_v1_Value>;

absl::get<SharedMessage<google_firestore_v1_Value>> is repeated so many times, it might make sense to create a helper function for it along the lines of:

ProtoValueT& GetValue() { return absl::get<SharedMessage<google_firestore_v1_Value>>(value_); }

absl::get returns a reference, so this could be auto& proto to avoid an unnecessary copy.

I added the helper method, which reduces the number of SharedMessage<google_firestore_v1_Value> types significantly. I do not think we need an alias anymore.

Also changes all access to references.

var-const · 2021-07-28T01:16:33Z

firestore/src/main/user_data_converter_main.cc

+      const std::string& key = kv.first;
+      const FieldValue& value = kv.second;
+
+      auto parsedValue = ParseData(value, context.ChildContext(key));


Nit: s/parsedValue/parsed_value/.

var-const · 2021-07-28T01:17:51Z

firestore/src/main/user_data_converter_main.cc

    }
  }

-  return model::ObjectValue::FromMap(std::move(result));
+  return std::move(result);


It's not necessary (and sometimes can be slightly harmful to performance) to move when returning local variables.

var-const · 2021-07-28T01:25:14Z

firestore/src/main/document_snapshot_main.cc

+  for (pb_size_t i = 0; i < object.fields_count; ++i) {
+    std::string key = MakeString(object.fields[i].key);
+    const google_firestore_v1_Value& value = object.fields[i].value;
+    result[key] = ConvertAnyValue(value, stb);


Nit: you can move key.

var-const · 2021-07-28T01:26:37Z

firestore/src/main/document_snapshot_main.cc


-  api::DocumentReference api_reference{reference.key(), snapshot_.firestore()};
+  api::DocumentReference api_reference{key, snapshot_.firestore()};


You can probably move key.

var-const · 2021-07-28T01:37:47Z

firestore/src/main/query_main.cc

@@ -190,10 +210,10 @@ core::Bound QueryInternal::ToBound(
      SimpleThrowInvalidArgument(message);
    }

-    components.push_back(std::move(value).value());
+    components->values[i] = *DeepClone(*value).release();


Question: why is DeepClone necessary here?

The memory is owned by the DocumentSnapshot and we need to move ownership to the query bound. See here: https://github.com/firebase/firebase-ios-sdk/blob/e8f419baf275899068e915a6c2ecb4ae1e43d264/Firestore/Source/API/FIRQuery.mm#L584

var-const · 2021-07-28T01:38:49Z

firestore/src/main/query_main.cc

    SimpleThrowInvalidArgument(
        "Invalid query. Expected a string for the document ID.");
  }
-  const std::string& document_id = from.string_value();
+  const std::string& document_id = MakeString(from->string_value);


Please make document_id a non-reference variable to avoid attaching a reference to a temporary.

schmidt-sebastian

Thanks for the quick review. I addressed your feedback, but I did not verify the build. I will wait until this is ready for merging until I assign this back.

schmidt-sebastian · 2021-07-28T15:28:07Z

firestore/src/main/document_snapshot_main.cc

+  for (pb_size_t i = 0; i < object.fields_count; ++i) {
+    std::string key = MakeString(object.fields[i].key);
+    const google_firestore_v1_Value& value = object.fields[i].value;
+    result[key] = ConvertAnyValue(value, stb);


schmidt-sebastian · 2021-07-28T15:34:49Z

firestore/src/main/field_value_main.cc

-FieldValueInternal::FieldValueInternal(bool value)
-    : type_{Type::kBoolean}, value_{model::FieldValue::FromBoolean(value)} {}
+FieldValueInternal::FieldValueInternal(bool value) : type_{Type::kBoolean} {
+  auto proto = absl::get<SharedMessage<google_firestore_v1_Value>>(value_);


I added the helper method, which reduces the number of SharedMessage<google_firestore_v1_Value> types significantly. I do not think we need an alias anymore.

Also changes all access to references.

schmidt-sebastian · 2021-07-28T15:34:56Z

firestore/src/main/field_value_main.cc

 }

 const uint8_t* FieldValueInternal::blob_value() const {
  SIMPLE_HARD_ASSERT(type_ == Type::kBlob);
-  return absl::get<model::FieldValue>(value_).blob_value().data();
+  auto value =


schmidt-sebastian · 2021-07-28T15:35:00Z

firestore/src/main/field_value_main.cc

 }

 size_t FieldValueInternal::blob_size() const {
  SIMPLE_HARD_ASSERT(type_ == Type::kBlob);
-  return absl::get<model::FieldValue>(value_).blob_value().size();
+  auto value =


schmidt-sebastian · 2021-07-28T15:40:02Z

firestore/src/main/query_main.cc

@@ -190,10 +210,10 @@ core::Bound QueryInternal::ToBound(
      SimpleThrowInvalidArgument(message);
    }

-    components.push_back(std::move(value).value());
+    components->values[i] = *DeepClone(*value).release();


The memory is owned by the DocumentSnapshot and we need to move ownership to the query bound. See here: https://github.com/firebase/firebase-ios-sdk/blob/e8f419baf275899068e915a6c2ecb4ae1e43d264/Firestore/Source/API/FIRQuery.mm#L584

schmidt-sebastian · 2021-07-28T15:40:26Z

firestore/src/main/query_main.cc

    SimpleThrowInvalidArgument(
        "Invalid query. Expected a string for the document ID.");
  }
-  const std::string& document_id = from.string_value();
+  const std::string& document_id = MakeString(from->string_value);


schmidt-sebastian · 2021-07-28T15:41:06Z

firestore/src/main/user_data_converter_main.cc

+      const std::string& key = kv.first;
+      const FieldValue& value = kv.second;
+
+      auto parsedValue = ParseData(value, context.ChildContext(key));


schmidt-sebastian · 2021-07-28T15:41:10Z

firestore/src/main/user_data_converter_main.cc

    }
  }

-  return model::ObjectValue::FromMap(std::move(result));
+  return std::move(result);


🍞 Dismissed stale approval on external PR.

…pp-sdk into mrschmidt/fieldvalue

var-const · 2021-08-18T16:29:03Z

Android/firebase_dependencies.gradle

@@ -37,7 +37,7 @@ def firebaseDependenciesMap = [
                 'com.google.flatbuffers:flatbuffers-java:1.12.0',
                 'com.google.android.gms:play-services-base:17.6.0'],
  'performance' : ['com.google.firebase:firebase-perf:20.0.2'],
-  'remote_config' : ['com.google.firebase:firebase-config:21.0.0',
+  'remote_config' : ['com.google.firebase:firebase-config:21.0.1',


Is this change intentional?

Yes, this is part of the version update that we want to release.

var-const · 2021-08-18T16:29:12Z

.github/workflows/integration_tests.yml

@@ -14,7 +14,7 @@ on:
        required: true
      apis:
        description: 'CSV of apis to build and test'
-        default: 'admob,analytics,auth,database,dynamic_links,firestore,functions,installations,messaging,remote_config,storage'
+        default: 'firestore'


This change needs to be reverted.

var-const · 2021-08-18T16:30:26Z

scripts/gha/integration_testing/test_validation.py

@@ -80,7 +80,7 @@ def validate_results_unity(log_text):
    # After the result string comes a list of failing testapps.
    summary = log_text[results_match.start():]
  else:
-    summary = _tail(log_text, 15)
+    summary = _tail(log_text, 1500000)


Should these be reverted?

google-cla bot added the cla: yes label Jul 26, 2021

FieldValue port

e5c1837

schmidt-sebastian force-pushed the mrschmidt/fieldvalue branch from 3d7391b to e5c1837 Compare July 27, 2021 01:16

schmidt-sebastian requested a review from var-const July 27, 2021 01:17

schmidt-sebastian assigned var-const Jul 27, 2021

schmidt-sebastian added the api: firestore label Jul 27, 2021

var-const suggested changes Jul 28, 2021

View reviewed changes

var-const assigned schmidt-sebastian and unassigned var-const Jul 28, 2021

Feedback

baed896

schmidt-sebastian commented Jul 28, 2021

View reviewed changes

schmidt-sebastian added 2 commits July 28, 2021 09:49

Lint

0ed529e

Update source to master

273171b

schmidt-sebastian force-pushed the mrschmidt/fieldvalue branch 3 times, most recently from bc9e33d to ec39332 Compare July 28, 2021 18:48

Fix compile

ab8b574

schmidt-sebastian force-pushed the mrschmidt/fieldvalue branch from ec39332 to ab8b574 Compare July 28, 2021 18:50

schmidt-sebastian added 3 commits July 28, 2021 13:08

Add nanopb

a19d7cc

Use header

2206809

Fix build

147d5a8

schmidt-sebastian added the tests-requested: quick Trigger a quick set of integration tests. label Jul 29, 2021

Merge branch 'main' into mrschmidt/fieldvalue

f139608

var-const previously approved these changes Aug 6, 2021

View reviewed changes

schmidt-sebastian added 2 commits August 9, 2021 18:50

Updated Android and iOS dependencies for 8.4.0 release

0b200d3

Merge

a511c1b

schmidt-sebastian added tests-requested: quick Trigger a quick set of integration tests. and removed tests-requested: quick Trigger a quick set of integration tests. labels Aug 10, 2021

github-actions bot removed the tests-requested: quick Trigger a quick set of integration tests. label Aug 10, 2021

schmidt-sebastian added 2 commits August 17, 2021 21:24

Revert debugging changes

e024ee1

Merge branch 'mrschmidt/fieldvalue' of github.com:firebase/firebase-c…

7899926

…pp-sdk into mrschmidt/fieldvalue

github-actions bot added the tests: failed This PR's integration tests failed. label Aug 18, 2021

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Aug 18, 2021

schmidt-sebastian added tests-requested: quick Trigger a quick set of integration tests. and removed tests: failed This PR's integration tests failed. labels Aug 18, 2021

github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Aug 18, 2021

schmidt-sebastian assigned var-const and unassigned schmidt-sebastian Aug 18, 2021

github-actions bot added the tests: failed This PR's integration tests failed. label Aug 18, 2021

var-const suggested changes Aug 18, 2021

View reviewed changes

Revert

2495ac3

schmidt-sebastian added tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). and removed tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. labels Aug 18, 2021

github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. and removed tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). labels Aug 18, 2021

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Aug 18, 2021

a-maurice approved these changes Aug 18, 2021

View reviewed changes

var-const approved these changes Aug 18, 2021

View reviewed changes

var-const assigned schmidt-sebastian and unassigned var-const Aug 18, 2021

schmidt-sebastian enabled auto-merge August 18, 2021 20:38

a-maurice removed the tests: failed This PR's integration tests failed. label Aug 18, 2021

schmidt-sebastian merged commit 4725fa6 into main Aug 18, 2021

github-actions bot added the tests: in-progress This PR's integration tests are in progress. label Aug 18, 2021

schmidt-sebastian deleted the mrschmidt/fieldvalue branch August 18, 2021 20:59

firebase locked and limited conversation to collaborators Sep 18, 2021


		api::DocumentReference api_reference{reference.key(), snapshot_.firestore()};
		api::DocumentReference api_reference{key, snapshot_.firestore()};

Migrate CPP SDK from FieldValue to Protobuf #577

Migrate CPP SDK from FieldValue to Protobuf #577

Uh oh!

Conversation

schmidt-sebastian commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

schmidt-sebastian commented Jul 26, 2021 •

edited

Loading