Upgrade Hot-Chocolate to v15 Cherry-Pick for 1.5 #2704
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RubenCerna2079
requested review from
Aniruddh25,
aaronburtle,
neeraj-sharma2592,
ravishetye,
sezal98 and
sourabh1007
as code owners
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
Contributor
There was a problem hiding this comment.
Pull Request Overview
A backport of Hot Chocolate GraphQL introspection support and new caching configuration into the 1.5 branch, alongside CI task updates and minor cleanup.
- Integrate HotChocolate introspection client and refactor schema fetching in
Exporter.cs - Add
cacheEnabled/cacheTtloptions to CLI commands, runtime config, and tests - Replace NuGet restore tasks with
DotNetCoreCLI@2in pipelines and bump SNI license version
Reviewed Changes
Copilot reviewed 119 out of 119 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Cli/Exporter.cs | Refactored GetGraphQLSchema, added CreateIntrospectionClient |
| src/Cli/ConfigGenerator.cs | Hooked up ConstructCacheOptions in entity creation/update |
| src/Cli/Commands/AddOptions.cs | Added cacheEnabled and cacheTtl parameters |
| src/Cli/Commands/UpdateOptions.cs | Added cacheEnabled and cacheTtl parameters |
| src/Cli/Commands/EntityOptions.cs | Exposed CacheEnabled and CacheTtl CLI flags |
| src/Cli/Cli.csproj | Added HotChocolate.Utilities.Introspection package |
| src/Cli.Tests/UpdateEntityTests.cs | Introduced caching test and new snapshot |
| src/Cli.Tests/AddEntityTests.cs | Added caching-enabled entity test |
| src/Cli.Tests/ModuleInitializer.cs | Ignored new cache TTL fields in verification |
| schemas/dab.draft.schema.json | Defined cache-ttl-seconds in JSON schema |
| scripts/notice-generation.ps1 | Updated SQL Client SNI license version |
| .pipelines/**/*.yml | Switched NuGet restore to DotNetCoreCLI@2 tasks |
| src/Auth/IAuthorizationResolver.cs | Minor XML doc punctuation fixes |
Comments suppressed due to low confidence (2)
src/Cli/Exporter.cs:173
- [nitpick] Blocking on a Task using
Wait()can cause thread-pool starvation and wraps exceptions inAggregateException. Consider using.GetAwaiter().GetResult()for synchronous calls or refactoringGetGraphQLSchemato be async and await the introspection call.
response.Wait();
src/Cli/Exporter.cs:47
- The original code also guarded against
runtimeConfigbeing null. IfTryLoadConfigcan return true with a nullruntimeConfig, this removal could lead to a null-reference exception downstream. Consider restoring the null check or validatingruntimeConfigbefore use.
if (!loader.TryLoadConfig(runtimeConfigFile, out RuntimeConfig? runtimeConfig, replaceEnvVar: true))
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
Collaborator
|
nit: please mention it is Hot Chocolate "upgrade" to v15 in the PR title |
RubenCerna2079
changed the title
RubenCerna2079
force-pushed
the
dev/rubencerna/hot-chocolate-cherry-pick
branch
from
f0d6e8b to
22137b3
Compare
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
Resolved #2645 We need to take the plural value for the creating the http payload for the graphql request. Further we need to camel case this value instead of using the objects value which I was currently using Local Testing 1. Type.Plural not provided and entity name is not the same as table name 2. Type.Plural not provided and entity name is caps 3. Type.Plural not provided and entity name is all small 4. source.objects is provided as `dbo.books` 5. Type.Plural provided but in all small case 6. Type.Plural provided in all caps <img width="310" alt="image" src="https://github.com/user-attachments/assets/26c35c70-34a0-40d9-b344-90d7c31d117f" /> <img width="234" alt="image" src="https://github.com/user-attachments/assets/b3385e59-c9ee-4ad7-8fdb-2555bfaf16ed" /> --------- Co-authored-by: sezalchug <[email protected]>
…#2617) - Closes #2554 - Enhances OTEL instrumentation with custom traces and metrics for the REST APIs This PR enhances the OTEL instrumentation for the REST APIs by adding custom traces and metrics. I have removed ASP NET Core standard instrumentation since it does not provide great value given the custom nature of the webservice. I have written two main Helper classes: `TelemetryMetricsHelper` and `TelemetryTracesHelper` to provide a single point of management for custom traces and metrics. Metrics can be filtered for `status_code`, `api_type`, `endpoint` and `method`. I have also fixed the loggings which are now sent to the configured OTEL endpoint.      - [ ] Integration Tests - [ ] Unit Tests To test everything locally I recommend using [this repo](https://github.com/tommasodotNET/dab-workbench) that allows to run the local build of the dab cli and send metrics to the .NET Aspire OTEL endoint. --------- Co-authored-by: Aaron Powell <[email protected]> Co-authored-by: RubenCerna2079 <[email protected]> Co-authored-by: aaronburtle <[email protected]> Co-authored-by: Ruben Cerna <[email protected]> Co-authored-by: Aniruddh Munde <[email protected]>
Internal Issue Resolved Created a check before creating the HttpClient where the URI would be validated Conditions 1. It ensures the URI is absolute. 2. It checks for valid HTTP/HTTPS schemes. 3. Disallow empty hostnames Working as expected Code QL resolution would be checked after merging and running the pipelines --------- Co-authored-by: sezalchug <[email protected]> Co-authored-by: aaronburtle <[email protected]>
Closes [#2534](#2534) This PR involves creating another parameter in the runtime config which defines the cache-ttl-seconds for the health endpoint. + I am using the IMemoryCache to define the cache which has the CacheKey: `HealthCheckResponse` in ComprehensiveResponseWriter and saves the serialized version of the response in the cache. + The changes in Convertor Factory are in respect to the serialization and deserialization of this attribute appropriately. We consider default as 5 seconds in case not provided in the config file. And in case this is 0, caching is disabled. + Added timestamp to the health check report - [x] Integration Tests - [x] Unit Tests Call this CURL curl --request GET \ --url http://localhost:5000/health \ --header 'User-Agent: insomnia/10.3.0' Define the cache value as 5. Then in case of hitting the request without any delay, we get the same response back from the cache. In case hitting after the delay, we get a new response. --------- Co-authored-by: sezalchug <[email protected]> Co-authored-by: Aniruddh Munde <[email protected]>
…h check (#2667) Resolves #2662 1. Created the HttpUtilities with HttpClient already instantiated and basic details filled from Startup.cs 2. Using this httpClient, call the REST and GraphQL requests with relative URI 3. Created a function in DatabaseObject to access mapping dict as it is an abstract class which was not allowed to be mocked. 4. Test cases for both REST and GraphQL 5. Rest Test case: Mock the client to return a 200 response when we receive a GET request with specific URI. Validate the error message from HttpUtilities should be null. 6. GraphQL Test case: Mock the client to return a 200 response when we get a POST request with /graphql URI and content payload. Mock the Dabase Object with columns in the entity map to only return the db object when a certain entity is called for. - [ ] Integration Tests - [x] Unit Tests Requests are running as expected. Test cases added for Rest and Graphql --------- Co-authored-by: sezalchug <[email protected]> Co-authored-by: Aniruddh Munde <[email protected]>
…2673) This closes #2642 This PR enhances the OTEL instrumentation for the GraphQL APIs by adding custom traces and metrics. Metrics can be filtered for `status_code`, `api_type`, `endpoint` and `method`. - [X] Local Testing - [ ] Integration Tests - [ ] Unit Tests All of the tests were done locally to check if the log information that was provided was correct, for both scenarios in which the query gave the proper information or when an exception was raised. Another thing that was tested is that when we open GraphQL it would send a few requests called `Introspection Queries` used to ensure that GraphQL is working properly. However, we do not want the user to see these requests as part of the total count as this is done automatically, which may confuse the users.    - Clone the following repo `https://github.com/tommasodotNET/dab-workbench.git` - Run your DAB version in CLI so the files from the `out` folder are created, and make sure to stop it before running the DAB Workbench since both cannot be running at the same time. - Find the path to the `Microsoft.DataApiBuilder.exe`, which should look something like `<PATH_TO_REPO>\data-api-builder\src\out\cli\net8.0\Microsoft.DataApiBuilder.exe` - Copy the path of the `.exe` file and paste it in the file `/DABWorkbench.AppHost/Program.cs` in the variable `dabCLIPath` which is found in line 3 as follows: `var dabCLIPath = @"<PATH_TO_REPO>\data-api-builder\src\out\cli\net8.0\Microsoft.DataApiBuilder.exe";` - Now you should be able to run DAB Workbench with your version of DAB. --------- Co-authored-by: Tommaso Stocchi <[email protected]> Co-authored-by: Jerry Nixon <[email protected]> Co-authored-by: Ruben Cerna <[email protected]>
…2681) When an API owner starts up DAB, the GraphQL data types for the fields are automatically derived from the SQL column types. However, if the API owner later changes the column type without restarting DAB, the requests will fail at runtime due to failure with parsing the JSON result in the leaf node resolver. Due to the unhandled exception, this will result in an error message `One of the identified items was in an invalid format` or `"The requested operation requires an element of type 'Number', but the target element has type 'String'.` 1. Wrap the field value parsing to catch specific exception types and throw a clearer exception 2. Ensure location and path in errors are retained for DataApiBuilderException for 4xx status codes - [x] Manual test - [] Integration Tests - [] Unit Tests Step 1: Create sample table in datasource ``` -- Create the table CREATE TABLE Employees ( EmployeeID INT PRIMARY KEY, Age INT NOT NULL, Salary INT NOT NULL ); -- Insert sample data INSERT INTO Employees (EmployeeID, Age, Salary) VALUES (1, 25, 50000), (2, 30, 60000), (3, 35, 70000), (4, 40, 80000), (5, 45, 90000); ``` Step 2: Configure dab config with following entity ``` "Employees": { "source": { "object": "dbo.Employees", "type": "table" }, "permissions": [ { "role": "anonymous", "actions": [ { "action": "*" } ] } ] }, ``` Verify employees query runs fine with Salary field ``` { employees { items { EmployeeID Salary } } } ``` Step 3: Update Salary column type ``` ALTER TABLE Employees ALTER COLUMN Salary NVARCHAR(100); ``` Run employees query again, response returned ``` { "errors": [ { "message": "The value could not be parsed for configured GraphQL data type Int", "locations": [ { "line": 5, "column": 13 } ], "path": [ "employees", "items", 0, "Salary" ], "extensions": { "code": "GraphQLMapping" } }, ... ] } ```
With Hot Chocolate 15 we have invested a lot into security, performance and GraphQL protocol standards. This PR will modernize the GraphQL stack of dab. - [X] Existing Regression tests --------- Co-authored-by: Aniruddh Munde <[email protected]>
RubenCerna2079
force-pushed
the
dev/rubencerna/hot-chocolate-cherry-pick
branch
from
22137b3 to
81e51cc
Compare
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
RubenCerna2079
changed the title
Aniruddh25
approved these changes
May 30, 2025
Collaborator
Aniruddh25
left a comment
Aniruddh25
left a comment
There was a problem hiding this comment.
LGTM! Thanks for these cherry picks and resolving the conflicts.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why make this change?
This change is made to add the Hot-Chocolate PR to the 1.5 branch
What is this change?
This change cherry picks the PRs related to Hot-Chocolate upgrade in order to add it to the branch 1.5.
How was this tested?
Sample Request(s)