From 455a3fc4914ffc8fa143e01ea5a859b9afb0fc99 Mon Sep 17 00:00:00 2001
From: George Barnett <gbarnett@apple.com>
Date: Fri, 11 Oct 2024 13:28:38 +0100
Subject: [PATCH] Fix decoding of durations in MethodConfig

Motivation:

MethodConfig uses a particular string based format for durations based
on the "google.protobuf.duration" message. On some decoding paths a
string was read and then decoded into a `Swift.Duration` rather than
decoding the `GoogleProtobufDuration` message directly.

The string-to-Swift.Duration path had a bug meaning fractional seconds
were incorrectly decoded.

Modifications:

- Add a test
- Remove the string to `Swift.Duration` path and always decode via
  `GoogleProtobufDuration` which has a correct implementation.

Result:

Fewer bugs
---
 .../GRPCCore/Configuration/MethodConfig.swift | 27 +++++--------------
 .../ServerCancellationManagerTests.swift      |  1 -
 .../MethodConfigCodingTests.swift             |  1 +
 3 files changed, 7 insertions(+), 22 deletions(-)

diff --git a/Sources/GRPCCore/Configuration/MethodConfig.swift b/Sources/GRPCCore/Configuration/MethodConfig.swift
index 9e1543636..13bafcf0e 100644
--- a/Sources/GRPCCore/Configuration/MethodConfig.swift
+++ b/Sources/GRPCCore/Configuration/MethodConfig.swift
@@ -393,21 +393,6 @@ private func validateMaxAttempts(_ value: Int) throws -> Int {
   return min(value, 5)
 }
 
-extension Duration {
-  fileprivate init(googleProtobufDuration duration: String) throws {
-    guard duration.utf8.last == UInt8(ascii: "s"),
-      let fractionalSeconds = Double(duration.dropLast())
-    else {
-      throw RuntimeError(code: .invalidArgument, message: "Invalid google.protobuf.duration")
-    }
-
-    let seconds = fractionalSeconds.rounded(.down)
-    let attoseconds = (fractionalSeconds - seconds) / 1e18
-
-    self.init(secondsComponent: Int64(seconds), attosecondsComponent: Int64(attoseconds))
-  }
-}
-
 extension MethodConfig: Codable {
   private enum CodingKeys: String, CodingKey {
     case name
@@ -506,12 +491,12 @@ extension RetryPolicy: Codable {
     let maxAttempts = try container.decode(Int.self, forKey: .maxAttempts)
     self.maxAttempts = try validateMaxAttempts(maxAttempts)
 
-    let initialBackoff = try container.decode(String.self, forKey: .initialBackoff)
-    self.initialBackoff = try Duration(googleProtobufDuration: initialBackoff)
+    let initialBackoff = try container.decode(GoogleProtobufDuration.self, forKey: .initialBackoff)
+    self.initialBackoff = initialBackoff.duration
     try Self.validateInitialBackoff(self.initialBackoff)
 
-    let maxBackoff = try container.decode(String.self, forKey: .maxBackoff)
-    self.maxBackoff = try Duration(googleProtobufDuration: maxBackoff)
+    let maxBackoff = try container.decode(GoogleProtobufDuration.self, forKey: .maxBackoff)
+    self.maxBackoff = maxBackoff.duration
     try Self.validateMaxBackoff(self.maxBackoff)
 
     self.backoffMultiplier = try container.decode(Double.self, forKey: .backoffMultiplier)
@@ -551,8 +536,8 @@ extension HedgingPolicy: Codable {
     let maxAttempts = try container.decode(Int.self, forKey: .maxAttempts)
     self.maxAttempts = try validateMaxAttempts(maxAttempts)
 
-    let delay = try container.decode(String.self, forKey: .hedgingDelay)
-    self.hedgingDelay = try Duration(googleProtobufDuration: delay)
+    let delay = try container.decode(GoogleProtobufDuration.self, forKey: .hedgingDelay)
+    self.hedgingDelay = delay.duration
 
     let statusCodes = try container.decode([GoogleRPCCode].self, forKey: .nonFatalStatusCodes)
     self.nonFatalStatusCodes = Set(statusCodes.map { $0.code })
diff --git a/Tests/GRPCCoreTests/Call/Server/Internal/ServerCancellationManagerTests.swift b/Tests/GRPCCoreTests/Call/Server/Internal/ServerCancellationManagerTests.swift
index 45c851de8..528ab88c3 100644
--- a/Tests/GRPCCoreTests/Call/Server/Internal/ServerCancellationManagerTests.swift
+++ b/Tests/GRPCCoreTests/Call/Server/Internal/ServerCancellationManagerTests.swift
@@ -65,7 +65,6 @@ struct ServerCancellationManagerTests {
   @Test("Remove cancellation handler")
   func removeCancellationHandler() async throws {
     let manager = ServerCancellationManager()
-    let signal = AsyncStream.makeStream(of: Void.self)
 
     let id = manager.addRPCCancelledHandler {
       Issue.record("Unexpected cancellation")
diff --git a/Tests/GRPCCoreTests/Configuration/MethodConfigCodingTests.swift b/Tests/GRPCCoreTests/Configuration/MethodConfigCodingTests.swift
index d9797343a..634ef3399 100644
--- a/Tests/GRPCCoreTests/Configuration/MethodConfigCodingTests.swift
+++ b/Tests/GRPCCoreTests/Configuration/MethodConfigCodingTests.swift
@@ -186,6 +186,7 @@ struct MethodConfigCodingTests {
         ("1s", .seconds(1)),
         ("1.000000s", .seconds(1)),
         ("0s", .zero),
+        ("0.1s", .milliseconds(100)),
         ("100.123s", .milliseconds(100_123)),
       ] as [(String, Duration)]
     )