Fix command timeout issues in DAB #2912
Conversation
vs-dsva
requested review from
Alekhya-Polavarapu,
Aniruddh25,
RubenCerna2079,
aaronburtle,
anushakolan,
neeraj-sharma2592,
rusamant,
sourabh1007,
souvikghosh04 and
vadeveka
as code owners
|
@vs-dsva please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for configuring command timeout for PostgreSQL operations in Data API Builder (DAB) to address timeout issues for long-running queries that exceed the default 30-second limit.
- Introduces a new
command-timeoutoption in data source configuration that overrides connection string timeout values - Implements PostgreSQL-specific options handling and connection string modification
- Adds comprehensive test coverage and documentation for the new feature
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Config/ObjectModel/DataSource.cs | Adds PostgreSQL options support and command timeout parsing logic |
| src/Config/RuntimeConfigLoader.cs | Updates PostgreSQL connection string processing to apply timeout configuration |
| src/Service.Tests/Configuration/ConfigurationTests.cs | Adds unit tests for command timeout functionality across database types |
| schemas/dab.draft.schema.json | Updates JSON schema to include command-timeout option for PostgreSQL and MSSQL |
| samples/postgresql-command-timeout-example.json | Provides example configuration with command timeout |
| samples/connection-string-timeout-example.json | Shows alternative connection string timeout approach |
| docs/postgresql-command-timeout.md | Documents the new command timeout feature |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| private int? ReadIntOption(string option) | ||
| { | ||
| if (Options is not null && Options.TryGetValue(option, out object? value)) | ||
| { | ||
| if (value is int intValue) | ||
| { | ||
| return intValue; | ||
| } | ||
| else if (value is string stringValue && int.TryParse(stringValue, out int parsedValue)) | ||
| { | ||
| return parsedValue; | ||
| } | ||
| else if (value is JsonElement jsonElement && jsonElement.ValueKind == JsonValueKind.Number && jsonElement.TryGetInt32(out int jsonValue)) | ||
| { | ||
| return jsonValue; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
The ReadIntOption method duplicates parsing logic found in GetCommandTimeout. Consider consolidating this logic to avoid code duplication and ensure consistent parsing behavior.
| public int GetCommandTimeout() | ||
| { | ||
| if (Options is not null && Options.TryGetValue("command-timeout", out object? value)) | ||
| { | ||
| if (value is int intValue) | ||
| { | ||
| return intValue; | ||
| } | ||
| else if (value is long longValue && longValue <= int.MaxValue && longValue >= int.MinValue) | ||
| { | ||
| return (int)longValue; | ||
| } | ||
| else if (value is string stringValue && int.TryParse(stringValue, out int parsedValue)) | ||
| { | ||
| return parsedValue; | ||
| } | ||
| } | ||
|
|
||
| return 30; // default command timeout | ||
| } |
There was a problem hiding this comment.
The magic number 30 for default command timeout should be extracted as a named constant to improve maintainability and avoid duplication with the schema default value.
| "properties": { | ||
| "command-timeout": { | ||
| "type": "integer", | ||
| "description": "Command timeout in seconds for database operations", | ||
| "minimum": 0, | ||
| "default": 30 | ||
| } | ||
| }, |
There was a problem hiding this comment.
The PostgreSQL schema section adds command-timeout support, but MySQL and other database types are not updated. Consider adding command-timeout support to all database types or documenting why it's only available for specific databases.
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
@souvikghosh04 didn't we already do this? |
@JerryNixon no we didn't. It was only discussed but not taken in implementation. |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,80 @@ | |||
| # PostgreSQL Command Timeout Configuration | |||
There was a problem hiding this comment.
The documentation title and content are misleading. This feature is not PostgreSQL-specific - the schema allows command-timeout for MSSQL, PostgreSQL, MySQL, and cosmosdb_postgresql (lines 119-150 in dab.draft.schema.json). The documentation should be generalized to cover all supported database types, not just PostgreSQL. Consider renaming this file to "command-timeout.md" or "database-command-timeout.md".
| ## Overview | ||
|
|
||
| Data API Builder now supports configuring PostgreSQL command timeout through the `command-timeout` option in the data source configuration. This feature allows you to override the default command timeout for all PostgreSQL queries executed by Data API Builder. |
There was a problem hiding this comment.
The documentation states "This feature allows you to override the default command timeout for all PostgreSQL queries" but the feature is supposed to work for MSSQL, MySQL, and other database types as well based on the schema. The overview should clarify that this feature supports multiple database types, not just PostgreSQL.
| "command-timeout": { | ||
| "type": "integer", | ||
| "description": "Command timeout in seconds for database operations", | ||
| "minimum": 0, | ||
| "default": 30 |
There was a problem hiding this comment.
The configuration property is named "command-timeout" but the documentation and code don't clearly indicate the unit. While it's stated in comments that it's in seconds, the original issue #2766 suggested naming it "command-timeout-seconds" to make the unit explicit. Consider either renaming to "command-timeout-seconds" or ensuring the documentation and schema description clearly specify the unit is seconds to avoid confusion.
| "command-timeout": { | ||
| "type": "integer", | ||
| "description": "Command timeout in seconds for database operations", | ||
| "minimum": 0, | ||
| "default": 30 |
There was a problem hiding this comment.
The schema defines command-timeout with a "default": 30, but this doesn't actually set a default value in JSON schema - it's just documentation. The actual default is implemented in code (DataSource.GetCommandTimeout returns 30). Consider removing the "default" property from the schema or clarifying that it's documentation-only to avoid confusion about how defaults work in JSON schema.
| private int? ReadIntOption(string option) | ||
| { | ||
| if (Options is not null && Options.TryGetValue(option, out object? value)) | ||
| { | ||
| if (value is int intValue) | ||
| { | ||
| return intValue; | ||
| } | ||
| else if (value is string stringValue && int.TryParse(stringValue, out int parsedValue)) | ||
| { | ||
| return parsedValue; | ||
| } | ||
| else if (value is JsonElement jsonElement && jsonElement.ValueKind == JsonValueKind.Number && jsonElement.TryGetInt32(out int jsonValue)) | ||
| { | ||
| return jsonValue; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
ReadIntOption doesn't handle the 'long' type case that GetCommandTimeout handles (lines 135-138). When JSON numbers are deserialized, they might come through as long values. For consistency and to prevent potential issues, ReadIntOption should also handle the long type conversion case similar to GetCommandTimeout.
| ## Related | ||
|
|
||
| - See `samples/postgresql-command-timeout-example.json` for a complete working example | ||
| - For other database types, command timeout can be configured directly in the connection string No newline at end of file |
There was a problem hiding this comment.
The statement "For other database types, command timeout can be configured directly in the connection string" is misleading. According to the schema (dab.draft.schema.json lines 119-150), MSSQL, MySQL, PostgreSQL, and cosmosdb_postgresql all support the command-timeout option. The documentation should clarify which databases support the option in the data-source.options configuration versus connection string only.
| - For other database types, command timeout can be configured directly in the connection string | |
| - For other supported database types (such as MSSQL, MySQL, and cosmosdb_postgresql), `command-timeout` can also be configured via the `data-source.options.command-timeout` setting, or via the connection string parameters supported by each provider. |
| /// else add the Application Name property with DataApiBuilder Application Name based on hosted/oss platform. | ||
| /// </summary> | ||
| /// <param name="connectionString">Connection string for connecting to database.</param> | ||
| /// <returns>Updated connection string with `Application Name` property.</returns> | ||
| internal static string GetPgSqlConnectionStringWithApplicationName(string connectionString) | ||
| /// <param name="dataSource">The data source configuration, used to get command timeout override.</param> | ||
| /// <returns>Updated connection string with `Application Name` property and command timeout.</returns> | ||
| internal static string GetPgSqlConnectionStringWithApplicationName(string connectionString, DataSource? dataSource = null) |
There was a problem hiding this comment.
The GetConnectionStringWithApplicationName method signature should be updated to accept a DataSource parameter (similar to GetPgSqlConnectionStringWithApplicationName) to enable applying command timeout from MsSqlOptions to the connection string. Without this parameter, MSSQL command timeout configuration cannot be properly applied.
| if (Options is not null && Options.TryGetValue("command-timeout", out object? value)) | ||
| { | ||
| if (value is int intValue) | ||
| { | ||
| return intValue; | ||
| } | ||
| else if (value is long longValue && longValue <= int.MaxValue && longValue >= int.MinValue) | ||
| { | ||
| return (int)longValue; | ||
| } | ||
| else if (value is string stringValue && int.TryParse(stringValue, out int parsedValue)) | ||
| { | ||
| return parsedValue; | ||
| } | ||
| } | ||
|
|
||
| return 30; // default command timeout |
There was a problem hiding this comment.
The GetCommandTimeout method duplicates parsing logic that already exists in ReadIntOption. Consider refactoring to use ReadIntOption internally and provide the default value if it returns null: return ReadIntOption("command-timeout") ?? 30;. This reduces code duplication and ensures consistent parsing behavior across the codebase.
| if (Options is not null && Options.TryGetValue("command-timeout", out object? value)) | |
| { | |
| if (value is int intValue) | |
| { | |
| return intValue; | |
| } | |
| else if (value is long longValue && longValue <= int.MaxValue && longValue >= int.MinValue) | |
| { | |
| return (int)longValue; | |
| } | |
| else if (value is string stringValue && int.TryParse(stringValue, out int parsedValue)) | |
| { | |
| return parsedValue; | |
| } | |
| } | |
| return 30; // default command timeout | |
| return ReadIntOption("command-timeout") ?? 30; |
| public void TestCommandTimeoutFromDataSourceOptions() | ||
| { | ||
| // Test SQL Server | ||
| Dictionary<string, object> mssqlOptions = new() | ||
| { | ||
| { "command-timeout", 60 }, | ||
| { "set-session-context", true } | ||
| }; | ||
| DataSource mssqlDataSource = new(DatabaseType.MSSQL, "Server=localhost;Database=test;", mssqlOptions); | ||
| Assert.AreEqual(60, mssqlDataSource.GetCommandTimeout()); | ||
|
|
||
| // Test PostgreSQL | ||
| Dictionary<string, object> pgOptions = new() | ||
| { | ||
| { "command-timeout", 120 } | ||
| }; | ||
| DataSource pgDataSource = new(DatabaseType.PostgreSQL, "Host=localhost;Database=test;", pgOptions); | ||
| Assert.AreEqual(120, pgDataSource.GetCommandTimeout()); | ||
|
|
||
| // Test MySQL | ||
| Dictionary<string, object> mysqlOptions = new() | ||
| { | ||
| { "command-timeout", 45 } | ||
| }; | ||
| DataSource mysqlDataSource = new(DatabaseType.MySQL, "Server=localhost;Database=test;", mysqlOptions); | ||
| Assert.AreEqual(45, mysqlDataSource.GetCommandTimeout()); | ||
|
|
||
| // Test default value when not specified | ||
| DataSource defaultDataSource = new(DatabaseType.MSSQL, "Server=localhost;Database=test;", new()); | ||
| Assert.AreEqual(30, defaultDataSource.GetCommandTimeout()); | ||
|
|
||
| // Test null options | ||
| DataSource nullOptionsDataSource = new(DatabaseType.MSSQL, "Server=localhost;Database=test;", null); | ||
| Assert.AreEqual(30, nullOptionsDataSource.GetCommandTimeout()); | ||
| } |
There was a problem hiding this comment.
The test only verifies that GetCommandTimeout() returns the correct value from configuration, but doesn't test that the timeout is actually applied to database commands or connection strings. Consider adding integration tests that verify the command timeout is properly set on DbCommand objects or in the processed connection strings for each database type (MSSQL, PostgreSQL, MySQL).
| /// <summary> | ||
| /// Options for MsSql database. | ||
| /// </summary> | ||
| public record MsSqlOptions(bool SetSessionContext = true) : IDataSourceOptions; |
There was a problem hiding this comment.
The MsSqlOptions record should include a CommandTimeout property similar to PostgreSqlOptions. Currently, the schema allows command-timeout for MSSQL (line 119-124 in dab.draft.schema.json), but there's no corresponding property in MsSqlOptions to parse and store it. This needs to be added: public record MsSqlOptions(bool SetSessionContext = true, int? CommandTimeout = null) : IDataSourceOptions;
| public record MsSqlOptions(bool SetSessionContext = true) : IDataSourceOptions; | |
| public record MsSqlOptions(bool SetSessionContext = true, int? CommandTimeout = null) : IDataSourceOptions; |
6 participants
Why make this change?
Offers a partial fix for #2766
This is important for queries that run longer than 30 sec (the default value) as in REST mode there are frequent 5XX type errors (waiting for headers) that resort back to this.
What is this change?
The
command timeoutparameter is not consistently passed downstream to database access libraries.Hence a new parameter in
data-source.options.command-timeoutto control it.How was this tested?
Sample Request(s)
The configuration accepts
{ "$schema": "../schemas/dab.draft.schema.json", "data-source": { "database-type": "postgresql", "connection-string": "Host=localhost;Database=bookstore;Username=postgres;Password=password;", "options": { "command-timeout": 60 } }, ...