[WIP] Introduce IOperationMessageListener #1010
Conversation
| #pragma warning disable CA2208 // Instantiate argument exceptions correctly | ||
| if (request == null) | ||
| throw new ArgumentNullException(nameof(message) + "." + nameof(OperationMessage.Payload)); | ||
| var request = Serializer.ReadNode<GraphQLRequest>(message.Payload) |
There was a problem hiding this comment.
There was analyzer warning about null check simplification.
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #1010 +/- ##
==========================================
- Coverage 93.42% 93.26% -0.16%
==========================================
Files 44 44
Lines 2175 2184 +9
Branches 366 372 +6
==========================================
+ Hits 2032 2037 +5
- Misses 102 104 +2
- Partials 41 43 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
This seems to have a very limited use case. It cannot be used to change the protocol or execution at all. It can't be used to support new message types. Essentially, all it can be used for is logging -- at best, it could disconnect the client. Can you provide your use case for consideration? |
internal class SubscriptionHeadersListener : IOperationMessageListener
{
private readonly IServiceCallContextAccessor _serviceCallContextAccessor;
private readonly ServiceCallContextHttpFactory _serviceCallContextHttpFactory;
private readonly IGraphQLSerializer _serializer;
private readonly ILogger? _logger;
public SubscriptionHeadersListener(
IServiceCallContextAccessor serviceCallContextAccessor,
ServiceCallContextHttpFactory serviceCallContextHttpFactory,
IGraphQLSerializer serializer,
ILoggerFactory? loggerFactory = null)
{
_serviceCallContextAccessor = serviceCallContextAccessor ?? throw new ArgumentNullException(nameof(serviceCallContextAccessor));
_serviceCallContextHttpFactory = serviceCallContextHttpFactory ?? throw new ArgumentNullException(nameof(serviceCallContextHttpFactory));
_serializer = serializer;
_logger = loggerFactory?.CreateLogger(nameof(SubscriptionHeadersListener));
}
public Task BeforeHandleAsync(MessageHandlingContext context)
{
if (context == null)
return Task.CompletedTask;
try
{
var message = context.Message;
if (message.Payload != null)
{
if (message.Type == MessageType.GQL_CONNECTION_INIT)
{
var deserialized = _serializer.ReadNode<Dictionary<string, string?>>(message.Payload);
ModifyServiceCallContext(context, deserialized);
}
else if (message.Type == MessageType.GQL_START)
{
var deserialized = _serializer.ReadNode<Dictionary<string, object?>>(message.Payload);
if (deserialized != null)
{
bool headersPresent = deserialized.TryGetValue("headers", out object? headers) || deserialized.TryGetValue("Headers", out headers);
if (headersPresent)
ModifyServiceCallContext(context, _serializer.ReadNode<Dictionary<string, string?>>(headers));
}
}
}
}
catch (Exception e)
{
context.Terminated = true;
_logger?.LogError(
e,
"Some error {@OperationMessage}",
new OperationMessage
{
Id = context.Message?.Id,
Type = context.Message?.Type,
Payload = context.Message?.Payload?.ToString(),
});
}
return Task.CompletedTask;
}
private void ModifyServiceCallContext(MessageHandlingContext context, Dictionary<string, string?>? headers)
{
var scc = _serviceCallContextAccessor.ServiceCallContext;
if (scc == null)
{
return;
}
var httpContext = new DefaultHttpContext();
if (headers?.Count > 0)
{
foreach (var header in headers)
{
httpContext.Request.Headers.Add(header.Key, header.Value);
}
}
var result = _serviceCallContextHttpFactory.CreateServiceCallContextAsync(httpContext);
if (result.SCC != null)
{
scc.MessageProperties = result.SCC.MessageProperties;
scc.CancellationToken = result.SCC.CancellationToken;
scc.Items.Clear();
foreach (var item in result.SCC.Items)
{
scc.Items.TryAdd(item.Key, item.Value);
}
}
else
{
if (context != null)
context.Terminated = true;
_logger?.LogError(
"Some error {@OperationMessage}: {ServiceCallContextCreationError}",
new OperationMessage
{
Id = context?.Message?.Id,
Type = context?.Message?.Type,
Payload = context?.Message?.Payload?.ToString(),
},
result.Error);
}
}
public Task HandleAsync(MessageHandlingContext context) => Task.CompletedTask;
public Task AfterHandleAsync(MessageHandlingContext context)
{
var scc = _serviceCallContextAccessor.ServiceCallContext;
if (context.Message.Type == MessageType.GQL_START && scc != null)
{
scc.MessageProperties = null;
scc.CancellationToken = default;
scc.Items.Clear();
}
return Task.CompletedTask;
}
}+ extension method to plug in that class into DI: public static IServiceCollection AddSubscriptionHeadersServiceCallContextIntegration(this IServiceCollection services)
{
services
.AddServiceCallContextAccessor()
.AddTransient<IOperationMessageListener, SubscriptionHeadersListener>();
return services;
} |
I do not need all that. |
|
|
|
GraphQL.NET Server v7 already contains the necessary abstractions to implement this within the GQL_CONNECTION_INIT message (and the corresponding message for the newer protocol). In fact, there are two different interfaces that can be used. // abstract service call context initialization service
internal interface IServiceCallContextInitializer
{
void ModifyServiceCallContext(HttpContext context, Dictionary<string, string?>? deserializedPayload, CancellationToken cancellationToken);
}
// Technique 1:
//
// Add to GraphQLBuilder with:
// .AddWebSocketAuthentication<MyWebSocketAuthenticationService>();
internal class MyWebSocketAuthenticationService : IWebSocketAuthenticationService
{
private readonly IGraphQLSerializer _serializer;
private readonly IServiceCallContextInitializer _serviceCallContextInitializer;
public MyWebSocketAuthenticationService(IGraphQLSerializer serializer, IServiceCallContextInitializer serviceCallContextInitializer)
{
_serializer = serializer;
_serviceCallContextInitializer = serviceCallContextInitializer;
}
public Task AuthenticateAsync(IWebSocketConnection connection, string subProtocol, OperationMessage operationMessage)
{
var deserialized = _serializer.ReadNode<Dictionary<string, string?>>(operationMessage.Payload);
var cancellationToken = connection.RequestAborted; // also tied to application lifetime rather than HttpContext.RequestAborted
_serviceCallContextInitializer.ModifyServiceCallContext(connection.HttpContext, deserialized, cancellationToken);
// connection can be closed if necessary via connection.CloseAsync or connection.HttpContext.Abort
return Task.CompletedTask;
}
}
// Technique 2:
builder.AddUserContextBuilder((context, payload) =>
{
var serializer = context.RequestServices.GetRequiredService<IGraphQLSerializer>();
var deserialized = serializer.ReadNode<Dictionary<string, string?>>(payload);
var initializer = context.RequestServices.GetRequiredService<IServiceCallContextInitializer>();
initializer.ModifyServiceCallContext(context, deserialized, context.RequestAborted);
var userContext = new Dictionary<string, object?>();
return Task.FromResult(userContext);
});See: https://github.com/graphql-dotnet/server#authentication-for-websocket-requests |
|
Neither technique will allow for monitoring of GQL_START as this violates protocol (as the GQL_START payload should be a GraphQL request message aka GraphQLRequest) and could cause concurrency issues where the HttpContext (or ServiceCallContext in your case) can be modified while it is in use on another thread. Any headers-like data should be under the Extensions property, which you could access with a document listener. |
|
So
I do not know much about code that I migrate but I see that it intentionally minitors GQL_START and there are some comments about using single WebSocket connection for all subscriptions. The author of this code no longer works for the company. I'm having a hard time figuring out what to do next because I haven't used subscriptions myself. Now I'm thinking of temporarily commenting out the problematic code, migrating the main part, and then returning to the rest when the main part is working. It is possible that everything will be solved easier and I do not see it now. |
|
Just FYI, in both the old protocol and the new protocol, authentication information should be passed within the initialization message. This was the design and purpose of the initialization message. A major limitation of the prior Server implementation (before my rewrite) was that there was no easy way to access the message payload to authenticate the request. This was doubly-fixed by the two techniques shown above. This was done "twice" because it is common for people to either (a) set and rely on HttpContext.User, such as is needed for the authentication validation rule, and/or (b) hold authentication information within the UserContext. It is very likely that the prior employee implemented the workaround as you see it because there was no other way to access the initialization payload in that version of GraphQL.NET Server. |
|
I would suggest implementing one of the techniques that I mentioned, and see if your client applications work properly. If they follow spec, it should not be a problem. I am doubtful of adding interfaces to support out-of-spec clients. In this case the suggested interface has an extremely specific use case. Even the GQL_START message itself (along with the entire See https://github.com/apollographql/subscriptions-transport-ws
Note that the original design of Server 7 included an interface solely used to select a subscription server implementation for use. This would allow a user to add (or replace) a subscription protocol without having a custom http middleware implementation. During the design review process, we eliminated this interface, opting for simplicity. You can review this old design here: I think the current design is better, considering the limited use cases for custom subscription server implementations. |
Essentially, yes. But also note that the subscription server also implements proper error handling prior to running that code. Specifically, if GQL_CONNECTION_INIT was sent a second time over the same websocket connection, an error would be sent and the connection terminated, pursuant to protocol.
|
|
So far I have commented out the problem code and will return later when I will have repro to experiment with. |
sungam3r
changed the title
|
@sungam3r Can we close this? GraphQL.NET Server already has a hook designed to hook into GQL_CONNECTION_INIT based on the spec. And if the user is not designing to spec, it's not that hard to write a custom message processor to handle messages as desired. |
There was
IOperationMessageListenerin v6 and some our code uses it. Now I need to migrate that code to the-latest-and-greatest v7. My main goal is to achieve the "old behavior" without inheriting fromGraphQLHttpMiddlewareon our side since it creates complexity (combinations) with our other code that inherits fromGraphQLHttpMiddlewarein other way. So I looked throught the code and tried to find the way to hook up into message execution in the most soft way.