Add `is_valid` methods to the public API classes that can be in an invalid state #560

var-const · 2021-07-21T00:42:36Z

Also improve the error message contained in the invalid futures returned from invalid objects.

Addresses b/172570071 and b/157227167.

github-actions · 2021-07-21T00:46:22Z

⏳ Integration test in progress...

Requested by @var-const on commit be461e5
Last updated: Thu Jul 22 21:33 PDT 2021
View integration test log & download artifacts

var-const · 2021-07-21T00:46:37Z

@dconeybe @schmidt-sebastian Feel free to tweak/nitpick the error message and the comments.

schmidt-sebastian

LGTM with nits you can ignore.

schmidt-sebastian · 2021-07-21T15:11:35Z

firestore/src/common/futures.h

+      "The object that issued this future is in an invalid state. This can be "
+      "because:\n- the object was default-constructed and never reassigned;\n"
+      "- the object was moved from;\n- the Firestore instance with which the "
+      "object was associated has been destroyed."));


Is it common to have error messages like this in C++? I know that in Java a line break in an error message would be pretty annoying and make the overall message (which includes the stacktrace) less readable.

Good point, removed the newlines.

schmidt-sebastian · 2021-07-21T15:12:30Z

firestore/src/include/firebase/firestore/document_change.h

@@ -159,6 +159,19 @@ class DocumentChange {
   */
  virtual std::size_t new_index() const;

+  /**
+   * @brief Returns true if this `DocumentChange` is valid, false if it is


The last part (false if it is not valid) does not seem to be strictly needed.

I agree, but historically the approach was to be as clear as possible, at the cost of some redundancy. I'd rather keep as is.

schmidt-sebastian · 2021-07-21T15:13:35Z

firestore/src/include/firebase/firestore/query.h

@@ -637,6 +637,18 @@ class Query {
      std::function<void(const QuerySnapshot&, Error, const std::string&)>
          callback);

+  /**
+   * @brief Returns true if this `Query` is valid, false if it is not valid. An


I am not sure if we need to mention this, but the main usage for this API will likely be via CollectionReference.is_valid. I am not sure if all of our users are aware of our type hierarchy.

I thought about this too. Unfortunately, I don't see a great way around this. We could make this function virtual just to override it to do the same thing in the derived class, but it gives the impression that the behavior of this function is somehow different in the derived class, which can be confusing. We could define the function with the same name without making it virtual, but that would be shadowing the function in the main class and is likely to trigger compilation warnings and just in general looks questionable.

Yeah, I also do not know of a good way to address this.

schmidt-sebastian · 2021-07-22T02:43:51Z

firestore/src/include/firebase/firestore/query.h

@@ -637,6 +637,18 @@ class Query {
      std::function<void(const QuerySnapshot&, Error, const std::string&)>
          callback);

+  /**
+   * @brief Returns true if this `Query` is valid, false if it is not valid. An


Yeah, I also do not know of a good way to address this.

var-const added 3 commits July 20, 2021 20:04

Initial commit

2998549

Small fixes

a7ff719

Add tests

b6021d7

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

Release notes

58674a8

var-const requested review from dconeybe and schmidt-sebastian July 21, 2021 00:45

var-const assigned schmidt-sebastian and dconeybe Jul 21, 2021

var-const added api: firestore tests-requested: quick Trigger a quick set of integration tests. labels Jul 21, 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 Jul 21, 2021

github-actions bot added the tests: succeeded This PR's integration tests succeeded. label Jul 21, 2021

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

dconeybe previously approved these changes Jul 21, 2021

View reviewed changes

dconeybe assigned var-const and schmidt-sebastian and unassigned schmidt-sebastian and dconeybe Jul 21, 2021

schmidt-sebastian previously approved these changes Jul 21, 2021

View reviewed changes

schmidt-sebastian removed their assignment Jul 21, 2021

Tweak error message

4a7b301

var-const dismissed stale reviews from schmidt-sebastian and dconeybe via 4a7b301 July 21, 2021 19:04

var-const assigned schmidt-sebastian and dconeybe and unassigned var-const Jul 22, 2021

var-const added tests-requested: quick Trigger a quick set of integration tests. and removed tests: succeeded This PR's integration tests succeeded. labels Jul 22, 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 Jul 22, 2021

var-const enabled auto-merge July 22, 2021 00:09

dconeybe approved these changes Jul 22, 2021

View reviewed changes

dconeybe assigned var-const and unassigned dconeybe Jul 22, 2021

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

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

schmidt-sebastian approved these changes Jul 22, 2021

View reviewed changes

schmidt-sebastian removed their assignment Jul 22, 2021

Merge branch 'main' into varconst/error-message-on-invalid

1f0fbe3

var-const added tests-requested: quick Trigger a quick set of integration tests. and removed tests: failed This PR's integration tests failed. labels Jul 22, 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 Jul 22, 2021

Updated submodule

00c50e4

var-const added tests-requested: quick Trigger a quick set of integration tests. and removed tests: in-progress This PR's integration tests are in progress. labels Jul 23, 2021

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

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

var-const merged commit be461e5 into main Jul 23, 2021

github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests: succeeded This PR's integration tests succeeded. labels Jul 23, 2021

firebase locked and limited conversation to collaborators Aug 23, 2021

jonsimantov deleted the varconst/error-message-on-invalid branch December 22, 2021 22:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add `is_valid` methods to the public API classes that can be in an invalid state #560

Add `is_valid` methods to the public API classes that can be in an invalid state #560

var-const commented Jul 21, 2021 •

edited

Loading

Uh oh!

github-actions bot commented Jul 21, 2021 •

edited

Loading

Uh oh!

var-const commented Jul 21, 2021

Uh oh!

schmidt-sebastian left a comment

Uh oh!

schmidt-sebastian Jul 21, 2021

Uh oh!

var-const Jul 21, 2021

Uh oh!

schmidt-sebastian Jul 21, 2021

Uh oh!

var-const Jul 21, 2021

Uh oh!

schmidt-sebastian Jul 21, 2021

Uh oh!

var-const Jul 21, 2021

Uh oh!

schmidt-sebastian Jul 22, 2021

Uh oh!

schmidt-sebastian Jul 22, 2021

Uh oh!

Uh oh!

Add is_valid methods to the public API classes that can be in an invalid state #560

Add is_valid methods to the public API classes that can be in an invalid state #560

Conversation

var-const commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⏳ Integration test in progress...

Uh oh!

var-const commented Jul 21, 2021

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!

Uh oh!

Add `is_valid` methods to the public API classes that can be in an invalid state #560

Add `is_valid` methods to the public API classes that can be in an invalid state #560

var-const commented Jul 21, 2021 •

edited

Loading

github-actions bot commented Jul 21, 2021 •

edited

Loading