[Ports] Cherry-pick few bug fixes to 0.9#1879
Merged
Aniruddh25 merged 6 commits intorelease/0.9from Nov 13, 2023
Merged
Conversation
## Why make this change? - Pick up latest patch version for HotChocolate 12 for bug fixes. ## What is this change? - Upgrade the patch version number in the global properties for all HotChocolate.* packages ## How was this tested? - Verified BCP is launched for local dev and can query GraphQL. - Existing integration tests -fix some to accommodate how upgraded hotchocolate behaves
## Why make this change? - Closes #1763 ## What is this change? - [Startup.cs] Adds an explicit `StopApplication()` call (https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.hosting.ihostapplicationlifetime.stopapplication?view=dotnet-plat-ext-6.0#microsoft-extensions-hosting-ihostapplicationlifetime-stopapplication) which signals the app to stop, instead of raising an Application Exception. The exception results in duplication logging of events. - [Startup.cs] Combines the Startup/DAB engine failure logging message to explain which log file caused the shutdown. - [Program.cs] Explicitly catches `TaskCanceledException` which is raised when `StopApplication()` called. In that catch block, the console error states that the engine is unable to launch. The reasons will be emitted in previous log events/exceptions. - [Program.cs] Adds back a pre 0.9 log message to the console which includes the full exception. This captures the Kestrel issue expected in the GitHub issue raised for which this PR addresses. ## How was this tested? - [x] existing test validate proper log messages are emitted. ## Sample Request(s) - add an intentional error to the runtime config to cause config parsing to fail: set global rest path to "/ap/i" ```json "rest": { "enabled": true, "path": "/ap/i", "request-body-strict": false }, ``` Observe that failure looks like:  - Start dab in a separate console window with command: ```shell dab start -c "<config file path>" ``` and see failure that elaborates on Kestrel server port unable to bind:  Co-authored-by: Aniruddh Munde <[email protected]>
## Why make this change? - OpenAPI document is needed only when rest is enabled. For cosmosdb_nosql it is never enabled. So, any attempts to create the document will always fail. This shows up as a failure in the logs and has the potential to misguide the actual error that could cause a failure to start the engine. - E.g. see below logs from a [question on stackoverflow](https://stackoverflow.com/questions/77432995/dataapibuilderexception-while-trying-to-run-swa-start) > [dataApi] fail: Azure.DataApiBuilder.Service.Startup[0] > [dataApi] OpenAPI Documentor initialization failed. > [dataApi] Azure.DataApiBuilder.Service.Exceptions.DataApiBuilderException: OpenAPI description document can't be created when the REST endpoint is disabled globally. > [dataApi] at Azure.DataApiBuilder.Core.Services.OpenApiDocumentor.CreateDocument() in /_/src/Core/Services/OpenAPI/OpenApiDocumentor.cs:line 108 > [dataApi] at Azure.DataApiBuilder.Service.Startup.PerformOnConfigChangeAsync(IApplicationBuilder app) in /_/src/Service/Startup.cs:line 687 > [dataApi] Unable to launch the runtime due to an error. > [dataApi] Error: Failed to start the engine. > [dataApi] cd "C:\Users\alexa\source\repos\az-lead\swa-db-connections" && "C:\Users\alexa\.swa\dataApiBuilder\0.9.6-rc\Microsoft.DataApiBuilder.exe" start -c staticwebapp.database.config.json --no-https-redirect exited with code 0 - On the surface the above error seems to be due to an OpenApi document creation failure, but it shouldn't be the case. We still need more customer logs to understand the issue. This PR is to fix the misguided logs. ## What is this change? - Gaurd OpenAPI document creation call with a check for REST is enabled or not. - Change the log error to log warning - to avoid misguiding. ## How was this tested? - Started the engine locally to verify after the change the failure is gone for CosmosDb_NoSql. ## Sample Request(s) Before when the engine has successfully started for CosmosDB_NoSql:  After:  --------- Co-authored-by: Abhishek Kumar <[email protected]>
…he Windows agent (#1876) ## Why make this change? Currently, we are running integration tests against a production CosmosDB account. Recently, we've encountered 429 errors during these tests because the same production account is used for each OS agent. To address this issue, we have started using the CosmosDB Local Emulator when running integration tests on the Windows agent. ## What is this change? Changed the Windows agent to windows-latest. This agent has an emulator preinstalled. Removed the FileTransform task for the Windows agent. Temporarily removed the Mac-os job to prevent 429 errors. Todo: Use an emulator in the Mac-os agent and Linux agent. ## How was this tested? - [x] Integration Tests Co-authored-by: Neeraj Sharma (from Dev Box) <[email protected]>
## Why make this change? - Saw this exception while running locally via VS:  - it is triggered asynchronously when the grapqhql schema is being generated in the background - Also, noticed telemetry client is being added irrespective of it is enabled or not. ## What is this change? - Add AppInsights Telemetry only when it is enabled. - Use `TryAdd` instead of simply doing an `Add` to the dictionary of global properties in the Initialize function that is used to initialize any telemetry object. - Depending on if the telemetry object had already been initialized with the 2 global properties we intend to add, they may or may not be already present. Examples of such telemetry objects include `MetricsTelemetry`, `DependencyTelemetry` etc. ## How was this tested? - Reran the engine to verify the exception was gone upon starting. - Added asserts in TelemetryTests to verify the client is created only when telemetry is enabled.
## Why make this change?
Updates the form of our NextLink cursor used for pagination. This only
updates the part used for the response, which is then used to continue
the paginated requests. Matches the desired form.
## What is this change?
Rather than using part of the `SqlQueryStructures` to form the cursor,
we create an object that is only used specifically for the NextLink with
respect to the response/request. This means we don't need all of the
fields that are included in the pagination columns, and that the
information contained in this object is really unrelated to the query
structure.
We therefore introduce a new class in our pagination utilities which can
be used to facilitate the serialization and deserialization of the
NextLink cursor in the response/request. Using an anonymous object was
considered, but we re-use this object in enough places, and it holds
complex enough data, that it will be easier to understand and be more
reusable if we define a new class here inside of our pagination
utilities.
The Json structure that is contained within the response/request that we
(de)serialize with this new class is therefore modified.
Previous
```
[{"Value":0,"Direction":0,"TableSchema":"foo","TableName":"bar","ColumnName":"baz"}]")
```
Updated
```
[{"EntityName":"foobar","FieldName":"qux","Value":0,"Direction":0}]
```
Complete NextLink without being decoded
```
"nextLink": "https://localhost:5001/rest/Book?$first=1&$after=W3siVmFsdWUiOjEsIkRpcmVjdGlvbiI6MCwiVGFibGVTY2hlbWEiOiJkYm8iLCJUYWJsZU5hbWUiOiJib29rcyIsIkNvbHVtbk5hbWUiOiJpZCJ9XQ%3D%3D"
```
## How was this tested?
Modified our current test suite to match the new format.
## Sample Request(s)
and paginated request should work as long as it uses or generated a
cursor for pagination.
one such example for when REST has a path of /rest against the Book
entity.
https://localhost:5001/rest/Book?$first=1
---------
Co-authored-by: Aniruddh Munde <[email protected]>
seantleonard
approved these changes
Nov 13, 2023
Collaborator
Author
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
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
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 is simply cherry-picking the following PRs before creating the next stable 0.9 version: