-
Notifications
You must be signed in to change notification settings - Fork 14
Feat: Add predicate-based Subject and sugar syntax #15
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
Conversation
Should I add something like |
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 left a couple of small notes but otherwise this looks great.
Self(predicate: { descriptor in | ||
services.contains(descriptor.service) | ||
}) |
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.
You can use trailing closure syntax here (and in other places):
Self(predicate: { descriptor in | |
services.contains(descriptor.service) | |
}) | |
Self { descriptor in | |
services.contains(descriptor.service) | |
} |
/// - Parameters: | ||
/// - predicate: A `@Sendable` closure evaluated per RPC. Return `true` to intercept. | ||
public static func allMatching( | ||
_ predicate: @Sendable @escaping (MethodDescriptor) -> Bool |
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.
The Swift API Design Guidelines recommend naming closure parameters:
_ predicate: @Sendable @escaping (MethodDescriptor) -> Bool | |
_ predicate: @Sendable @escaping (_ descriptor: MethodDescriptor) -> Bool |
This is nice because tooling can take advantage of it to suggest the parameter name.
} | ||
/// An operation subject specifying an interceptor that will be applied to RPCs whose ``MethodDescriptor`` satisfies a `predicate`. | ||
/// | ||
/// - Important: The result of `predicate` is **cached per `MethodDescriptor`** by the client. |
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.
Nice note 👍
/// - Parameters: | ||
/// - services: The list of service descriptors for which this interceptor should **not** intercept RPCs. | ||
/// - methods: The list of method descriptors for which this interceptor should **not** intercept RPCs. | ||
public static func allExcluding( |
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.
Can you annotate this with @available(gRPCSwift 2.2, *)
?
/// | ||
/// - Parameters: | ||
/// - predicate: A `@Sendable` closure evaluated per RPC. Return `true` to intercept. | ||
public static func allMatching( |
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.
Can you annotate this with @available(gRPCSwift 2.2, *)
?
I updated the PR .apply(credentialsInterceptor, to: .allExcluding(services: [CredentialsService.descriptor], methods: [])) |
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.
Looks great, thanks very much @MathieuTricoire!
Head branch was pushed to by a user without write access
There was an issue in CI: https://github.com/grpc/grpc-swift-2/actions/runs/17046668344/job/48324929547 |
Agh, Swift 6.0 only. I'd try adding type annotations to the closure to see if that helps. |
), | ||
( | ||
.allMatching { (_ descriptor: MethodDescriptor) -> Bool in | ||
descriptor.method == "baz" | ||
}, | ||
[.fooBaz, .barBaz], | ||
[.fooBar, .barFoo] | ||
), | ||
] as [(ConditionalInterceptor<any Sendable>.Subject, [MethodDescriptor], [MethodDescriptor])] |
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.
Give it's fine on 6.1+ and newer, I think it's fine in this instance to compile out that test case on Swift 6.0:
), | |
( | |
.allMatching { (_ descriptor: MethodDescriptor) -> Bool in | |
descriptor.method == "baz" | |
}, | |
[.fooBaz, .barBaz], | |
[.fooBar, .barFoo] | |
), | |
] as [(ConditionalInterceptor<any Sendable>.Subject, [MethodDescriptor], [MethodDescriptor])] | |
), | |
#if compiler(>=6.1) | |
( | |
.allMatching { (_ descriptor: MethodDescriptor) -> Bool in | |
descriptor.method == "baz" | |
}, | |
[.fooBaz, .barBaz], | |
[.fooBar, .barFoo] | |
), | |
#endif | |
] as [(ConditionalInterceptor<any Sendable>.Subject, [MethodDescriptor], [MethodDescriptor])] |
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.
What do you think about moving the allMatching
case into its own test? (As it seems the issue is about the closure in @Test
)
It also made me notice that the current test is annotated with @available(gRPCSwift 2.0, *)
while the new subjects are available from 2.2
. I wouldn’t expect that to be the cause of the error, but it does feel odd to test 2.2
APIs inside a test marked 2.0
? Or are we ok with that?
Proposal: split the tests by availability, like this (Let me know how you would name the functions and tests):
@Suite("ConditionalInterceptor")
struct ConditionalInterceptorTests {
@Test(
"Applies to all, services and methods",
arguments: [
(
.all,
[.fooBar, .fooBaz, .barFoo, .barBaz],
[]
),
(
.services([ServiceDescriptor(package: "pkg", service: "foo")]),
[.fooBar, .fooBaz],
[.barFoo, .barBaz]
),
(
.methods([.barFoo]),
[.barFoo],
[.fooBar, .fooBaz, .barBaz]
),
] as [(ConditionalInterceptor<any Sendable>.Subject, [MethodDescriptor], [MethodDescriptor])]
)
@available(gRPCSwift 2.0, *)
func appliesTo(
target: ConditionalInterceptor<any Sendable>.Subject,
applicableMethods: [MethodDescriptor],
notApplicableMethods: [MethodDescriptor]
) {
for applicableMethod in applicableMethods {
#expect(target.applies(to: applicableMethod))
}
for notApplicableMethod in notApplicableMethods {
#expect(!target.applies(to: notApplicableMethod))
}
}
@Test(
"Applies to only and allExcluding",
arguments: [
(
.only(services: [.foo], methods: [.barFoo]),
[.fooBar, .fooBaz, .barFoo],
[.barBaz]
),
(
.allExcluding(services: [.foo], methods: [.barFoo]),
[.barBaz],
[.fooBar, .fooBaz, .barFoo]
),
] as [(ConditionalInterceptor<any Sendable>.Subject, [MethodDescriptor], [MethodDescriptor])]
)
@available(gRPCSwift 2.2, *)
func appliesToOnlyAndAllExcluding(
target: ConditionalInterceptor<any Sendable>.Subject,
applicableMethods: [MethodDescriptor],
notApplicableMethods: [MethodDescriptor]
) {
for applicableMethod in applicableMethods {
#expect(target.applies(to: applicableMethod))
}
for notApplicableMethod in notApplicableMethods {
#expect(!target.applies(to: notApplicableMethod))
}
}
@Test("Applies to all matching")
@available(gRPCSwift 2.2, *)
func appliesToAllMatching() {
let target = ConditionalInterceptor<any Sendable>.Subject.allMatching { descriptor in
descriptor.method == "baz"
}
let applicableMethods: [MethodDescriptor] = [.fooBaz, .barBaz]
let notApplicableMethods: [MethodDescriptor] = [.fooBar, .barFoo]
for applicableMethod in applicableMethods {
#expect(target.applies(to: applicableMethod))
}
for notApplicableMethod in notApplicableMethods {
#expect(!target.applies(to: notApplicableMethod))
}
}
}
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 fine with that. The availability in tests doesn't matter much to be honest.
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.
FWIW: the Check generated code (pull_request) job is fixed on main
so if you update your branch with the base branch the job should pass.
034fd7f
to
6743dce
Compare
6743dce
to
84858e0
Compare
Motivation
When using
ConditionalInterceptor.Subject
it's currently only possible to target.all
,.services
, or.methods
.This makes it difficult to express common cases like "apply this interceptor to all methods except a small set" without manually listing every included method or service. For example applying authentication everywhere except health check and authentication services.
Modifications
ConditionalInterceptor.Subject
to store a singlepredicate
closure instead of an enum of cases.This simplifies the matching mechanism: every variant is now expressed as a
predicate
overMethodDescriptor
..all
,.services
,.methods
) as convenience "sugar" static functions that construct the appropriatepredicate
..only(services:methods:)
— applies only to the given services and/or methods..allExcluding(services:methods:)
— applies to all except the given services and/or methods..allMatching(_:)
— applies to all methods satisfying a user-provided predicate.Result
Users can now express inclusion/exclusion rules more naturally:
Fixes #14