Skip to content

Commit

Permalink
Preserve distinction between {} and object in compiled JSON (#13597)
Browse files Browse the repository at this point in the history
Resolves #13556 

Given an object type declaration with no catchall schema, the type
checker will issue warnings if a property whose name does not match any
property declarations is supplied.

```bicep
param example { foo: string } = { bar: 'string' }         // <-- Issues a BCP037 warning because there is no known `bar` property

@Sealed()
param sealedExample { foo: string } = { bar: 'string' }   // <-- Issues a BCP037 error because there is no known `bar` property and the object is sealed
```

This means that given an object type declaration of `{}`, all supplied
properties will trigger a BCP037 warning because an empty object was
expected. This behaves differently from the untyped object `object`,
which matches without warning against any object. The distinction
between `{}` and `object` was getting dropped when templates were
compiled to JSON, however, because the compiler wasn't including any
information in the type definition that would allow the two to be
distinguished; instead, they would both be compiled to `{"type":
"object"}`.

This PR updates the compiler to include an empty `properties` constraint
on object type declarations that include no properties so that it can
tell the difference between `{}` and `object` when loading or importing
from JSON modules.
###### Microsoft Reviewers: [Open in
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/13597)
  • Loading branch information
jeskew committed Mar 13, 2024
1 parent 0c0394b commit 148466a
Show file tree
Hide file tree
Showing 8 changed files with 107 additions and 26 deletions.
35 changes: 35 additions & 0 deletions src/Bicep.Core.IntegrationTests/ScenarioTests.cs
Expand Up @@ -5898,4 +5898,39 @@ public void Test_Issue13534()
("BCP036", DiagnosticLevel.Warning, """The property "gateway" expected a value of type "string" but the provided value is of type "object"."""),
});
}

// https://github.com/Azure/bicep/issues/13556
[TestMethod]
public void Distinction_between_empty_and_untyped_objects_should_survive_compilation_to_JSON()
{
var originalTypesTemplate = """
@export()
type emptyObject = {}

@export()
type untypedObject = object
""";

var compiledTypesTemplate = CompilationHelper.Compile(originalTypesTemplate).Template;
compiledTypesTemplate.Should().NotBeNull();

var result = CompilationHelper.Compile(
("types.bicep", originalTypesTemplate),
("types.json", compiledTypesTemplate!.ToString()),
("main.bicep", """
import * as fromBicep from 'types.bicep'
import * as fromJson from 'types.json'

param a fromBicep.emptyObject = {fizz: 'buzz'}
param b fromBicep.untypedObject = {foo: 'bar'}
param c fromJson.emptyObject = {snap: 'crackle'}
param d fromJson.untypedObject = {wishy: 'washy'}
"""));

result.ExcludingLinterDiagnostics().Should().HaveDiagnostics(new[]
{
("BCP037", DiagnosticLevel.Warning, """The property "fizz" is not allowed on objects of type "{ }". No other properties are allowed."""),
("BCP037", DiagnosticLevel.Warning, """The property "snap" is not allowed on objects of type "{ }". No other properties are allowed."""),
});
}
}
12 changes: 12 additions & 0 deletions src/Bicep.Core.IntegrationTests/UserDefinedTypeTests.cs
Expand Up @@ -450,6 +450,18 @@ public void Warning_should_be_shown_when_setting_unknown_properties_on_unsealed_
});
}

[TestMethod]
public void Empty_object_should_be_distinguishable_from_untyped_object_in_compiled_JSON()
{
var result = CompilationHelper.Compile("""
type emptyObject = {}
type untypedObect = object
""");

result.Template.Should().HaveValueAtPath("definitions.emptyObject.properties", new JObject());
result.Template.Should().NotHaveValueAtPath("definitions.untypedObject.properties");
}

