search

protobuf-net / protobuf-net.Grpc

GRPC bindings for protobuf-net and grpc-dotnet
Other
850 stars 108 forks source link

Recognize well-known types #7

Open mgravell opened 5 years ago

mgravell commented 5 years ago

for APIs that involve, say, DateTime (rather than custom message types) we should try to pick the most reasonable looking well-known-type and use that;

obvious candidates:

the following probably already work fine (not sure about byte[]), as "assume a message with a single field with index 1" is the default behavior already:

Note: with the above, we should probably hard remove the ability to opt in/out of data-contract types on the marshaller; it should be "nope, it is either something we recognize, or a data-contract type".

Note: we should also think about null in the above; since outer-most message don't include a length prefix, this could be awkward - since wrappers.proto is proto3, that means that there is no way of expressing a difference between 0 and null. Maybe we just don't support nulls ?

less obvious - the only ones I can see us even considering are Type and Api

jakubsuchybio commented 2 years ago

Any updates on this? Thx especially on fieldMasks

ghost commented 2 years ago

@mgravell I'm really interested is support for google.protobuf.Struct. I saw you mentioned that you might consider adding support for struct.proto in this issue as well. Is it on your radar to implement any time soon? P.S. I see you removed "help wanted" label from this one, but if there is any way I could contribute (probably will need some guidance) - let me know.

codymullins commented 4 months ago

Hey @mgravell I'd like to add support for field_mask.proto, even if just on a fork or an extension to this library. Could you give any guidance or advice on a high level of how you'd implement this? Would like to stay close to your vision in case it's something you'd want to pull in later.

mgravell commented 4 months ago

honestly, I can't think of any good ways of fitting this retrospectively; I had in mind trying to add this as part of the AOT work, because I genuinely think it needs a very different approach; my idea was (approximately):

abstract class FieldMask {
    public static FieldMask All {get;} = new OpenFieldMask(); 
    public abstract bool Include(string field, [NotNullWhen(true)] out FieldMask? children);

    sealed class FilteredFieldMask : FieldMask
    {
        private readonly Dictionary<string, FieldMask> fields = ...
        public override bool Include(string field, [NotNullWhen(true)] out FieldMask? children)
            => fields.TryGetValue(field, out children);
    }
    sealed class OpenFieldMask : FieldMask
    {
        public override bool Include(string field, [NotNullWhen(true)] out FieldMask? children)
        {
            children = All;
            return true;
        }
    }
}

with some kind of parse step that normalizes string[] (or similar) into the composed field maps, then changing "serialize" and "deserialize" to add a logical if (mask.Include("a", out var children)) { /* serialize, passing down children */}; I think that covers the core semantic requirements, but it is not trivial to add to the IL code, hence my idea of adding it in the AOT work.

codymullins commented 4 months ago

Is the AOT work branched anywhere? I might be missing it (but it also might not be started yet -- which is fine)

mgravell commented 4 months ago

This is ultimately protobuf-net thing, so mostly over in protobuf-net.BuildTools (although there is a recent branch here too), however: most of the existing work needs to be massively refactored - largely a reference pie e and a "now that we know all those gaps" thing. I need to carve a big chunk o' time. It isn't small.

The change you want is one I support, but I don't think it is one you can just squeeze in quickly - although I do support the enthusiasm.

codymullins commented 4 months ago

Understood; would love to help with this effort, so please let me know when and how I can contribute!

codymullins commented 4 months ago

Here are some examples of how we are using this "pattern" in our app - it's not fancy or pretty, but it's us shimming in support in a few places as we build out our APIs and background tasks that use the masking feature. Hopefully this helps add a real world scenario outside of what you already may be thinking.

Updating specific data on an artifact, this is on the server:

if (request.UpdateMask is not null)
{
    if (request.UpdateMask.Fields.Contains("ArtifactStage"))
    {
        artifact.ArtifactStage = request.ArtifactDetails.ArtifactStage;
    }

    if (request.UpdateMask.Fields.Contains("ImageSha"))
    {
        artifact.ImageSha = request.ArtifactDetails.ImageSha;
    }
}
else
{
    artifact.ArtifactStage = request.ArtifactDetails.ArtifactStage;
    artifact.ImageSha = request.ArtifactDetails.ImageSha;
}

Setting a deployment as failed, this is on the client/background task:

await hostingService.UpdateDeployment(
    new UpdateDeploymentRequest
    {
        DeploymentId = context.Data.DeploymentId.Value,
        DeployStatus = DeployStatus.Error,
        UpdateMask = ["DeployStatus"]
    }, context.CallContext());