Add support for using local json templates as modules #3367

shenglol · 2021-06-24T13:53:42Z

~~This is still a work in progress~~. Things to do:

Fix FileWatherTests and add tests
Module path completion for .json files (I'll send a follow-up PR to implement this)

Closes #3160.

shenglol · 2021-06-24T14:17:43Z

src/Bicep.Core/Syntax/Converters/StatementSyntaxConverter.cs

+
+namespace Bicep.Core.Syntax.Converters
+{
+ public static class StatementSyntaxConverter


The source code of the converters is basically copied from the decompiler with some tweaks. We cannot use the decompiler directly here because: 1) I wanted to avoid having Core reference Decompiler; 2) it won't work when a JSON template contains a nested template.

src/Bicep.Core/Syntax/SyntaxTreeFactory.cs

majastrz · 2021-06-30T01:52:31Z

src/Bicep.Core/Syntax/SyntaxTreeFactory.cs

+ : CreateSyntaxTree(fileUri, fileContents);
+
+ public static SyntaxTree CreateSyntaxTree(Uri fileUri, string fileContents) =>
+ PathHelper.HasBicepExtension(fileUri) || !PathHelper.HasAnyExtension(fileUri)


HasAnyExtension

Maybe I missed it elsewhere, but we are rather strict in what we allow as a module path. I think we should restrict the extensions we use to reference JSON templates. (We can always add more later, but once we open it up fully, we can never use any other extension in the path.)

Good pt. I'll reverse the logic so that it will only create JSON modules if the extension is .{json,jsonc,arm}.

src/Bicep.Core/Syntax/SyntaxTreeFactory.cs

src/Bicep.Core/Syntax/Converters/ProgramSyntaxConverter.cs

majastrz · 2021-06-30T02:09:59Z

public static class TestTypeHelper

General test strategy question: The decompiler was always a best effort tool, but now we are adding decompiler-like logic to the core.

If we added a new type or some additional type metadata to the JSON, we'll need to remember to update this logic so we can convert it back into a contract bicep file.

Is there something we could do to catch it? Maybe take all the Valid bicep test cases we have and try to use them as a module via both JSON and Bicep and then query the semantic model to see if there's any diff?

Refers to: src/Bicep.Core.UnitTests/Utils/TestTypeHelper.cs:19 in f98d3f1. [](commit_id = f98d3f1, deletion_comment = False)

majastrz · 2021-06-30T02:11:52Z

public static class TestTypeHelper

Or maybe take any data set that compiles, run its JSON through the converter logic and then see if the outputs and params match?

In reply to: 871043323

Refers to: src/Bicep.Core.UnitTests/Utils/TestTypeHelper.cs:19 in f98d3f1. [](commit_id = f98d3f1, deletion_comment = False)

src/Bicep.Core/Syntax/SyntaxTreeFactory.cs

shenglol · 2021-07-06T11:36:32Z

If we added a new type or some additional type metadata to the JSON, we'll need to remember to update this logic so we can convert it back into a contract bicep file.

Yeah this is definitely something that should be guarded by tests.

Or maybe take any data set that compiles, run its JSON through the converter logic and then see if the outputs and params match?

I like it. I'll add a data set test in this way.

anthony-c-martin · 2021-07-07T15:36:33Z

src/Bicep.Core/Syntax/JsonSyntaxTree.cs

