Skip to content

fix for bicep issue #15458 #16237

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

fix for bicep issue #15458 #16237

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5dd7d19
fix for bicep issue #15458
Jan 28, 2025
a25423b
merge main and make sure that the integrated test covering the fixed …
ouldsid Jan 29, 2025
df3a1b3
fixing test to cover the fixed scenario
ouldsid Jan 30, 2025
eb0673b
fixed an exception in case of no text in the returned synthaxBase value
ouldsid Jan 30, 2025
1f6df0a
comments
ouldsid Feb 5, 2025
06a4e9e
Fix a merge issue
ouldsid Feb 5, 2025
4bc280e
fixing an issue introduced in my last iteration which introduced a fi…
ouldsid Feb 5, 2025
dea5122
moving the fix introduced in last iteration from ExpressionBuilder an…
ouldsid Feb 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 42 additions & 42 deletions src/Bicep.Core/Emit/PlaceholderParametersBicepParamWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public void Write(TextWriter writer, string existingContent)

var result = filteredParameterDeclarations
.OfType<ParameterDeclarationSyntax>()
.Select(e => new ParameterAssignmentSyntax(e.Keyword, e.Name, SyntaxFactory.AssignmentToken, this.GetValueForParameter(e)))
.Select(e => new ParameterAssignmentSyntax(e.Keyword, e.Name, SyntaxFactory.AssignmentToken, GetValueForParameter(e)))
.SelectMany(e => new List<SyntaxBase>() { e, SyntaxFactory.NewlineToken });

var processedSyntaxList = new List<SyntaxBase>()
Expand All @@ -47,7 +47,47 @@ public void Write(TextWriter writer, string existingContent)
writer.WriteLine(output);
}

private SyntaxBase GetValueForParameter(ParameterDeclarationSyntax syntax)
private static SyntaxBase CheckFunctionCallsInObjectSyntax(ObjectSyntax objectSyntax)
{
var value = objectSyntax.Properties.Select(e =>
{
if (e.Value is FunctionCallSyntax valueAsFunctionCallSyntax)
{
return SyntaxFactory.CreateObjectProperty((e.Key as IdentifierSyntax)?.IdentifierName ?? "", CreateCommentSyntaxForFunctionCallSyntax(valueAsFunctionCallSyntax.Name.IdentifierName));
}
else if (e.Value is ArraySyntax valueAsFunctionArraySyntax)
{
var value = valueAsFunctionArraySyntax.Items.Select(f =>
{
if (f.Value is FunctionCallSyntax valueAsFunctionCallSyntax)
{
return SyntaxFactory.CreateArrayItem(CreateCommentSyntaxForFunctionCallSyntax(valueAsFunctionCallSyntax.Name.IdentifierName));
}

return f;
}).ToList();

return SyntaxFactory.CreateObjectProperty((e.Key as IdentifierSyntax)?.IdentifierName ?? "", SyntaxFactory.CreateArray(value));
}
else if (e.Value is ObjectSyntax valueAsObjectSyntax)
{
var syntax = CheckFunctionCallsInObjectSyntax(valueAsObjectSyntax);
var item = SyntaxFactory.CreateObjectProperty((e.Key as IdentifierSyntax)?.IdentifierName ?? "", syntax);
return item;
}

return e;
}).ToList();

return SyntaxFactory.CreateObject(value);
}

private static SyntaxBase CreateCommentSyntaxForFunctionCallSyntax(string functionName)
{
return SyntaxFactory.CreateInvalidSyntaxWithComment($" TODO : please fix the value assigned to this parameter `{functionName}()` ");
}

public static SyntaxBase GetValueForParameter(ParameterDeclarationSyntax syntax)
{
var defaultValue = SyntaxHelper.TryGetDefaultValue(syntax);
if (defaultValue != null)
Expand Down Expand Up @@ -102,45 +142,5 @@ private SyntaxBase GetValueForParameter(ParameterDeclarationSyntax syntax)

return SyntaxFactory.NewlineToken;
}

