fix: replace env defaults for stored procedures#3652
Open
nanookclaw wants to merge 1 commit into
Open
Conversation
nanookclaw
requested review from
Alekhya-Polavarapu,
Aniruddh25,
JerryNixon,
RubenCerna2079,
aaronburtle,
anushakolan,
rusamant,
sourabh1007,
souvikghosh04,
stuartpa and
vadeveka
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adds support for environment-variable replacement when deserializing stored procedure parameter defaults from runtime config, and adds tests to validate both config parsing and SQL execution parameter typing.
Changes:
- Apply
DeserializationVariableReplacementSettingswhen converting stored procedure parameter default values inEntitySourceConverter. - Add a deserialization unit test verifying stored procedure parameter default env-var replacement (on/off).
- Add a SQL execution unit test verifying a config-provided default date is resolved as a
DateTimeparameter.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Config/Converters/EntitySourceConverterFactory.cs | Pass replacement settings into parameter default value deserialization so @env(...) can be replaced at parse time. |
| src/Service.Tests/UnitTests/RuntimeConfigLoaderJsonDeserializerTests.cs | Adds a data-driven test covering env-var replacement behavior for stored procedure parameter defaults. |
| src/Service.Tests/UnitTests/SqlExecuteStructureUnitTests.cs | Adds a unit test verifying config default date is materialized as a DateTime SQL parameter value. |
Comment on lines
+30
to
+79
| Environment.SetEnvironmentVariable(envVarName, "2024-06-30"); | ||
| try | ||
| { | ||
| Assert.IsTrue(RuntimeConfigLoader.TryParseConfig( | ||
| GetStoredProcedureConfigWithEnvDefault(), | ||
| out RuntimeConfig runtimeConfig, | ||
| replacementSettings: new DeserializationVariableReplacementSettings( | ||
| azureKeyVaultOptions: null, | ||
| doReplaceEnvVar: true, | ||
| doReplaceAkvVar: false))); | ||
|
|
||
| StoredProcedureDefinition storedProcedureDefinition = new(); | ||
| storedProcedureDefinition.Parameters.Add( | ||
| parameterName, | ||
| new ParameterDefinition | ||
| { | ||
| SystemType = typeof(DateTime), | ||
| DbType = DbType.DateTime, | ||
| HasConfigDefault = true, | ||
| ConfigDefaultValue = runtimeConfig.Entities[entityName].Source.Parameters[0].Default | ||
| }); | ||
|
|
||
| DatabaseStoredProcedure storedProcedure = new(schemaName: "dbo", tableName: "get_records_by_date") | ||
| { | ||
| SourceType = EntitySourceType.StoredProcedure, | ||
| StoredProcedureDefinition = storedProcedureDefinition | ||
| }; | ||
|
|
||
| Mock<ISqlMetadataProvider> metadataProvider = new(); | ||
| metadataProvider.SetupGet(x => x.EntityToDatabaseObject).Returns(new Dictionary<string, DatabaseObject> | ||
| { | ||
| { entityName, storedProcedure } | ||
| }); | ||
| metadataProvider.Setup(x => x.GetStoredProcedureDefinition(entityName)).Returns(storedProcedureDefinition); | ||
| metadataProvider.Setup(x => x.GetSourceDefinition(entityName)).Returns(storedProcedureDefinition); | ||
|
|
||
| SqlExecuteStructure executeStructure = new( | ||
| entityName, | ||
| metadataProvider.Object, | ||
| Mock.Of<IAuthorizationResolver>(), | ||
| null, | ||
| new Dictionary<string, object?>()); | ||
|
|
||
| string dbConnectionParameterName = (string)executeStructure.ProcedureParameters[parameterName]; | ||
| Assert.AreEqual(expectedDate, executeStructure.Parameters[dbConnectionParameterName].Value); | ||
| } | ||
| finally | ||
| { | ||
| Environment.SetEnvironmentVariable(envVarName, null); | ||
| } |
Comment on lines
+173
to
+221
| const string envVarName = "year-end"; | ||
| Environment.SetEnvironmentVariable(envVarName, "2024-06-30"); | ||
|
|
||
| try | ||
| { | ||
| string configJson = @" | ||
| { | ||
| ""data-source"": { | ||
| ""database-type"": ""mssql"", | ||
| ""connection-string"": ""Server=tcp:127.0.0.1,1433;Persist Security Info=False;Trusted_Connection=True;TrustServerCertificate=True;MultipleActiveResultSets=False;Connection Timeout=5;"" | ||
| }, | ||
| ""entities"": { | ||
| ""GetRecordsByDate"": { | ||
| ""source"": { | ||
| ""object"": ""get_records_by_date"", | ||
| ""type"": ""stored-procedure"", | ||
| ""parameters"": { | ||
| ""YearEndDate"": ""@env('year-end')"" | ||
| } | ||
| }, | ||
| ""permissions"": [ | ||
| { | ||
| ""role"": ""anonymous"", | ||
| ""actions"": [ | ||
| { | ||
| ""action"": ""execute"" | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| }"; | ||
|
|
||
| bool isParsingSuccessful = RuntimeConfigLoader.TryParseConfig( | ||
| configJson, | ||
| out RuntimeConfig runtimeConfig, | ||
| replacementSettings: new DeserializationVariableReplacementSettings( | ||
| azureKeyVaultOptions: null, | ||
| doReplaceEnvVar: replaceEnvVar, | ||
| doReplaceAkvVar: false)); | ||
|
|
||
| Assert.IsTrue(isParsingSuccessful); | ||
| Assert.AreEqual(expectedDefaultValue, runtimeConfig.Entities["GetRecordsByDate"].Source.Parameters[0].Default); | ||
| } | ||
| finally | ||
| { | ||
| Environment.SetEnvironmentVariable(envVarName, null); | ||
| } |
Comment on lines
+127
to
+132
| private static string? DeserializeStringValue(JsonElement element, DeserializationVariableReplacementSettings? replacementSettings) | ||
| { | ||
| Utf8JsonReader reader = new(JsonSerializer.SerializeToUtf8Bytes(element.GetString())); | ||
| reader.Read(); | ||
| return reader.DeserializeString(replacementSettings); | ||
| } |
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.
Closes #2067.
Old-format stored procedure
parametersare normalized inEntitySourceConverterFactorybefore the normal entity source deserialization path. That path copied string defaults withJsonElement.GetString(), so a config default like@env('year-end')was stored literally even when environment-variable replacement was enabled. Later SQL execute type coercion tried to convert the literal placeholder toDateTimeand failed.This updates the old-format parameter reader to deserialize string defaults through the existing string converter with replacement settings, leaving number, boolean, and null handling unchanged. That gives stored procedure config defaults the same environment-variable replacement behavior as other string config fields before
SqlExecuteStructurecoerces the value to the database parameter type.Tests cover replacement-enabled and replacement-disabled config parsing for
@env('year-end'), plus a focused execute-structure regression that verifies the substituted default becomes aDateTimeparameter value.Verification:
git diff --checkdotnet test src/Service.Tests/Azure.DataApiBuilder.Service.Tests.csproj --no-restore --filter "FullyQualifiedName~RuntimeConfigLoaderJsonDeserializerTests|FullyQualifiedName~SqlExecuteStructureUnitTests"could not run locally becausedotnetis not installed in this containerdotnet build src/Config/Azure.DataApiBuilder.Config.csproj --no-restorecould not run locally for the same reason