+
+namespace Bicep.Core.Syntax
+{
+ public class JsonSyntaxTree : SyntaxTree


Have you considered representing the template semantically without actually generating a syntax tree? My concern with extending SyntaxTree is just that it's a corner case to maintain with quite different behavior. e.g. when raising diagnostics or doing type analysis, it'll be a subtlety that could easily be missed, or add complexity to the code, when it feels like there are real meaningful differences between a 'real' syntax tree and a generated one. Providing an accurate decompilation also feels like it could be a maintenance overhead in the long run which may not be necessary if we avoid generating syntax.

Yes, actually my original design was like that (to create a different semantic module for ARM templates), but I switch to the current approach to the existing semantic model and type checker as much as possible. Now I realize it might not be worth it given the maintenance overhead. @majastrz also raised the a similar concern (in the comments on adding tests to ensure the converters and the decompiler logic are in sync). It won't be too hard to switch back to my original design since I still have the code, but I would like to have a discussion with you and Marcin before moving forward.

As we discussed offline, I've refactored the code to create a different semantic model for JSON templates, which removes the converters and avoids extending SyntaxTree.

miqm · 2021-07-12T06:17:15Z

src/Bicep.Core.Samples/Files/Modules_CRLF/main.json

@@ -1,1829 +1,1914 @@
-{


this is odd...

This should to be due to line endings normalization which is expected.

shenglol · 2021-07-12T13:36:54Z

src/Bicep.Core/Syntax/SyntaxTree.cs


 namespace Bicep.Core.Syntax
 {
- public class SyntaxTree
+ public class SyntaxTree : ISourceFile


Per our discussion offline, I'm going to rename the class to BicepFile in a follow-up PR. I've created #3560 to track it.

majastrz · 2021-07-12T23:11:10Z

I played with the VSIX a bit. I really like how even unchanged JSON changes are noticed by the consuming Bicep file in VS code.

majastrz · 2021-07-12T23:20:56Z

src/Bicep.Core/Workspaces/SourceFileFactory.cs

+
+ ValidateTemplate(template);
+
+ var templateObject = JObject.Parse(fileContents, new JsonLoadSettings


JsonLoadSettings

Does the default duplicate property behavior match the runtime? Should we set it explicitly?

Yes it matches (the runtime does not specify DuplicatePropertyNameHandling).

majastrz

Add support for using local json templates as modules

e654bc9

shenglol commented Jun 24, 2021

View reviewed changes

shenglol added 4 commits June 28, 2021 14:16

Fix compilation manager tests

5063f9a

Update compilation manager tests to cover non-bicep files

9eb0b70

Add sample tests for JSON modules

fc042f0

Add more tests and fix bugs

d75400d

shenglol marked this pull request as ready for review June 29, 2021 12:42

Clean up csproj

f98d3f1

majastrz reviewed Jun 30, 2021

View reviewed changes

src/Bicep.Core/Syntax/SyntaxTreeFactory.cs Outdated Show resolved Hide resolved

majastrz reviewed Jun 30, 2021

View reviewed changes

src/Bicep.Core/Syntax/SyntaxTreeFactory.cs Outdated Show resolved Hide resolved

majastrz reviewed Jun 30, 2021

View reviewed changes

src/Bicep.Core/Syntax/Converters/ProgramSyntaxConverter.cs Outdated Show resolved Hide resolved

majastrz reviewed Jun 30, 2021

View reviewed changes

src/Bicep.Core/Syntax/SyntaxTreeFactory.cs Outdated Show resolved Hide resolved

Fix a typo

234b59e

shenglol added 3 commits July 6, 2021 20:11

Merge branch 'main' into shenglol/local-json-file-as-module

6274903

Restrict the extensions used to reference JSON templates

2a4ce68

Use TemplateEngine to simplify converters

2800d97

anthony-c-martin reviewed Jul 7, 2021

View reviewed changes

alex-frankel mentioned this pull request Jul 8, 2021

Scoping does not work correctly for 'naive' nested deployments #1828

Closed

miqm reviewed Jul 12, 2021

View reviewed changes

shenglol added 2 commits July 12, 2021 20:32

Create a different semantic model for ARM templates

05f519e

Merge branch 'main' into shenglol/local-json-file-as-module

f2adeca

shenglol commented Jul 12, 2021

View reviewed changes

majastrz reviewed Jul 12, 2021

View reviewed changes

majastrz approved these changes Jul 12, 2021

View reviewed changes

Merge branch 'main' into shenglol/local-json-file-as-module

7b9d96c

shenglol enabled auto-merge (squash) July 13, 2021 04:01

shenglol merged commit b416c57 into main Jul 13, 2021

shenglol deleted the shenglol/local-json-file-as-module branch July 13, 2021 04:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for using local json templates as modules #3367

Add support for using local json templates as modules #3367

shenglol commented Jun 24, 2021 •

edited

Loading

shenglol Jun 24, 2021 •

edited

Loading

majastrz Jun 30, 2021

shenglol Jul 6, 2021

majastrz commented Jun 30, 2021

majastrz commented Jun 30, 2021

shenglol commented Jul 6, 2021

anthony-c-martin Jul 7, 2021

shenglol Jul 8, 2021

shenglol Jul 12, 2021

miqm Jul 12, 2021

shenglol Jul 12, 2021

shenglol Jul 12, 2021 •

edited

Loading

majastrz commented Jul 12, 2021

majastrz Jul 12, 2021

shenglol Jul 13, 2021

majastrz left a comment


		ValidateTemplate(template);

		var templateObject = JObject.Parse(fileContents, new JsonLoadSettings

Add support for using local json templates as modules #3367

Add support for using local json templates as modules #3367

Conversation

shenglol commented Jun 24, 2021 • edited Loading

shenglol Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majastrz commented Jun 30, 2021

majastrz commented Jun 30, 2021

shenglol commented Jul 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shenglol Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

majastrz commented Jul 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majastrz left a comment

Choose a reason for hiding this comment

shenglol commented Jun 24, 2021 •

edited

Loading

shenglol Jun 24, 2021 •

edited

Loading

shenglol Jul 12, 2021 •

edited

Loading