private SyntaxBase CheckFunctionCallsInObjectSyntax(ObjectSyntax objectSyntax)
{
var value = objectSyntax.Properties.Select(e =>
{
if (e.Value is FunctionCallSyntax valueAsFunctionCallSyntax)
{
return SyntaxFactory.CreateObjectProperty((e.Key as IdentifierSyntax)?.IdentifierName ?? "", CreateCommentSyntaxForFunctionCallSyntax(valueAsFunctionCallSyntax.Name.IdentifierName));
}
else if (e.Value is ArraySyntax valueAsFunctionArraySyntax)
{
var value = valueAsFunctionArraySyntax.Items.Select(f =>
{
if (f.Value is FunctionCallSyntax valueAsFunctionCallSyntax)
{
return SyntaxFactory.CreateArrayItem(CreateCommentSyntaxForFunctionCallSyntax(valueAsFunctionCallSyntax.Name.IdentifierName));
}

return f;
}).ToList();

return SyntaxFactory.CreateObjectProperty((e.Key as IdentifierSyntax)?.IdentifierName ?? "", SyntaxFactory.CreateArray(value));
}
else if (e.Value is ObjectSyntax valueAsObjectSyntax)
{
var syntax = CheckFunctionCallsInObjectSyntax(valueAsObjectSyntax);
var item = SyntaxFactory.CreateObjectProperty((e.Key as IdentifierSyntax)?.IdentifierName ?? "", syntax);
return item;
}

return e;
}).ToList();

return SyntaxFactory.CreateObject(value);
}

private SyntaxBase CreateCommentSyntaxForFunctionCallSyntax(string functionName)
{
return SyntaxFactory.CreateInvalidSyntaxWithComment($" TODO : please fix the value assigned to this parameter `{functionName}()` ");
}
}
}
23 changes: 5 additions & 18 deletions src/Bicep.Core/Emit/PlaceholderParametersJsonWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,25 +97,12 @@ private JToken GenerateTemplate(string contentVersion)
{
jsonWriter.WritePropertyName(parameterSymbol.Name);

var value = PlaceholderParametersBicepParamWriter.GetValueForParameter(parameterSymbol.DeclaringParameter);
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't feel safe to me to depend on this method and then convert from Bicep -> JSON, because it can return invalid Bicep syntax - e.g. here:

bicep/src/Bicep.Core/Emit/PlaceholderParametersBicepParamWriter.cs

Lines 141 to 144 in eb0673b

}
return SyntaxFactory.NewlineToken;
}

My suggestion would probably be to copy the pattern used in PlaceholderParametersBicepParamWriter and introduce a dedicated method like GetValueForParameter which is able to accurately generate the right type of JSON node, including JSON comments if we're unable to generate valid syntax.

Copy link
Member Author

@ouldsid ouldsid Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

is parameterDeclarationSyntax propre to bicep or can also be used by json ? my understanding is that : this.Context.SemanticModel.Root.ParameterDeclarations will be the same whether we are dealing with generating bicep or json file ...am I missing something? the value you pointed out will be handled by the the added if statement you have pointed out with your second comment


jsonWriter.WriteStartObject();
switch (parameterSymbol.Type.Name)
{
case "string":
emitter.EmitProperty("value", "");
break;
case "int":
emitter.EmitProperty("value", () => jsonWriter.WriteValue(0));
break;
case "bool":
emitter.EmitProperty("value", () => jsonWriter.WriteValue(false));
break;
case "object":
emitter.EmitProperty("value", () => { jsonWriter.WriteStartObject(); jsonWriter.WriteEndObject(); });
break;
case "array":
emitter.EmitProperty("value", () => { jsonWriter.WriteStartArray(); jsonWriter.WriteEndArray(); });
break;
}

emitter.EmitProperty("value", value);
Copy link
Member

Choose a reason for hiding this comment

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

One additional behavior change this will introduce is that a default of "" will be used for any parameter using a user-defined type if I'm reading this correctly. For example, I believe the following template:

param foo {
  requiredProperty: string
  optionalProperty: string?
}

will generate a .bicepparam file of

param foo = ''

Copy link
Member Author

Choose a reason for hiding this comment

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

The way to generate .bicepparam should not change. The change is to make .parameters.json consistant with it. Having said that this might be an issue but unrelated to this change


jsonWriter.WriteEndObject();
}

Expand Down
Loading