[TestMethod]
public void Error_should_be_shown_when_setting_unknown_properties_that_do_not_match_additional_properties_type()
{
Expand Down
Expand Up @@ -6,7 +6,7 @@
"_generator": {
"name": "bicep",
"version": "dev",
"templateHash": "7210691896706139249"
"templateHash": "8162009081861809153"
}
},
"definitions": {
Expand Down Expand Up @@ -194,6 +194,7 @@
},
"stringStringDictionary": {
"type": "object",
"properties": {},
"additionalProperties": {
"type": "string"
}
Expand Down Expand Up @@ -665,6 +666,7 @@
},
"discriminatedUnionInlineAdditionalProps1": {
"type": "object",
"properties": {},
"additionalProperties": {
"type": "object",
"discriminator": {
Expand All @@ -682,6 +684,7 @@
},
"discriminatedUnionInlineAdditionalProps2": {
"type": "object",
"properties": {},
"additionalProperties": {
"type": "object",
"discriminator": {
Expand Down
Expand Up @@ -305,6 +305,7 @@ type tupleSecondItem = tuple[1]
type stringStringDictionary = {
//@ "stringStringDictionary": {
//@ "type": "object",
//@ "properties": {},
//@ },
*: string
//@ "additionalProperties": {
Expand Down Expand Up @@ -903,6 +904,7 @@ type discriminatorUnionAsPropertyType = {
type discriminatedUnionInlineAdditionalProps1 = {
//@ "discriminatedUnionInlineAdditionalProps1": {
//@ "type": "object",
//@ "properties": {},
//@ },
@discriminator('type')
*: typeA | typeB
Expand All @@ -925,6 +927,7 @@ type discriminatedUnionInlineAdditionalProps1 = {
type discriminatedUnionInlineAdditionalProps2 = {
//@ "discriminatedUnionInlineAdditionalProps2": {
//@ "type": "object",
//@ "properties": {},
//@ },
@discriminator('type')
*: (typeA | typeB)?
Expand Down
Expand Up @@ -6,7 +6,7 @@
"_generator": {
"name": "bicep",
"version": "dev",
"templateHash": "7210691896706139249"
"templateHash": "8162009081861809153"
}
},
"definitions": {
Expand Down Expand Up @@ -194,6 +194,7 @@
},
"stringStringDictionary": {
"type": "object",
"properties": {},
"additionalProperties": {
"type": "string"
}
Expand Down Expand Up @@ -665,6 +666,7 @@
},
"discriminatedUnionInlineAdditionalProps1": {
"type": "object",
"properties": {},
"additionalProperties": {
"type": "object",
"discriminator": {
Expand All @@ -682,6 +684,7 @@
},
"discriminatedUnionInlineAdditionalProps2": {
"type": "object",
"properties": {},
"additionalProperties": {
"type": "object",
"discriminator": {
Expand Down
Expand Up @@ -514,6 +514,30 @@ public void Resource_derivation_metadata_causes_tagged_union_variant_to_be_loade
fallbackType.Properties["name"].TypeReference.Type.Should().Be(TypeFactory.CreateStringLiteralType("default"));
}

[TestMethod]
public void Empty_properties_constraint_causes_loaded_object_to_use_FallbackProperty_flag_on_additional_properties()
{
var parameterType = GetLoadedParameterType("""
{
"$schema": "https://schema.management.azure.com/schemas/2019-04-01/deploymentTemplate.json#",
"contentVersion": "1.0.0.0",
"languageVersion": "2.0",
"resources": {},
"parameters": {
"emptyObject": {
"type": "object",
"properties": {}
}
}
}
""",
"emptyObject");

var objectParameterType = parameterType.Should().BeAssignableTo<ObjectType>().Subject;
objectParameterType.AdditionalPropertiesType.Should().Be(LanguageConstants.Any);
objectParameterType.AdditionalPropertiesFlags.Should().HaveFlag(TypePropertyFlags.FallbackProperty);
}

private static TypeSymbol GetLoadedParameterType(string jsonTemplate, string parameterName)
{
var model = LoadModel(jsonTemplate);
Expand Down
5 changes: 1 addition & 4 deletions src/Bicep.Core/Emit/TemplateWriter.cs
Expand Up @@ -745,10 +745,7 @@ private ObjectExpression GetTypePropertiesForObjectType(ObjectTypeExpression exp
propertySchemata.Add(ExpressionFactory.CreateObjectProperty(property.PropertyName, propertySchema, property.SourceSyntax));
}

if (propertySchemata.Any())
{
properties.Add(ExpressionFactory.CreateObjectProperty("properties", ExpressionFactory.CreateObject(propertySchemata, expression.SourceSyntax)));
}
properties.Add(ExpressionFactory.CreateObjectProperty("properties", ExpressionFactory.CreateObject(propertySchemata, expression.SourceSyntax)));

if (expression.AdditionalPropertiesExpression is { } addlPropsType)
{
Expand Down
44 changes: 24 additions & 20 deletions src/Bicep.Core/TypeSystem/ArmTemplateTypeLoader.cs
Expand Up @@ -243,38 +243,42 @@ private static TypeSymbol GetObjectType(SchemaValidationContext context, ITempla
return new DiscriminatedObjectType(string.Join(" | ", TypeHelper.GetOrderedTypeNames(variants)), flags, discriminator.PropertyName.Value, variants);
}

return GetObjectType(context, schemaNode.Properties.CoalesceEnumerable(), schemaNode.Required?.Value, schemaNode.AdditionalProperties, flags);
return GetObjectType(context, schemaNode.Properties, schemaNode.Required?.Value, schemaNode.AdditionalProperties, flags);
}

private static TypeSymbol GetObjectType(SchemaValidationContext context,
IEnumerable<KeyValuePair<string, TemplateTypeDefinition>> properties,
IEnumerable<KeyValuePair<string, TemplateTypeDefinition>>? properties,
IEnumerable<string>? requiredProperties,
TemplateBooleanOrSchemaNode? additionalProperties,
TypeSymbolValidationFlags flags)
{
var requiredProps = requiredProperties is not null ? ImmutableHashSet.CreateRange(requiredProperties) : null;

ObjectTypeNameBuilder nameBuilder = new();
List<TypeProperty> propertyList = new();
List<TypeProperty>? propertyList = null;
ITypeReference? additionalPropertiesType = LanguageConstants.Any;
TypePropertyFlags additionalPropertiesFlags = TypePropertyFlags.FallbackProperty;

foreach (var (propertyName, schema) in properties)
if (properties is not null)
{
// 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
|| (requiredProps?.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) &&
descriptionToken is JValue { Value: string descriptionString }
? descriptionString
: null;

var (type, typeName) = GetDeferrableTypeInfo(context, schema);
propertyList.Add(new(propertyName, type, propertyFlags, description));
nameBuilder.AppendProperty(propertyName, typeName);
propertyList = new();
foreach (var (propertyName, schema) in properties)
{
// 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
|| (requiredProps?.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) &&
descriptionToken is JValue { Value: string descriptionString }
? descriptionString
: null;

var (type, typeName) = GetDeferrableTypeInfo(context, schema);
propertyList.Add(new(propertyName, type, propertyFlags, description));
nameBuilder.AppendProperty(propertyName, typeName);
}
}

if (additionalProperties is not null)
Expand All @@ -293,12 +297,12 @@ private static TypeSymbol GetObjectType(SchemaValidationContext context, ITempla
}
}

if (propertyList.Count == 0 && additionalProperties is null)
if (propertyList is null && additionalProperties is null)
{
return flags.HasFlag(TypeSymbolValidationFlags.IsSecure) ? LanguageConstants.SecureObject : LanguageConstants.Object;
}

return new ObjectType(nameBuilder.ToString(), flags, propertyList, additionalPropertiesType, additionalPropertiesFlags);
return new ObjectType(nameBuilder.ToString(), flags, propertyList.CoalesceEnumerable(), additionalPropertiesType, additionalPropertiesFlags);
}

private class SansMetadata : ITemplateSchemaNode
Expand Down

0 comments on commit 148466a

Please sign in to comment.