Skip to content

Commit

Permalink
Do not emit null when serializing a nullable union type, but accept i…
Browse files Browse the repository at this point in the history
…t if encountered in the allowedValues constraint (#11546)

Resolves #11544
###### Microsoft Reviewers: [Open in
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/11546)
  • Loading branch information
jeskew authored Aug 17, 2023
1 parent c653d20 commit 5eb49f6
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 18 deletions.
59 changes: 58 additions & 1 deletion src/Bicep.Core.IntegrationTests/UserDefinedTypeTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -727,7 +727,7 @@ public void Duplicate_property_names_should_raise_descriptive_diagnostic()
}

[TestMethod]
public void Union_types_with_single_normalized_member_raise_diagnostics()
public void Union_types_with_single_normalized_member_compile_without_error()
{
var result = CompilationHelper.Compile("""
type union = 'a' | 'a'
Expand All @@ -743,4 +743,61 @@ public void Union_types_with_single_normalized_member_raise_diagnostics()
}
"""));
}

[TestMethod]
public void Nullable_union_types_do_not_include_null_in_allowed_values_constraint()
{
var result = CompilationHelper.Compile("""
type union = 'a' | 'b' | 'c' | null
type unionWithOneMember = null | 'a'
""");

result.Should().NotHaveAnyDiagnostics();

result.Template.Should().NotBeNull();
result.Template!.Should().HaveValueAtPath("definitions.union", JToken.Parse("""
{
"type": "string",
"allowedValues": ["a", "b", "c"],
"nullable": true
}
"""));
result.Template!.Should().HaveValueAtPath("definitions.unionWithOneMember", JToken.Parse("""
{
"type": "string",
"allowedValues": ["a"],
"nullable": true
}
"""));
}

[TestMethod]
public void Param_with_null_in_allowedValues_constraint_can_be_loaded()
{
var result = CompilationHelper.Compile(
("main.bicep", """
module mod 'mod.json' = {
name: 'mod'
params: {
foo: 'foo'
}
}
"""),
("mod.json", """
{
"$schema": "https://schema.management.azure.com/schemas/2019-04-01/deploymentTemplate.json#",
"languageVersion": "2.0",
"contentVersion": "1.0.0.0",
"parameters": {
"foo": {
"type": "string",
"allowedValues": ["foo", "bar", "baz", null]
}
},
"resources": []
}
"""));

result.Should().NotHaveAnyDiagnostics();
}
}
22 changes: 8 additions & 14 deletions src/Bicep.Core/Emit/TemplateWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ private ObjectExpression GetTypePropertiesForArrayType(ArrayTypeExpression expre

if (TryGetAllowedValues(expression.BaseExpression) is {} allowedValues)
{
properties.Add(ExpressionFactory.CreateObjectProperty("allowedValues", allowedValues, expression.BaseExpression.SourceSyntax));
properties.Add(AllowedValuesProperty(allowedValues, expression.BaseExpression.SourceSyntax));
}
else
{
Expand Down Expand Up @@ -500,25 +500,19 @@ private ObjectExpression GetTypePropertiesForTupleType(TupleTypeExpression expre
ExpressionFactory.CreateObjectProperty("items", ExpressionFactory.CreateBooleanLiteral(false), expression.SourceSyntax),
});

private static (bool Nullable, string NonLiteralTypeName) GetUnionTypeNullabilityAndType(UnionType unionType) =>
TypeHelper.TryRemoveNullability(unionType) switch
{
UnionType nonNullableUnion => (true, GetNonLiteralTypeName(nonNullableUnion.Members.First().Type)),
TypeSymbol nonNullable => (true, GetNonLiteralTypeName(nonNullable)),
_ => (false, GetNonLiteralTypeName(unionType.Members.First().Type)),
};

private ObjectExpression GetTypePropertiesForUnionTypeExpression(UnionTypeExpression expression)
{
var (nullable, nonLiteralTypeName) = GetUnionTypeNullabilityAndType(expression.ExpressedUnionType);
var (nullable, nonLiteralTypeName, allowedValues) = TypeHelper.TryRemoveNullability(expression.ExpressedUnionType) switch
{
UnionType nonNullableUnion => (true, GetNonLiteralTypeName(nonNullableUnion.Members.First().Type), GetAllowedValuesForUnionType(nonNullableUnion, expression.SourceSyntax)),
TypeSymbol nonNullable => (true, GetNonLiteralTypeName(nonNullable), SingleElementArray(ToLiteralValue(nonNullable))),
_ => (false, GetNonLiteralTypeName(expression.ExpressedUnionType.Members.First().Type), GetAllowedValuesForUnionType(expression.ExpressedUnionType, expression.SourceSyntax)),
};

var properties = new List<ObjectPropertyExpression>
{
TypeProperty(nonLiteralTypeName, expression.SourceSyntax),
ExpressionFactory.CreateObjectProperty(
"allowedValues",
GetAllowedValuesForUnionType(expression.ExpressedUnionType, expression.SourceSyntax),
expression.SourceSyntax),
AllowedValuesProperty(allowedValues, expression.SourceSyntax),
};

if (nullable)
Expand Down
12 changes: 9 additions & 3 deletions src/Bicep.Core/TypeSystem/ArmTemplateTypeLoader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,13 @@ private static TypeSymbol TryGetLiteralUnionType(JArray allowedValues, Func<JTok
List<TypeSymbol> literalTypeTargets = new();
foreach (var element in allowedValues)
{
if (element.Type == JTokenType.Null)
{
// The compiler was erroneously including `null` in the allowedValues array if the overall type was nullable (e.g., `type foo = ('bar' | 'baz')?`)
// This error has been corrected, but some of those compiled templates may still be out in the wild, so just skip over any `null`s encountered
continue;
}

if (!validator(element) || TypeHelper.TryCreateTypeLiteral(element) is not { } literal)
{
return ErrorType.Create(diagnosticOnMismatch(DiagnosticBuilder.ForDocumentStart()));
Expand Down Expand Up @@ -121,7 +128,7 @@ private static TypeSymbol GetArrayType(SchemaValidationContext context, ITemplat

if (schemaNode.Items?.SchemaNode is { } items)
{
if (items.Ref?.Value is { } @ref)
if (items.Ref?.Value is not null)
{
var (type, typeName) = GetDeferrableTypeInfo(context, items);
return new TypedArrayType($"{typeName}[]", type, default, schemaNode.MinLength?.Value, schemaNode.MaxLength?.Value);
Expand Down Expand Up @@ -198,8 +205,7 @@ private static TypeSymbol GetObjectType(SchemaValidationContext context, ITempla
// depending on the language version, either only properties included in schemaNode.Required are required,
// or all of them are (but some may be nullable)
var required = context.TemplateLanguageVersion?.HasFeature(TemplateLanguageFeature.NullableParameters) == true
? true
: schemaNode.Required?.Value.Contains(propertyName) ?? false;
|| (schemaNode.Required?.Value.Contains(propertyName) ?? false);
var propertyFlags = required ? TypePropertyFlags.Required : TypePropertyFlags.None;
var description = schema.Metadata?.Value is JObject metadataObject &&
metadataObject.TryGetValue(LanguageConstants.MetadataDescriptionPropertyName, out var descriptionToken) &&
Expand Down

0 comments on commit 5eb49f6

Please sign in to comment.