From 0d20c005da086c3c1cadd835f43aea58fc4a6668 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Thu, 21 Jul 2022 09:39:55 +0100 Subject: [PATCH] feat: gRPC transcoding improvements This includes: - Implementing pattern matching - Implementing additional binding matching - Implementing the data-driven tests (I've changed HttpRulePathPattern to use a file-scoped namespace as so much of it was changing anyway that the diffs would already be a mess.) --- .../ConformanceTestData.cs | 37 ++ .../Rest/HttpRulePathPatternTest.cs | 33 +- .../Rest/HttpRuleTranscoderTest.cs | 95 +++++ .../Rest/ReadHttpResponseMessageTest.cs | 3 +- .../Rest/HttpRulePathPattern.cs | 332 ++++++++++++------ .../Rest/HttpRuleTranscoder.cs | 169 +++++---- Google.Api.Gax.Grpc/Rest/RestMethod.cs | 14 +- Google.Api.Gax.Grpc/Rest/TranscodingOutput.cs | 6 +- 8 files changed, 497 insertions(+), 192 deletions(-) create mode 100644 Google.Api.Gax.Grpc.Tests/ConformanceTestData.cs create mode 100644 Google.Api.Gax.Grpc.Tests/Rest/HttpRuleTranscoderTest.cs diff --git a/Google.Api.Gax.Grpc.Tests/ConformanceTestData.cs b/Google.Api.Gax.Grpc.Tests/ConformanceTestData.cs new file mode 100644 index 00000000..c6a44146 --- /dev/null +++ b/Google.Api.Gax.Grpc.Tests/ConformanceTestData.cs @@ -0,0 +1,37 @@ +/* + * Copyright 2022 Google LLC + * Use of this source code is governed by a BSD-style + * license that can be found in the LICENSE file or at + * https://developers.google.com/open-source/licenses/bsd + */ + +using System; +using System.IO; + +namespace Google.Api.Gax.Grpc.Tests; + +/// +/// Utility code for locating and loading test data. +/// +public static class ConformanceTestData +{ + /// + /// Finds the root directory of the repository by looking for common files. + /// + public static string FindRepositoryRootDirectory() + { + var currentDirectory = Path.GetFullPath("."); + var directory = new DirectoryInfo(currentDirectory); + while (directory != null && + (!File.Exists(Path.Combine(directory.FullName, "LICENSE")) + || !Directory.Exists(Path.Combine(directory.FullName, "Google.Api.CommonProtos")))) + { + directory = directory.Parent; + } + if (directory == null) + { + throw new InvalidOperationException("Unable to determine root directory. Please run within gax-dotnet repository."); + } + return directory.FullName; + } +} diff --git a/Google.Api.Gax.Grpc.Tests/Rest/HttpRulePathPatternTest.cs b/Google.Api.Gax.Grpc.Tests/Rest/HttpRulePathPatternTest.cs index c7b65205..d53dd7a6 100644 --- a/Google.Api.Gax.Grpc.Tests/Rest/HttpRulePathPatternTest.cs +++ b/Google.Api.Gax.Grpc.Tests/Rest/HttpRulePathPatternTest.cs @@ -12,7 +12,8 @@ namespace Google.Api.Gax.Grpc.Rest.Tests { public class HttpRulePathPatternTest { - public static TheoryData ValidPatternData = new TheoryData + // We want to end up with theory parameters that are all serializable for XUnit, but avoid calling ToString in each line of the test description. + public static TheoryData ValidPatternData = ConvertTheoryData(new TheoryData { { "x/y:custom", new RuleTestRequest(), "x/y:custom" }, { "firstPart/{x}/secondPart/{y}", new RuleTestRequest { X = "x1", Y = "y2" }, "firstPart/x1/secondPart/y2" }, @@ -24,31 +25,43 @@ public class HttpRulePathPatternTest { "pattern/{x=abc/**}", new RuleTestRequest { X = "abc/def/ghi" }, "pattern/abc/def/ghi" }, { "pattern/{x=**}", new RuleTestRequest { X = "abc/New York" }, "pattern/abc/New%20York" }, { "nested/{nested.a}", new RuleTestRequest { Nested = new RuleTestRequest.Types.Nested { A = "aaa" } }, "nested/aaa" }, - // The segment resolves to an empty string instead of failing - { "nested/{nested.a}/end", new RuleTestRequest(), "nested//end" } - }; + // The nested field isn't present, so this doesn't match. + { "nested/{nested.a}/end", new RuleTestRequest(), null }, + { "before/{int}/end", new RuleTestRequest { Int = 5 }, "before/5/end" }, + }); + + private static TheoryData ConvertTheoryData(TheoryData theoryData) + { + var ret = new TheoryData(); + foreach (var item in theoryData) + { + ret.Add((string) item[0], ((RuleTestRequest) item[1]).ToString(), (string) item[2]); + } + return ret; + } [Theory] [MemberData(nameof(ValidPatternData))] - public void ValidPattern(string pattern, RuleTestRequest request, string expectedFormatResult) + public void ValidPattern(string pattern, string requestJson, string expectedFormatResult) { var rulePathPattern = ParsePattern(pattern); - string actualFormatResult = rulePathPattern.Format(request); + var request = RuleTestRequest.Parser.ParseJson(requestJson); + string actualFormatResult = rulePathPattern.TryFormat(request); Assert.Equal(expectedFormatResult, actualFormatResult); } [Theory] - [InlineData("before/{unterminated-brace/end", Skip = "We don't detect this at the moment.")] + [InlineData("before/{unterminated-brace/end")] + [InlineData("before/unstarted-brace}/end")] + [InlineData("before/unstarted-brace}/{valid}/end")] [InlineData("before/{missing}/end")] [InlineData("before/{nested}/end")] - [InlineData("before/{int}/end")] [InlineData("before/{repeated}/end")] [InlineData("before/{map}/end")] public void InvalidPattern(string pattern) { Assert.Throws(() => HttpRulePathPattern.Parse(pattern, RuleTestRequest.Descriptor)); - } - + } private static HttpRulePathPattern ParsePattern(string pattern) => HttpRulePathPattern.Parse(pattern, RuleTestRequest.Descriptor); diff --git a/Google.Api.Gax.Grpc.Tests/Rest/HttpRuleTranscoderTest.cs b/Google.Api.Gax.Grpc.Tests/Rest/HttpRuleTranscoderTest.cs new file mode 100644 index 00000000..be4c1e28 --- /dev/null +++ b/Google.Api.Gax.Grpc.Tests/Rest/HttpRuleTranscoderTest.cs @@ -0,0 +1,95 @@ +/* + * Copyright 2022 Google LLC + * Use of this source code is governed by a BSD-style + * license that can be found in the LICENSE file or at + * https://developers.google.com/open-source/licenses/bsd + */ + +using Google.Api.Gax.Grpc.Tests; +using Google.Protobuf; +using Google.Protobuf.Reflection; +using Google.Protobuf.WellKnownTypes; +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using Xunit; +using Xunit.Abstractions; +using static Google.Api.Gax.Grpc.Rest.Tests.Test; + +namespace Google.Api.Gax.Grpc.Rest.Tests; + +public class HttpRuleTranscoderTest +{ + private static TestFile s_testFile = LoadTestFile(); + public static IEnumerable Tests => s_testFile.Tests + .Select(t => new object[] { new SerializableTest(t) }); + + private static TestFile LoadTestFile() + { + // TODO: Adjust when we move the tests to the google-cloud-conformance repo + var rootDirectory = ConformanceTestData.FindRepositoryRootDirectory(); + var path = Path.Combine(rootDirectory, "Google.Api.Gax.Grpc.Tests", "Rest", "transcoder_tests.json"); + string json = File.ReadAllText(path); + return TestFile.Parser.ParseJson(json); + } + + [Theory] + [MemberData(nameof(Tests))] + public void Transcode(SerializableTest testWrapper) + { + var test = testWrapper.Test; + var rule = test.RuleCase switch + { + RuleOneofCase.InlineRule => test.InlineRule, + RuleOneofCase.RuleName => s_testFile.NamedRules[test.RuleName], + _ => throw new ArgumentException("No rule specified") + }; + + var requestMessageDescriptor = TranscoderTestReflection.Descriptor.FindTypeByName(test.RequestMessageName); + var request = test.Request is null ? null : JsonParser.Default.Parse(test.Request.ToString(), requestMessageDescriptor); + + switch (test.ExpectedResultCase) + { + case ExpectedResultOneofCase.InvalidRule: + Assert.Throws(CreateTranscoder); + break; + case ExpectedResultOneofCase.NonmatchingRequest: + Assert.NotNull(request); + Assert.Null(CreateTranscoder().Transcode(request)); + break; + case ExpectedResultOneofCase.Success: + Assert.NotNull(request); + var actualResult = CreateTranscoder().Transcode(request); + var expectedResult = test.Success; + Assert.Equal(expectedResult.Method, actualResult.Method.Method); + Assert.Equal(expectedResult.Uri, actualResult.RelativeUri); + var actualBody = actualResult.Body is null ? null : Struct.Parser.ParseJson(actualResult.Body); + Assert.Equal(expectedResult.Body, actualBody); + break; + default: + throw new ArgumentException($"Unknown expected result case: {test.ExpectedResultCase}"); + } + + HttpRuleTranscoder CreateTranscoder() => new HttpRuleTranscoder(test.Name, requestMessageDescriptor, rule); + } +} + +public class SerializableTest : IXunitSerializable +{ + public Test Test { get; private set; } + + // Used in deserialization + public SerializableTest() { } + + public SerializableTest(Test test) => Test = test; + + public void Deserialize(IXunitSerializationInfo info) => + Test = Test.Parser.ParseFrom(info.GetValue(nameof(Test))); + + public void Serialize(IXunitSerializationInfo info) => + info.AddValue(nameof(Test), Test.ToByteArray()); + + public override string ToString() => Test?.Name ?? "(Unknown)"; +} + diff --git a/Google.Api.Gax.Grpc.Tests/Rest/ReadHttpResponseMessageTest.cs b/Google.Api.Gax.Grpc.Tests/Rest/ReadHttpResponseMessageTest.cs index 8fb60be5..ea894dba 100644 --- a/Google.Api.Gax.Grpc.Tests/Rest/ReadHttpResponseMessageTest.cs +++ b/Google.Api.Gax.Grpc.Tests/Rest/ReadHttpResponseMessageTest.cs @@ -5,7 +5,6 @@ * https://developers.google.com/open-source/licenses/bsd */ -using Google.Api.Gax.Grpc.Rest; using Google.Protobuf.WellKnownTypes; using Google.Rpc; using System.Net; @@ -13,7 +12,7 @@ using Xunit; using gc = Grpc.Core; -namespace Google.Api.Gax.Grpc.Tests.Rest +namespace Google.Api.Gax.Grpc.Rest.Tests { public class ReadHttpResponseMessageTest { diff --git a/Google.Api.Gax.Grpc/Rest/HttpRulePathPattern.cs b/Google.Api.Gax.Grpc/Rest/HttpRulePathPattern.cs index bdce0c61..1a0509d8 100644 --- a/Google.Api.Gax.Grpc/Rest/HttpRulePathPattern.cs +++ b/Google.Api.Gax.Grpc/Rest/HttpRulePathPattern.cs @@ -9,158 +9,262 @@ using Google.Protobuf.Reflection; using System; using System.Collections.Generic; +using System.Globalization; using System.Linq; +using System.Text; using System.Text.RegularExpressions; -namespace Google.Api.Gax.Grpc.Rest +namespace Google.Api.Gax.Grpc.Rest; + +/// +/// Representation of a pattern within a google.api.http option, such as "/v4/{parent=projects/*/tenants/*}/jobs". +/// The pattern is parsed once, and placeholders (such as "parent" in the above) are interpreted as fields within +/// a protobuf request message. The pattern can then be formatted later within the context of a request. +/// +internal sealed class HttpRulePathPattern { + private static readonly Regex s_braceRegex = new Regex(@"\{[^}]*\}"); + /// - /// Representation of a pattern within a google.api.http option, such as "/v4/{parent=projects/*/tenants/*}/jobs". - /// The pattern is parsed once, and placeholders (such as "parent" in the above) are interpreted as fields within - /// a protobuf request message. The pattern can then be formatted later within the context of a request. + /// The path segments - these are *not* slash-separated segments, but instead they're based on fields. So + /// /xyz/{a}/abc/{b} would contain four segments, "/xyz/", "{a}", "/abc/", "{b}". /// - internal sealed class HttpRulePathPattern - { - private static readonly Regex s_braceRegex = new Regex(@"\{[^}]*\}"); + private readonly IEnumerable _segments; - private readonly IEnumerable _segments; + private HttpRulePathPattern(List segments) => + _segments = segments; - private HttpRulePathPattern(List segments) => - _segments = segments; + internal static HttpRulePathPattern Parse(string pattern, MessageDescriptor requestDescriptor) + { + var segments = new List(); - internal static HttpRulePathPattern Parse(string pattern, MessageDescriptor requestDescriptor) + int currentIndex = 0; + while (currentIndex < pattern.Length) { - var segments = new List(); - var matches = s_braceRegex.Matches(pattern); - int lastEnd = 0; - foreach (Match match in matches) + int braceStart = pattern.IndexOf('{', currentIndex); + int braceEnd = pattern.IndexOf('}', currentIndex); + if (braceStart == -1) { - int start = match.Index; - if (start > lastEnd) + if (braceEnd != -1) { - string literal = pattern.Substring(lastEnd, start - lastEnd); - segments.Add(HttpRulePathPatternSegment.CreateFromLiteral(literal)); + throw new ArgumentException($"Resource pattern '{pattern}' is invalid: unmatched }}"); } - - // `startIndex: 1` for `{`; `match.Value.Length - 2` for `{}` - string boundFieldPath = match.Value.Substring(1, match.Value.Length - 2); - string boundFieldPattern = ""; - - int endOfName = match.Value.IndexOf('='); - if (endOfName != -1) - { - // `startIndex: 1` for `{`; `endOfName - 1` for `=` - boundFieldPath = match.Value.Substring(1, endOfName - 1); - // `endOfName + 1` for `=`; `match.Value.Length - 3` for `={}` - boundFieldPattern = match.Value.Substring(endOfName + 1, match.Value.Length - 3 - boundFieldPath.Length); - } - - segments.Add(HttpRulePathPatternSegment.CreateFromBoundField(boundFieldPath, boundFieldPattern, requestDescriptor)); - lastEnd = match.Index + match.Length; + break; } - if (lastEnd != pattern.Length) + // Either the closing brace comes before the opening one, or doesn't exist. + if (braceEnd < braceStart) { - string literal = pattern.Substring(lastEnd); - segments.Add(HttpRulePathPatternSegment.CreateFromLiteral(literal)); + throw new ArgumentException($"Resource pattern '{pattern}' is invalid: unmatched {{"); } - return new HttpRulePathPattern(segments); + if (braceStart != currentIndex) + { + string literal = pattern.Substring(currentIndex, braceStart - currentIndex); + segments.Add(new LiteralSegment(literal)); + } + segments.Add(new FieldSegment(pattern.Substring(braceStart, braceEnd - braceStart + 1), requestDescriptor)); + currentIndex = braceEnd + 1; + } + if (currentIndex != pattern.Length) + { + segments.Add(new LiteralSegment(pattern.Substring(currentIndex))); } + return new HttpRulePathPattern(segments); + } - internal string Format(IMessage message) => string.Join("", _segments.Select(segment => segment.Format(message))); + /// + /// Attempts to format the path with respect to the given request, + /// returning the formatted segment or null if formatting failed due to the request data. + /// + internal string TryFormat(IMessage message) + { + StringBuilder builder = new StringBuilder(); + var segments = string.Join("", _segments.Select(segment => segment.TryFormat(message))); + foreach (var segment in _segments) + { + var formattedSegment = segment.TryFormat(message); + if (formattedSegment is null) + { + return null; + } + builder.Append(formattedSegment); + } + return builder.ToString(); + } + + /// + /// Names of the fields of the top-level message that are bound by this pattern. + /// + internal List TopLevelFieldNames => _segments + .OfType() + .Select(s => s.TopLevelFieldName) + .ToList(); + private interface IPathSegment + { /// - /// Names of the fields of the top-level message that are bound by this pattern. + /// Formats this segment in the context of the given request, + /// returning null if the data in the request means that the path doesn't match. /// - internal List TopLevelFieldNames => _segments - .Where(segment => segment.TopLevelFieldName != null) - .Select(s => s.TopLevelFieldName) - .ToList(); + string TryFormat(IMessage request); + } + /// + /// A path segment that matches a field in the request. + /// + private sealed class FieldSegment : IPathSegment + { /// - /// A segment of the HTTP Rule pattern. + /// The separator between the field path and the pattern. /// - internal sealed class HttpRulePathPatternSegment - { - /// - /// Given a message, 'fill in' this segment's value. - /// - private readonly Func _formatter; - - /// - /// A name of the field in the top-level message that the segment is bound to - /// (e.g. for a binding `{foo.bar}`, this will be `foo`) - /// - internal string TopLevelFieldName { get; } - - private HttpRulePathPatternSegment(string topLevelFieldName, Func formatter) => - (TopLevelFieldName, _formatter) = (topLevelFieldName, formatter); - - internal string Format(IMessage message) => _formatter(message); - - /// - /// Creates a new path pattern segment from a literal path segment, e.g. `resources` in `v1/resources/{resource.id}` - /// - /// A literal path segment - /// A new HttpRulePathPatternSegment - internal static HttpRulePathPatternSegment CreateFromLiteral(string literal) => - new HttpRulePathPatternSegment(null, _ => literal); - - /// - /// Creates a new path pattern segment from a path segment with the bound field, e.g. `{resource.id}` in `v1/resources/{resource.id}` - /// - /// A - /// - /// - /// - internal static HttpRulePathPatternSegment CreateFromBoundField(string propertyPath, - string boundFieldPattern, MessageDescriptor descriptor) - { - int periodIndex = propertyPath.IndexOf('.'); - bool singleFieldPath = periodIndex == -1; - string fieldName = singleFieldPath ? propertyPath : propertyPath.Substring(0, periodIndex); + private static readonly char[] s_fieldPathPatternSeparator = { '=' }; + /// + /// The separator between fields within the field path. + /// + private static readonly char[] s_fieldPathSeparator = { '.' }; - var propertyAccessor = CreatePropertyAccessor(descriptor, propertyPath); + /// + /// The top-level field name, used to determine which fields should be populated as query parameters. + /// + internal string TopLevelFieldName { get; } - // Per `google/api/http.proto`, if the pattern has multiple path segments, such as `"{var=foo/*}"`, - // or if the pattern contains `**`, the `/` symbol should not be encoded, otherwise it should. - // TODO: [virost, 2021-12] Match pattern on the bound field, in addition to using it to select the escape function - Func escapeRegular = value => Uri.EscapeDataString(value); - Func escapeMultisegment = value => - string.Join("/", value.Split('/').Select(segment => Uri.EscapeDataString(segment))); + private readonly Regex _validationRegex; + private readonly Func _propertyAccessor; + private readonly bool _multiSegmentEscaping; - Func escape = boundFieldPattern.Contains("/") || boundFieldPattern.Contains("**") - ? escapeMultisegment - : escapeRegular; + /// + /// Creates a segment representing the given field text, with respect to + /// the given request descriptor. + /// + /// The text of the field segment, e.g. "{foo=*}" + /// The descriptor within which to find the field. + internal FieldSegment(string fieldText, MessageDescriptor descriptor) + { + // Strip the leading and trailing braces + fieldText = fieldText.Substring(1, fieldText.Length - 2); - return new HttpRulePathPatternSegment(fieldName, msg => escape(propertyAccessor(msg))); - } + // Split out the field path from the pattern it has to match (if any) + string[] bits = fieldText.Split(s_fieldPathPatternSeparator, 2); + string path = bits[0]; + string pattern = bits.Length == 2 ? bits[1] : "*"; + _multiSegmentEscaping = pattern.Contains("/") || pattern.Contains("**"); + _validationRegex = ParsePattern(pattern); - private static Func CreatePropertyAccessor(MessageDescriptor descriptor, string propertyPath) - { - int periodIndex = propertyPath.IndexOf('.'); - bool singleFieldPath = periodIndex == -1; - string fieldName = singleFieldPath ? propertyPath : propertyPath.Substring(0, periodIndex); - var field = descriptor.FindFieldByName(fieldName) ?? throw new ArgumentException($"Field {fieldName} not found in message {descriptor.FullName}"); - var expectedFieldType = singleFieldPath ? FieldType.String : FieldType.Message; + string[] fieldNames = path.Split(s_fieldPathSeparator); + TopLevelFieldName = fieldNames[0]; - if (field.FieldType != expectedFieldType || field.IsRepeated || field.IsMap) + Func messageSelector = message => message; + // Create a delegate to navigate down all the fields except the last. These must be singular message fields. + // TODO: Refactor the lambda expressions that all invoke the previous selector. They're somewhat repetitive. + for (int i = 0; i < fieldNames.Length - 1; i++) + { + var field = GetField(descriptor, fieldNames[i]); + if (field.FieldType != FieldType.Message) { - throw new ArgumentException($"Field {fieldName} in message {descriptor.FullName} cannot be used in a resource pattern"); + throw new ArgumentException($"Field {fieldNames[i]} in message {descriptor.FullName} cannot be used as non-final field in a resource pattern"); } + var previousSelector = messageSelector; + messageSelector = message => + { + var parent = previousSelector(message); + return parent is null ? null : (IMessage) field.Accessor.GetValue(previousSelector(parent)); + }; + descriptor = field.MessageType; + } + // Now handle the last field, which must be a singular integer or string field. + var lastFieldName = fieldNames.Last(); + var lastField = GetField(descriptor, lastFieldName); + _propertyAccessor = lastField.FieldType switch + { + FieldType.String => + message => + { + var parent = messageSelector(message); + return parent is null ? null : (string) lastField.Accessor.GetValue(parent); + }, + FieldType.Int32 or FieldType.SInt32 or FieldType.UInt32 or FieldType.Fixed32 or + FieldType.Int64 or FieldType.SInt64 or FieldType.UInt64 or FieldType.Fixed64 => + message => + { + var parent = messageSelector(message); + var value = parent is null ? null : ((IFormattable) lastField.Accessor.GetValue(parent)); + return value?.ToString(format: null, CultureInfo.InvariantCulture); + }, + _ => throw new ArgumentException($"Field {lastFieldName} in message {descriptor.FullName} cannot be used as a final field in a resource pattern") + }; - var accessor = field.Accessor; - if (singleFieldPath) + static FieldDescriptor GetField(MessageDescriptor descriptor, string name) + { + var field = descriptor.FindFieldByName(name); + if (field is null) + { + throw new ArgumentException($"Field {name} not found in message {descriptor.FullName}"); + } + if (field.IsRepeated || field.IsMap) { - return message => (string) accessor.GetValue(message); + throw new ArgumentException($"Field {name} in message {descriptor.FullName} cannot be used in a resource pattern"); } + return field; + } - var remainder = CreatePropertyAccessor(field.MessageType, propertyPath.Substring(periodIndex + 1)); - return message => + static Regex ParsePattern(string pattern) + { + var builder = new StringBuilder(); + int currentIndex = 0; + while (currentIndex < pattern.Length) { - var nested = (IMessage)accessor.GetValue(message); - return nested is null ? "" : remainder(nested); - }; + int starStart = pattern.IndexOf('*', currentIndex); + if (starStart < 0) + { + break; + } + builder.Append(Regex.Escape(pattern.Substring(currentIndex, starStart - currentIndex))); + int starEnd = starStart + 1; + while (starEnd < pattern.Length && pattern[starStart] == '*') + { + starEnd++; + } + int starCount = starEnd - starStart; + switch (starCount) + { + case 1: + builder.Append("[^/]+"); + break; + case 2: + builder.Append(".+"); + break; + default: + throw new ArgumentException($"Resource pattern '{pattern}' is invalid"); + } + currentIndex = starEnd; + } + return new Regex(builder.ToString(), RegexOptions.Compiled); + } + } + + public string TryFormat(IMessage request) + { + string result = _propertyAccessor(request); + if (result is null || _validationRegex?.IsMatch(result) == false) + { + return null; } + string escaped = _multiSegmentEscaping + ? string.Join("/", result.Split('/').Select(segment => Uri.EscapeDataString(segment))) + : Uri.EscapeDataString(result); + return escaped; } } + + /// + /// A path segment that is just based on a literal string. This always + /// succeeds, producing the same result every time. + /// + private sealed class LiteralSegment : IPathSegment + { + private string _literal; + + internal LiteralSegment(string literal) => _literal = literal; + + public string TryFormat(IMessage request) => _literal; + } } diff --git a/Google.Api.Gax.Grpc/Rest/HttpRuleTranscoder.cs b/Google.Api.Gax.Grpc/Rest/HttpRuleTranscoder.cs index 355ccbf2..6347166c 100644 --- a/Google.Api.Gax.Grpc/Rest/HttpRuleTranscoder.cs +++ b/Google.Api.Gax.Grpc/Rest/HttpRuleTranscoder.cs @@ -9,89 +9,140 @@ using Google.Protobuf.Reflection; using System; using System.Collections.Generic; +using System.Globalization; using System.Linq; using System.Net.Http; namespace Google.Api.Gax.Grpc.Rest; /// -/// A transcoder for a single HttpRule, not including any additional bindings. -/// A has one or more transcoders, applied in turn until -/// a match is found with a result. +/// A transcoder for an HttpRule, including any additional bindings, which are +/// applied in turn until a match is found with a result. /// internal sealed partial class HttpRuleTranscoder { - private static readonly FieldType[] TypesIneligibleForQueryStringEncoding = { FieldType.Group, FieldType.Message, FieldType.Bytes }; - private static readonly HttpMethod s_patchMethod = new HttpMethod("PATCH"); - - private readonly HttpMethod _httpMethod; - private readonly Func _bodyTranscoder; - private readonly HttpRulePathPattern _pathPattern; - private readonly List _queryParameterFields; + private readonly List _transcoders; /// - /// Creates a transcoder for the given method (name only for simplicity of testing) with the specified + /// Creates a transcoder for the given method (named only for error messages) with the specified /// request message descriptor and HttpRule. See AIP-127 (https://google.aip.dev/127) and the proto comments /// for google.api.HttpRule (https://github.com/googleapis/googleapis/blob/master/google/api/http.proto#L44-L312) /// /// Name of the method, used only for diagnostic purposes. - /// The descriptor for the message request type + /// The descriptor for the message request type /// The HttpRule that the new transcoder should represent, excluding any additional bindings. - internal HttpRuleTranscoder(string methodName, MessageDescriptor requestMessageDescriptor, HttpRule rule) + internal HttpRuleTranscoder(string methodName, MessageDescriptor requestMessage, HttpRule rule) { - (string pattern, _httpMethod) = rule.PatternCase switch - { - HttpRule.PatternOneofCase.Get => (rule.Get, HttpMethod.Get), - HttpRule.PatternOneofCase.Post => (rule.Post, HttpMethod.Post), - HttpRule.PatternOneofCase.Patch => (rule.Patch, s_patchMethod), - HttpRule.PatternOneofCase.Put => (rule.Put, HttpMethod.Put), - HttpRule.PatternOneofCase.Delete => (rule.Delete, HttpMethod.Delete), - HttpRule.PatternOneofCase.Custom => (rule.Custom.Path, new HttpMethod(rule.Custom.Kind)), - _ => throw new ArgumentException("HTTP rule has no pattern") - }; - - _pathPattern = HttpRulePathPattern.Parse(pattern, requestMessageDescriptor); - - (bool allFieldsInBody, string bodyName, _bodyTranscoder) = rule.Body switch + _transcoders = new List { new SingleRuleTranscoder(methodName, requestMessage, rule) }; + _transcoders.AddRange(rule.AdditionalBindings.Select(binding => new SingleRuleTranscoder(methodName, requestMessage, binding))); + } + + /// + /// Returns the transcoding result from the first matching HttpRule, or + /// null if no rules match. + /// + internal TranscodingOutput Transcode(IMessage request) => + _transcoders + .Select(t => t.Transcode(request)) + .FirstOrDefault(result => result is not null); + + /// + /// A transcoder for a single rule, ignoring additional bindings. + /// + private class SingleRuleTranscoder + { + private static readonly FieldType[] TypesIneligibleForQueryStringEncoding = { FieldType.Group, FieldType.Message, FieldType.Bytes }; + private static readonly HttpMethod s_patchMethod = new HttpMethod("PATCH"); + + private readonly HttpMethod _httpMethod; + private readonly Func _bodyTranscoder; + private readonly HttpRulePathPattern _pathPattern; + private readonly List _queryParameterFields; + + internal SingleRuleTranscoder(string methodName, MessageDescriptor requestMessage, HttpRule rule) { - // TODO: The body shouldn't contain any fields that are present in the URI. - "*" => (true, null, new Func(protoRequest => protoRequest.ToString())), - "" => (false, null, new Func(protoRequest => null)), - string name when requestMessageDescriptor.FindFieldByName(name) is FieldDescriptor field => (false, name, new Func(protoRequest => field.Accessor.GetValue(protoRequest).ToString())), - _ => throw new ArgumentException($"Method {methodName} has a body parameter {rule.Body} in the 'google.api.http' annotation which is not a field in {requestMessageDescriptor.Name}") - }; - - _queryParameterFields = new List(); - if (!allFieldsInBody) + (string pattern, _httpMethod) = rule.PatternCase switch + { + HttpRule.PatternOneofCase.Get => (rule.Get, HttpMethod.Get), + HttpRule.PatternOneofCase.Post => (rule.Post, HttpMethod.Post), + HttpRule.PatternOneofCase.Patch => (rule.Patch, s_patchMethod), + HttpRule.PatternOneofCase.Put => (rule.Put, HttpMethod.Put), + HttpRule.PatternOneofCase.Delete => (rule.Delete, HttpMethod.Delete), + HttpRule.PatternOneofCase.Custom => (rule.Custom.Path, new HttpMethod(rule.Custom.Kind)), + _ => throw new ArgumentException("HTTP rule has no pattern") + }; + + _pathPattern = HttpRulePathPattern.Parse(pattern, requestMessage); + + (bool allFieldsInBody, string bodyName, _bodyTranscoder) = rule.Body switch + { + // TODO: The body shouldn't contain any fields that are present in the URI. + "*" => (true, null, new Func(protoRequest => protoRequest.ToString())), + "" => (false, null, new Func(protoRequest => null)), + string name when requestMessage.FindFieldByName(name) is FieldDescriptor field => (false, name, new Func(protoRequest => field.Accessor.GetValue(protoRequest).ToString())), + _ => throw new ArgumentException($"Method {methodName} has a body parameter {rule.Body} in the 'google.api.http' annotation which is not a field in {requestMessage.Name}") + }; + + _queryParameterFields = new List(); + if (!allFieldsInBody) + { + var usedFieldNames = new HashSet(_pathPattern.TopLevelFieldNames); + if (bodyName != null) + { + usedFieldNames.Add(bodyName); + } + + var queryStringParamsEligibleFields = requestMessage.Fields.InDeclarationOrder() + .Where(f => !TypesIneligibleForQueryStringEncoding.Contains(f.FieldType)); + + _queryParameterFields = queryStringParamsEligibleFields + .Where(field => !usedFieldNames.Contains(field.Name)) + .OrderBy(field => field.JsonName, StringComparer.Ordinal) + .ToList(); + } + } + + private const uint UnsignedInt32Zero = 0; + + /// + /// Attempts to use this rule to transcode the given request. + /// + /// The request to transcode. Must not be null. + /// The result of transcoding, or null if the rule did not match. + internal TranscodingOutput Transcode(IMessage request) { - var usedFieldNames = new HashSet(_pathPattern.TopLevelFieldNames); - if (bodyName != null) + string path = _pathPattern.TryFormat(request); + if (path is null) { - usedFieldNames.Add(bodyName); + return null; } + string body = _bodyTranscoder(request); + var queryParameters = from field in _queryParameterFields + from value in GetValues(field, request) + select new KeyValuePair(field.JsonName, value); + return new TranscodingOutput(_httpMethod, path, queryParameters, body); - var queryStringParamsEligibleFields = requestMessageDescriptor.Fields.InDeclarationOrder() - .Where(f => !TypesIneligibleForQueryStringEncoding.Contains(f.FieldType)); + static IEnumerable GetValues(FieldDescriptor field, IMessage request) + { + if (field.HasPresence && !field.Accessor.HasValue(request)) + { + yield break; + } + + object value = field.Accessor.GetValue(request); + if (!field.HasPresence && (value is "" || value is 0.0f || value is 0.0d || value is 0 || value is 0L || value is 0UL || value is UnsignedInt32Zero)) + { + yield break; + } + yield return value is IFormattable formattable + ? formattable.ToString(format: null, CultureInfo.InvariantCulture) + : value?.ToString(); - _queryParameterFields = queryStringParamsEligibleFields.Where(field => !usedFieldNames.Contains(field.Name)).ToList(); + // TODO: Handle repeated fields + // TODO: Handle message fields (currently prohibited via TypesIneligibleForQueryStringEncoding, but theoretically valid) + } } } - /// - /// Attempts to use this rule to transcode the given request. - /// - /// The request to transcode. Must not be null. - /// The result of transcoding, or null if the rule did not match. - internal TranscodingOutput Transcode(IMessage request) - { - string body = _bodyTranscoder(request); - string path = _pathPattern.Format(request); - var queryParameters = _queryParameterFields - .Where(field => !field.HasPresence || field.Accessor.HasValue(request)) - // TODO: Format the field value with the invariant culture. - // TODO: Handle repeated fields - // TODO: Handle message fields (currently prohibited via TypesIneligibleForQueryStringEncoding, but theoretically valid) - .ToDictionary(field => field.JsonName, field => field.Accessor.GetValue(request).ToString()); - return new TranscodingOutput(_httpMethod, path, queryParameters, body); - } + } diff --git a/Google.Api.Gax.Grpc/Rest/RestMethod.cs b/Google.Api.Gax.Grpc/Rest/RestMethod.cs index 28d98624..ca57c886 100644 --- a/Google.Api.Gax.Grpc/Rest/RestMethod.cs +++ b/Google.Api.Gax.Grpc/Rest/RestMethod.cs @@ -9,6 +9,8 @@ using Google.Protobuf.Reflection; using Grpc.Core; using System; +using System.Collections.Generic; +using System.Linq; using System.Net.Http; using System.Threading.Tasks; @@ -25,6 +27,9 @@ internal class RestMethod private readonly JsonParser _parser; private readonly HttpRuleTranscoder _transcoder; + /// + /// The service-qualified method name, as used by gRPC, e.g. "/google.somepackage.SomeService/SomeMethod" + /// internal string FullName { get; } private RestMethod(MethodDescriptor protoMethod, JsonParser parser, HttpRuleTranscoder transcoder) => @@ -44,16 +49,17 @@ internal static RestMethod Create(MethodDescriptor method, JsonParser parser) { throw new ArgumentException($"Method {method.Name} in service {method.Service.Name} has no HTTP rule"); } - // TODO: create multiple rules with annotation bindings var transcoder = new HttpRuleTranscoder(method.FullName, method.InputType, rule); return new RestMethod(method, parser, transcoder); } internal HttpRequestMessage CreateRequest(IMessage request, string host) { - // TODO: Use multiple rules, iterating over them until a match is found. - var output = _transcoder.Transcode(request); - return output.CreateRequest(host); + var transcodingOutput = _transcoder.Transcode(request) + // TODO: Is this the right exception to use? + ?? throw new ArgumentException("Request could not be transcoded; it does not match any HTTP rule."); + + return transcodingOutput.CreateRequest(host); } // TODO: Handle cancellation? diff --git a/Google.Api.Gax.Grpc/Rest/TranscodingOutput.cs b/Google.Api.Gax.Grpc/Rest/TranscodingOutput.cs index dc1018a6..e08320e4 100644 --- a/Google.Api.Gax.Grpc/Rest/TranscodingOutput.cs +++ b/Google.Api.Gax.Grpc/Rest/TranscodingOutput.cs @@ -25,9 +25,9 @@ internal sealed class TranscodingOutput internal string Body { get; } internal HttpMethod Method { get; } - internal TranscodingOutput(HttpMethod method, string uriPath, Dictionary queryStringParameters, string body) => + internal TranscodingOutput(HttpMethod method, string uriPath, IEnumerable> queryStringParameters, string body) => (Method, RelativeUri, Body) = - (method, AppendQueryStringParameters(uriPath, queryStringParameters.OrderBy(kvp => kvp.Key, StringComparer.Ordinal)), body); + (method, AppendQueryStringParameters(uriPath, queryStringParameters), body); internal HttpRequestMessage CreateRequest(string host) { @@ -50,7 +50,7 @@ internal HttpRequestMessage CreateRequest(string host) /// The path component of the service URI /// The parameters to encode in the query string /// An uri path merged with the encoded query string parameters - private static string AppendQueryStringParameters(string uriPath, IOrderedEnumerable> queryStringParameters) + private static string AppendQueryStringParameters(string uriPath, IEnumerable> queryStringParameters) { var sb = new StringBuilder(); sb.Append(uriPath);