Hot Chocolate 14 Migration #2348
Conversation
| @@ -197,263 +196,260 @@ public static bool CreateAuthorizationDirectiveIfNecessary( | |||
| /// <returns>True when name resolution succeeded, false otherwise.</returns> | |||
| public static bool TryExtractGraphQLFieldModelName(IDirectiveCollection fieldDirectives, [NotNullWhen(true)] out string? modelName) | |||
| { | |||
| foreach (Directive dir in fieldDirectives) | |||
| foreach (Directive dir in fieldDirectives[ModelDirectiveType.DirectiveName]) | |||
There was a problem hiding this comment.
[Needs Discussion] The original code here looked wrong ... lets chat about the intention here.
|
@Aniruddh25, I will probably complete it this with whom shall I go through changes and questions? |
| @@ -189,7 +189,7 @@ private void OnNewFileContentsDetected(object? sender, EventArgs e) | |||
| /// <returns>True if the config was loaded, otherwise false.</returns> | |||
| public bool TryLoadConfig( | |||
| string path, | |||
| [NotNullWhen(true)] out RuntimeConfig? outConfig, | |||
| [NotNullWhen(true)] out RuntimeConfig? config, | |||
There was a problem hiding this comment.
[Info] I changed the name here to bring it back into alignment with the XML docs
| IReadOnlyList<AuthorizeDirective> directives, | ||
| CancellationToken cancellationToken = default) | ||
| { | ||
| throw new NotImplementedException(); |
There was a problem hiding this comment.
[TODO] This needs an implementation still
| @@ -683,6 +683,7 @@ void ProcessPaginationFields(IReadOnlyList<ISelectionNode> paginationSelections) | |||
| /// takes place which is required to fetch nested data. | |||
| /// </summary> | |||
| /// <param name="selections">Fields selection in the GraphQL Query.</param> | |||
| // TODO : This is inefficient and could lead to errors. we should rewrite this to use the ISelection API. | |||
There was a problem hiding this comment.
[TODO] This needs a rewrite on top of ISelection
| @@ -378,8 +378,8 @@ private void TestTypeNameChanges(DatabaseObject databaseobject, string objectNam | |||
| Assert.IsTrue(namespaceString.Contains("Azure.DataApiBuilder.Config.DatabasePrimitives")); | |||
| Assert.AreEqual(namespaceString, "Azure.DataApiBuilder.Config.DatabasePrimitives." + objectName); | |||
|
|
|||
| string projectNameString = typeNameSplitParts[1].Trim(); | |||
| Assert.AreEqual(projectNameString, "Azure.DataApiBuilder.Config"); | |||
| string projectstring = typeNameSplitParts[1].Trim(); | |||
There was a problem hiding this comment.
[TODO] Why change this?
| @@ -277,7 +277,6 @@ private void AddGraphQLService(IServiceCollection services, GraphQLRuntimeOption | |||
| }) | |||
| .AddHttpRequestInterceptor<IntrospectionInterceptor>() | |||
| .AddAuthorization() | |||
| .AllowIntrospection(false) | |||
There was a problem hiding this comment.
[Needs Discussion] AllowIntrospection is no longer needed. With HC 14 we have this as a default for production environment. There are a couple of other changes around this that we should discuss.
| // Allows DAB to override the HTTP error code set by HotChocolate. | ||
| // This is used to ensure HTTP code 4XX is set when the datatbase | ||
| // returns a "bad request" error such as stored procedure params missing. | ||
| .UseRequest<DetermineStatusCodeMiddleware>() |
There was a problem hiding this comment.
[Info] This middleware replaces the serializer. In HC 14 we can hint status codes we would want to use if the execution runs on the HTTP transport. This allows us to centralize the whole execution flow in one place.
michaelstaib
requested review from
Aniruddh25,
aaronburtle,
abhishekkumams and
RubenCerna2079
as code owners
michaelstaib
requested review from
ravishetye,
rohkhann,
neeraj-sharma2592,
sourabh1007 and
sezal98
as code owners
|
@JerryNixon can you trigger the tests on this? |
|
/azp run |
|
/azp run |
Why make this change?
With Hot Chocolate 14 we have invested a lot into security, performance and GraphQL protocol standards.
What is this change?
This PR will modernize the GraphQL stack of dab.
How was this tested?