Skip to content

[Ports] Cherry-pick few bug fixes to 0.9#1879

Merged
Aniruddh25 merged 6 commits intorelease/0.9from
cherryPick0.9
Nov 13, 2023
Merged

[Ports] Cherry-pick few bug fixes to 0.9#1879
Aniruddh25 merged 6 commits intorelease/0.9from
cherryPick0.9

Conversation

Aniruddh25 and others added 6 commits November 13, 2023 14:42
## 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: 

![image](https://github.com/Azure/data-api-builder/assets/6414189/7995fe30-abf2-4dcf-990f-6bfb6ac595ce)


- 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:

![image](https://github.com/Azure/data-api-builder/assets/6414189/7f5a497c-5071-4387-aaa7-d13c4107704c)

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:

![image](https://github.com/Azure/data-api-builder/assets/3513779/9f0f7a59-3c07-4f9c-b002-900fcf6e8bc5)

After:

![image](https://github.com/Azure/data-api-builder/assets/3513779/b54dafca-f7aa-4bd4-a4f2-0a88321d0959)

---------

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:


![image](https://github.com/Azure/data-api-builder/assets/3513779/3d2b827d-de56-4cbb-9982-fcf8e9f339ce)

- 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]>
@Aniruddh25
Copy link
Copy Markdown
Collaborator Author

/azp run

@Aniruddh25 Aniruddh25 enabled auto-merge (squash) November 13, 2023 22:53
@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 6 pipeline(s).

Copy link
Copy Markdown
Contributor

@aaronburtle aaronburtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Aniruddh25 Aniruddh25 merged commit e560142 into release/0.9 Nov 13, 2023
@Aniruddh25 Aniruddh25 deleted the cherryPick0.9 branch November 13, 2023 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants