-
Notifications
You must be signed in to change notification settings - Fork 42
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
feat(generator/dart): implement the 'Any' WKT #1547
Conversation
/// | ||
/// [fullyQualifiedName] should be the fully qualified type name of the given | ||
/// message (for example, `google.protobuf.Duration`). | ||
void packInto(Message message, String fullyQualifiedName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, we require the user to pass in the FQN name of the given message. We could generate an instance member for Message
which would respond w/ the fully qualified type name. However, that would add a new member into the namespace of every message that would really only be used for Any.packInto
- I'm not sure its worth it for this single use-case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems sad that we are asking folks to provide a value (the fully qualified name) when we know what the answer should be. We should be able to prevent bugs like this one:
any.packInto(Duration(...), "type.googleapis.com/google.protobuf.Timestamp")
Could we make this into a generic function and access the type name through the class?
void packInto<M extends Message>(M message) {
// Can we use `M.fullyQualifiedName` here ?? Or maybe some helper generic `SomeTrait<M>().fullyQualifiedName()` ?
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure there's a way to do that. Interfaces in Dart only apply to instance members, not static ones.
If we do want to reduce risk here (which seems reasonable, even for a less frequent use-case), I think that there will need to be an instance member on Message
that you can ask for its FQN.
We could possibly up-level APIs / protobuf packages to first-class objects (so, a message would be able to tell you it's API
; that api instance could tell you a message's fully qualified name). It doesn't reduce the Message
surface area but perhaps an API abstraction would be useful for other things as well.
I'll try the Message.qualifiedName
route first.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1547 +/- ##
=======================================
Coverage 95.09% 95.09%
=======================================
Files 39 39
Lines 1610 1610
=======================================
Hits 1531 1531
Misses 79 79 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an improvement over what we have today, feel free to merge.
I think it could be better: we should try to avoid APIs that can be used incorrectly, like packInto()
and unpackFrom()
.
/// describes the type of the serialized message. | ||
class Any extends Message { | ||
static const Set<String> _customEncodedTypes = { | ||
'google.protobuf.Duration', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will be more, Timestamp
, and StringValue
(generally, all the *Value
wrappers), NullValue
, Value
, and Struct
. A hard-coded list works, but maybe we should add some functionality to Message
? Feel free to post-pone this to a future PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if its a fixed list I'm ok w/ hard-coding (once in Dart code, once in Go code?). I can keep thinking about this though. Are the only custom encoded types those in the google.protobuf
package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can keep thinking about this though.
In Rust we were able to use some nifty stuff around traits with default implementations and specialization, but does not look like that is a thing we could do in Dart (that is fine).
Are the only custom encoded types those in the
google.protobuf
package?
AFAIK, yes. The authoritative source (which I only learnt of recently) is: https://protobuf.dev/programming-guides/json/
/// | ||
/// [fullyQualifiedName] should be the fully qualified type name of the given | ||
/// message (for example, `google.protobuf.Duration`). | ||
void packInto(Message message, String fullyQualifiedName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems sad that we are asking folks to provide a value (the fully qualified name) when we know what the answer should be. We should be able to prevent bugs like this one:
any.packInto(Duration(...), "type.googleapis.com/google.protobuf.Timestamp")
Could we make this into a generic function and access the type name through the class?
void packInto<M extends Message>(M message) {
// Can we use `M.fullyQualifiedName` here ?? Or maybe some helper generic `SomeTrait<M>().fullyQualifiedName()` ?
}
any.packInto(duration, Duration.fullyQualifiedName); | ||
expect(any.typeName, 'google.protobuf.Duration'); | ||
|
||
final actual = any.unpackFrom(Duration.fromJson); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally I should be able to say: any.unpackFrom(Duration)
(or any.unpackFrom<Duration>()
if that is possible?) the function is something we should be able to figure out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, not easily. I'm going off the working assumption that interacting with Any
is somewhat rare?
We could switch this around so that unpacking from Any
looks like:
final message = MyMessage.fromJson(any.toJson());
or even:
final message = MyMessage.unpack(any);
But adding a new unpack to every message would really only make sense if using Any were common.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But adding a new unpack to every message would really only make sense if using Any were common.
It depends on how good of a job we do with errors and LROs.
Errors
The errors returned from Google services include this:
// A list of messages that carry the error details. There is a common set of
// message types for APIs to use.
repeated google.protobuf.Any details = 3;
The "common set" is the error_details.proto file. At least Google Cloud Storage returns something that is not in that list. No worries, that service needs to be hand-crafted anyway. But that indicates that other services are free to do the same... seems unlikely, because I do not know how customers would deal with unexpected errors details.
LROs
LROs return the operation result as an Any, and the metadata too (I think of this as a report of the LRO progress). We should extract the response and metadata from the operation ourselves. That is very tedious code for applications to write. Compare:
google-cloud-rust/guide/samples/src/lro.rs
Lines 97 to 101 in a778170
// ANCHOR: automatic-poller-until-done | |
.poller() | |
.until_done() | |
.await?; | |
// ANCHOR_END: automatic-poller-until-done |
vs.
google-cloud-rust/guide/samples/src/lro.rs
Lines 168 to 213 in a778170
let mut operation = operation; | |
// ANCHOR: manual-if-done | |
loop { | |
if operation.done { | |
// ANCHOR_END: manual-if-done | |
// ANCHOR: manual-match-none | |
match &operation.result { | |
None => { | |
return Err("missing result for finished operation".into()); | |
} | |
// ANCHOR_END: manual-match-none | |
// ANCHOR: manual-match-error | |
Some(r) => { | |
return match r { | |
longrunning::model::operation::Result::Error(e) => { | |
Err(format!("{e:?}").into()) | |
} | |
// ANCHOR_END: manual-match-error | |
// ANCHOR: manual-match-success | |
longrunning::model::operation::Result::Response(any) => { | |
let response = | |
any.try_into_message::<speech::model::BatchRecognizeResponse>()?; | |
Ok(response) | |
} | |
// ANCHOR_END: manual-match-success | |
// ANCHOR: manual-match-default | |
_ => Err(format!("unexpected result branch {r:?}").into()), | |
// ANCHOR_END: manual-match-default | |
}; | |
} | |
} | |
} | |
// ANCHOR: manual-metadata | |
if let Some(any) = &operation.metadata { | |
let metadata = any.try_into_message::<speech::model::OperationMetadata>()?; | |
println!("LRO in progress, metadata={metadata:?}"); | |
} | |
// ANCHOR_END: manual-metadata | |
// ANCHOR: manual-backoff | |
tokio::time::sleep(std::time::Duration::from_millis(500)).await; | |
// ANCHOR_END: manual-backoff | |
// ANCHOR: manual-poll-again | |
if let Ok(attempt) = client.get_operation(operation.name.clone()).send().await { | |
operation = attempt; | |
} | |
// ANCHOR_END: manual-poll-again |
Beyond errors and LROs, there are 50 or so fields of type google.protobuf.Any
. Considering there are thousands of fields, that is rare indeed.
I think we can minimize the concerns around the usability of Any
if we extract the error details and we handle the LROs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that's good context.
I think we can minimize the concerns around the usability of Any if we extract the error details and we handle the LROs.
👍
* feat(generator/dart): implement the 'Any' WKT
Implement the 'Any' WKT:
fullyQualifiedName
static member to every generated messageAny
well-known type (this is entirely hand-written, and lives in thegoogle_cloud_protobuf/lib/src/protobuf.p.dart
file)unpackFrom()
andpackInto()
as the methods to deserialize and serialize messages, but these could also bedeserializeFrom()
/serializeInto()
, ...