Skip to content
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

Fix decoding of durations in MethodConfig #2093

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Oct 11, 2024

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

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
@glbrntt glbrntt added the semver/patch No public API change. label Oct 11, 2024
@glbrntt glbrntt enabled auto-merge (squash) October 11, 2024 12:33
@glbrntt glbrntt merged commit 7393a28 into grpc:main Oct 11, 2024
5 of 8 checks passed
@glbrntt glbrntt deleted the v2/fix-duration-conversion branch October 11, 2024 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants