Mark old IGraphQLBuilder extension methods obsolete and create new ones #602
Conversation
|
I have yet to update the samples.... |
Welcome back! GraphQL.Net 4.6.x has been very stable with very few issues. The biggest outstanding issues relate to WebSockets in conjunction with a load balancer (#621), and federation. I don't use either so I'm having trouble helping. There is only two issues here merged but not released:
Also I finished migrating I think the next major version of the server project should eliminate Newtonsoft dependencies when using System.Text.Json. Also would like to fix the websockets problem. But I have not worked on either (partially since I don't use the server project in my applications). |
src/Transports.AspNetCore.NewtonsoftJson/Transports.AspNetCore.NewtonsoftJson.csproj
Outdated
Show resolved
Hide resolved
src/Transports.Subscriptions.Abstractions/Transports.Subscriptions.Abstractions.csproj
Outdated
Show resolved
Hide resolved
src/Transports.AspNetCore.SystemTextJson/Transports.AspNetCore.SystemTextJson.csproj
Outdated
Show resolved
Hide resolved
|
@sungam3r I'd like to merge this so I can use the new IGraphQLBuilder design in my projects that use server. Please lmk your thoughts. |
|
I'm in process of reading all that email from this and other projects: 87 left for server. I read every comment and review each PR. There is little time, a lot of work. Gradually, I will get to the current state of the project. Now I am somewhere in the July emails. |
|
@Shane32 This issue is the single one left for me to read about from server project. I will try to review tomorrow. |
|
I see you bumped version to 4.6.1 to use new APIs from GraphQL.NET so it will require me more time to figure out since I have not yet watched all changes there. |
|
Waiting for graphql-dotnet/graphql-dotnet#2811 to merge? |
Not necessarily. This PR is designed to work with GraphQL.NET v4.6.1+, and marks the old server project's |
|
Please note that I will be on the road for the next few days, so I may be a little slow to respond, but I'll try to review your PRs promptly as I'm able. |
| /// <returns>Reference to the passed <paramref name="builder"/>.</returns> | ||
| public static DI.IGraphQLBuilder AddGraphQLAuthorization(this DI.IGraphQLBuilder builder, Action<AuthorizationOptions> configure) | ||
| { | ||
| if (!(builder is IServiceCollection services)) |
There was a problem hiding this comment.
I would remove : IServiceCollection from GraphQLBuilder and add that:
public interface IServiceCollectionProvider
{
public IServiceCollection ServiceCollection { get; }
}
Then
internal class GraphQLBuilder : GraphQLBuilderBase, IServiceRegister, IServiceCollectionProvider
{
public override IServiceRegister Services => this;
public IServiceCollection ServiceCollection { get; }
}
+ remove
int ICollection<ServiceDescriptor>.Count => ServiceCollection.Count;
bool ICollection<ServiceDescriptor>.IsReadOnly => ServiceCollection.IsReadOnly;
ServiceDescriptor IList<ServiceDescriptor>.this[int index] { get => ServiceCollection[index]; set => ServiceCollection[index] = value; }
int IList<ServiceDescriptor>.IndexOf(ServiceDescriptor item) => ServiceCollection.IndexOf(item);
void IList<ServiceDescriptor>.Insert(int index, ServiceDescriptor item) => ServiceCollection.Insert(index, item);
void IList<ServiceDescriptor>.RemoveAt(int index) => ServiceCollection.RemoveAt(index);
void ICollection<ServiceDescriptor>.Add(ServiceDescriptor item) => ServiceCollection.Add(item);
void ICollection<ServiceDescriptor>.Clear() => ServiceCollection.Clear();
bool ICollection<ServiceDescriptor>.Contains(ServiceDescriptor item) => ServiceCollection.Contains(item);
void ICollection<ServiceDescriptor>.CopyTo(ServiceDescriptor[] array, int arrayIndex) => ServiceCollection.CopyTo(array, arrayIndex);
bool ICollection<ServiceDescriptor>.Remove(ServiceDescriptor item) => ServiceCollection.Remove(item);
IEnumerator<ServiceDescriptor> IEnumerable<ServiceDescriptor>.GetEnumerator() => ServiceCollection.GetEnumerator();
IEnumerator IEnumerable.GetEnumerator() => ((IEnumerable)ServiceCollection).GetEnumerator();
Then cast builder.Services to IServiceCollectionProvider
There was a problem hiding this comment.
That's fine (for v5 obviously). This is the 3rd option I mentioned. It will require a dependency on GraphQL.MicrosoftDI from the server project. Since it only works for Microsoft DI, the dependency makes sense.
GraphQLBuilder should remain public so that if someone wants to add additional initialization code to RegisterDefaultServices, they don't need to rewrite the entire class. There's no reason to make it internal/private.
There was a problem hiding this comment.
But... then project needs a reference to IServiceCollectionProvider... So... Let's just move : IServiceCollection from IGraphQLBuilder to IServiceRegister.
There was a problem hiding this comment.
If OK I'll do a PR to graphql-net/develop
There was a problem hiding this comment.
GraphQLBuilder should remain public so that if someone wants to add additional initialization code to RegisterDefaultServices, they don't need to rewrite the entire class. There's no reason to make it internal/private.
OK.
It will require a dependency on GraphQL.MicrosoftDI from the server project.
I think we will be able to go without it.
src/Authorization.AspNetCore/GraphQLBuilderAuthorizationExtensions.cs
Outdated
Show resolved
Hide resolved
src/Transports.AspNetCore.NewtonsoftJson/GraphQLBuilderNewtonsoftJsonExtensions.cs
Outdated
Show resolved
Hide resolved
| /// </param> | ||
| /// <returns>GraphQL Builder.</returns> | ||
| public static DI.IGraphQLBuilder AddNewtonsoftJson(this DI.IGraphQLBuilder builder, | ||
| Action<JsonSerializerSettings> configureDeserializerSettings = null, |
There was a problem hiding this comment.
Nullable annotations may be added later.
src/Transports.AspNetCore.SystemTextJson/GraphQLBuilderSystemTextJsonExtensions.cs
Outdated
Show resolved
Hide resolved
src/Transports.AspNetCore.SystemTextJson/GraphQLBuilderSystemTextJsonExtensions.cs
Show resolved
Hide resolved
| builder.Register<IUserContextBuilder, TUserContextBuilder>(DI.ServiceLifetime.Singleton); | ||
| builder.ConfigureExecution(async options => | ||
| { | ||
| if (options.UserContext == null || options.UserContext.Count == 0 && options.UserContext.GetType() == typeof(Dictionary<string, object>)) |
There was a problem hiding this comment.
Debatable.
After
options.UserContext = await contextBuilder.BuildUserContext(httpContext);
UserContext may become null.
There was a problem hiding this comment.
Also I doubt about BuildUserContext double call .
There was a problem hiding this comment.
From ExecutionOptions:
Probably it harms us.
| { | ||
| builder | ||
| .Register(typeof(IWebSocketConnectionFactory<>), typeof(WebSocketConnectionFactory<>), DI.ServiceLifetime.Transient) | ||
| .Register<IOperationMessageListener, LogMessagesListener>(DI.ServiceLifetime.Transient) |
There was a problem hiding this comment.
I would not add LogMessagesListener. At least add bool or fluent argument:
public static DI.IGraphQLBuilder AddWebSockets(this DI.IGraphQLBuilder builder, Action<IWebSocketsBuilder> configure)
Then:
services.AddGraphQL(b => b.AddWebSockets(opt => opt.AddLogging(...)));
There was a problem hiding this comment.
Keep in mind that one of the disadvantages of the project I see unnecessary dependencies, namely Microsoft.Extensions.Logging and Newtonsoft.Json.
There was a problem hiding this comment.
For sure. Right now even when using System.Text.Json there is a dependency on Newtonsoft.Json. That’s one of the things I want to solve when embedding both serialization and deserialization into GraphQL.SystemTextJson and GraphQL.NewtonsoftJson in an abstract fashion.
| .AddGraphQL((options, provider) => | ||
| .AddSingleton<ChatSchema>(); | ||
|
|
||
| MicrosoftDI.GraphQLBuilderExtensions.AddGraphQL(services) |
There was a problem hiding this comment.
Apparently, this is a temporary inconvenience.
There was a problem hiding this comment.
Yes. Although it is binary compatible it is probably not source compatible. There should be notes somewhere in here to that effect.
There was a problem hiding this comment.
(That may happen when using GraphQL 4.6.1 regardless of the changes in this PR.)
There was a problem hiding this comment.
There should be notes somewhere
I suggest to use GraphQL.NET documentation site as a single place for projects' documentation. I would to make new sidebar section for server project.
There was a problem hiding this comment.
|
I’ll fix it sometime today. |
Codecov Report
@@ Coverage Diff @@
## master #602 +/- ##
==========================================
- Coverage 55.09% 51.25% -3.85%
==========================================
Files 64 65 +1
Lines 1590 1715 +125
Branches 158 166 +8
==========================================
+ Hits 876 879 +3
- Misses 660 789 +129
+ Partials 54 47 -7
Continue to review full report at Codecov.
|
|
Can the version for the PR be released? Can't seem to find docs for an older version! |
|
Hopefully tomorrow. #688 |
|
#693 is coming 😉 |
See: graphql-dotnet/graphql-dotnet#2429
I don't like how the new SystemTextJson and NewtonsoftJson extension methods are handled. Currently the new methods (AddSystemTextJson and AddNewtonsoftJson) conflict with the methods of the same name in the base GraphQL library. But they are necessary because they support deserialization and websockets. I think that for the next major version, the remaining functionality should be moved into the base project. It's very little code and fits well there -- the base library's serialization projects already support serialization, so why not deserialization as well? However, as a non-breaking change, and to allow an upgrade path, we have the changes as proposed in this PR. (The next major version will remove the old IGraphQLBuilder interface.)