Changes in the iOS API for FieldValue migration #8034
Conversation
Generated by 🚫 Danger |
|
|
||
| google_firestore_v1_ArrayValue components; | ||
| components.values_count = CheckedSize(order_bys.size()); | ||
| components.values = MakeArray<google_firestore_v1_Value>(components.values_count); |
There was a problem hiding this comment.
Not related to this PR: what do you think about having a helper like
std::tie(components.values, components.values_count) =
MakeArray<google_firestore_v1_Value>(order_bys.size());? Or I guess another alternative would be to create a bunch of helpers similar to FieldsArray that holds information on each array (it might be a lot of boilerplate, though). Usage could look something like this:
NanopbArray<ArrayValue>(components) = MakeArray<google_firestore_v1_Value>(order_bys.size());or
MakeArray<ArrayValue, google_firestore_v1_Value>(components, order_bys.size());There was a problem hiding this comment.
I will consider this if there are more callsites where I can't use SetRepeatedField. I think this should be limited to the iOS Objective-C code, since it uses datastructures that are not compatible with the current method.
Firestore/Source/API/FIRQuery.mm
Outdated
| ThrowInvalidArgument("Invalid query. Expected a string for the document ID."); | ||
| } | ||
| const std::string &documentID = fieldValue.string_value(); | ||
| std::string documentID = MakeString(fieldValue.string_value); |
There was a problem hiding this comment.
Optional: can this be a string_view? (there is a ResourcePath::FromStringView function)
There was a problem hiding this comment.
If I recall correctly, you said that I am not able to use string_view APIs from iOS code. Is that correct?
There was a problem hiding this comment.
You cannot use Abseil in the calls between our code from firebase-cpp-sdk (aka public C++) and firebase-ios-sdk (aka core C++) because they are compiled separately. In this case, all the code would be within the iOS SDK and compiled together, so it's not an issue. However, making it possible for the public iOS types to access those APIs while hiding them from public C++ sounds like more trouble than it's worth.
Firestore/Source/API/FIRQuery.mm
Outdated
| @@ -605,13 +626,14 @@ - (Bound)boundFromFieldValues:(NSArray<id> *)fieldValues isBefore:(BOOL)isBefore | |||
| path.CanonicalString()); | |||
| } | |||
| DocumentKey key{path}; | |||
| fieldValue = FieldValue::FromReference(self.firestore.databaseID, key); | |||
| FreeNanopbMessage(google_firestore_v1_Value_fields, &fieldValue); | |||
| fieldValue = RefValue(self.firestore.databaseID, key); | |||
There was a problem hiding this comment.
Question: can you use Message to avoid manually freeing? (Note that it has a release method)
| components.values[idx] = RefValue(self.firestore.databaseID, key); | ||
| } else { | ||
| fieldValue.release(); | ||
| components.values[idx] = *fieldValue; |
There was a problem hiding this comment.
Optional nit: release returns a pointer, so this could be
components.values[idx] = *fieldValue.release();There was a problem hiding this comment.
I will fix this in the main PR.
This changes the code in the iOS API to directly read from and write to Protobuf.