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

Add a 'UserInfo' heterotyped dictionary #1031

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Nov 4, 2020

Motivation:

Interceptors may be used to provide additional information to the
service provider. This could include information about an authenticated
user, for example. However, we don't currently have such a mechanism.

Modifications:

  • Add a type-safe 'UserInfo' dictionary
  • Add a 'UserInfo' requirement to the 'ServerCallContext' protocol
  • Store a 'Ref' in the base call handler and pipeline,
    exposing the 'UserInfo' in both the server call context and server
    interceptor context.

Result:

Interceptors can share information in a type-safe way with the service
provider.

@glbrntt glbrntt requested a review from Lukasa November 4, 2020 15:15
@glbrntt glbrntt added the 🆕 semver/minor Adds new public API. label Nov 4, 2020
@glbrntt
Copy link
Collaborator Author

glbrntt commented Nov 4, 2020

cc @Davidde94

* limitations under the License.
*/

internal final class Ref<Value> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extreme nit: If it's internal, does it really need to be final? (this doesn't change anything, just food for thought)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do this out of habit to be honest.

/// Metadata to return at the end of the RPC. If this is required it should be updated before
/// the `responsePromise` or `statusPromise` is fulfilled.
public var trailers = HPACKHeaders()

public init(eventLoop: EventLoop, headers: HPACKHeaders, logger: Logger) {
public convenience init(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it's worth documenting behaviour re default UserInfo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, to be honest. These context types are only public because they were created as public. They're not particularly useful by themselves.

A little more context: a subclass of this will be provided to the user provided method handler (i.e. the implementations of methods on their service), but of course the user isn't responsible for creating that context. Indeed the actual context provided is internal. The only time a user would want to create their own context would be to unit test their service provider in isolation, and we provide testing contexts just for that. I guess to sum up: documentation isn't useful because this API isn't really useful. And we're only keeping the API to not break existing users.

@@ -31,12 +31,26 @@ open class StreamingResponseCallContext<ResponsePayload>: ServerCallContextBase

public let statusPromise: EventLoopPromise<GRPCStatus>

override public init(eventLoop: EventLoop, headers: HPACKHeaders, logger: Logger) {
public convenience init(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, up to you re documentation.

@@ -35,12 +35,26 @@ open class UnaryResponseCallContext<ResponsePayload>: ServerCallContextBase, Sta
public let responsePromise: EventLoopPromise<ResponsePayload>
public var responseStatus: GRPCStatus = .ok

override public init(eventLoop: EventLoop, headers: HPACKHeaders, logger: Logger) {
public convenience init(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs?


public protocol UserInfoKey {
/// The type of `Value` identified by this key.
associatedtype Value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems strange to use Value to describe Value. You could just say "The type identified by this key".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Changed.

Motivation:

Interceptors may be used to provide additional information to the
service provider. This could include information about an authenticated
user, for example. However, we don't currently have such a mechanism.

Modifications:

- Add a type-safe 'UserInfo' dictionary
- Add a 'UserInfo' requirement to the 'ServerCallContext' protocol
- Store a 'Ref<UserInfo>' in the base call handler and pipeline,
  exposing the 'UserInfo' in both the server call context and server
  interceptor context.

Result:

Interceptors can share information in a type-safe way with the service
provider.
@glbrntt glbrntt force-pushed the gb-server-interceptors-heterotyped-dict branch from a41246d to 94530ec Compare November 4, 2020 16:45
@glbrntt glbrntt merged commit 8e02898 into grpc:gb-feature-interceptors Nov 5, 2020
@glbrntt glbrntt deleted the gb-server-interceptors-heterotyped-dict branch November 5, 2020 08:59
glbrntt added a commit that referenced this pull request Nov 6, 2020
Motivation:

Interceptors may be used to provide additional information to the
service provider. This could include information about an authenticated
user, for example. However, we don't currently have such a mechanism.

Modifications:

- Add a type-safe 'UserInfo' dictionary
- Add a 'UserInfo' requirement to the 'ServerCallContext' protocol
- Store a 'Ref<UserInfo>' in the base call handler and pipeline,
  exposing the 'UserInfo' in both the server call context and server
  interceptor context.

Result:

Interceptors can share information in a type-safe way with the service
provider.
glbrntt added a commit that referenced this pull request Nov 12, 2020
Motivation:

Interceptors may be used to provide additional information to the
service provider. This could include information about an authenticated
user, for example. However, we don't currently have such a mechanism.

Modifications:

- Add a type-safe 'UserInfo' dictionary
- Add a 'UserInfo' requirement to the 'ServerCallContext' protocol
- Store a 'Ref<UserInfo>' in the base call handler and pipeline,
  exposing the 'UserInfo' in both the server call context and server
  interceptor context.

Result:

Interceptors can share information in a type-safe way with the service
provider.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants