Watch for creation/deletion/modification of local bicepconfig.json file #4038

bhsubra · 2021-08-16T15:58:21Z

anthony-c-martin · 2021-08-16T16:06:39Z

src/Bicep.LangServer/Configuration/BicepConfigChangeHandler.cs

+            {
+                // We want to push error dignostics in case of invalid bicepconfig.json.
+                // We won't be able to apply validation until user fixes issues in config file
+                return true;


Why return true if it's unparseable?

This will enable to re trigger compilation and push error diagnostics. RetriggerCompilationOfSourceFilesInWorkspace_WithInvalidBicepConfigFile_ShouldRetriggerCompilation unit test covers this scenario. Let me know if I am missing something here

This code doesn't exist anymore. Made updates here:
7c53107

anthony-c-martin · 2021-08-16T16:11:49Z

src/Bicep.Core/Configuration/ConfigHelper.cs

@@ -39,7 +37,7 @@ private IConfigurationRoot BuildConfig(string? localFolder)
            // load the default settings from file embedded as resource
            var assembly = Assembly.GetExecutingAssembly();
            var names = assembly.GetManifestResourceNames();
-            var defaultConfigResourceName = names.FirstOrDefault(n => n.EndsWith(SettingsFileName));
+            var defaultConfigResourceName = names.FirstOrDefault(n => n.EndsWith(LanguageConstants.BicepConfigSettingsFileName));


I know this is existing code, but I think it would be better to be precise here and hard-code the manifest resource path, rather than searching for it.

Updated here:
6992577

anthony-c-martin · 2021-08-16T16:16:05Z

src/Bicep.LangServer/Configuration/BicepConfigChangeHandler.cs

+            try
+            {
+                JObject jObject = JObject.Parse(bicepConfig);
+                return jObject.IsValid(bicepConfigSchema);


Do we need to do server-side schema validation? Performance on schema validation can be quite slow.

If so, shouldn't this be built into the Bicep core libraries to provide a consistent experience for CLI & LSP users?

I did this to avoid re triggering compilation on every key stroke. I couldn't think of a better way to handle the scenario. I don't quite understand your question on moving to bicep core libraries. Do you mean the validation logic?

So at present, ConfigHelper handles the loading and reading of config deep inside the semantic model:

bicep/src/Bicep.Core/Semantics/SemanticModel.cs

Line 68 in 3e6e581

this.linterAnalyzerLazy = new Lazy<LinterAnalyzer>(() => new LinterAnalyzer());

->

bicep/src/Bicep.Core/Analyzers/Linter/LinterAnalyzer.cs

Line 26 in 3e6e581

private readonly ConfigHelper defaultConfigHelper = new ConfigHelper();

->

bicep/src/Bicep.Core/Configuration/ConfigHelper.cs

Line 26 in 3e6e581

this.Config = BuildConfig(Directory.GetCurrentDirectory());

->

bicep/src/Bicep.Core/Configuration/ConfigHelper.cs

Lines 51 to 57 in 3e6e581

if (DiscoverLocalConfigurationFile(localFolder) is string localConfig)

{

// we must set reloadOnChange to false here - if it set to true, then ConfigurationBuilder will initialize

// a FileSystem.Watcher instance - which has severe performance impact on non-Windows OSes (https://github.com/dotnet/runtime/issues/42036)

configBuilder.AddJsonFile(localConfig, optional: true, reloadOnChange: false);

this.CustomSettingsFileName = localConfig;

}

I'm wondering if the creator of the SemanticModel instance should be responsible for creating the config and passing it in as a constructor argument, to avoid unneccessary file reads. The ConfigHelper itself could also be responsible for validating the config file, rather than relying on a JSON schema validator.

Updated as part of this commit:
7c53107

anthony-c-martin · 2021-08-16T16:18:48Z

src/Bicep.LangServer/Bicep.LangServer.csproj

@@ -11,11 +11,18 @@
  </ItemGroup>

  <ItemGroup>
+    <PackageReference Include="Newtonsoft.Json.Schema" Version="3.0.14" />


This library has some strange commercial licensing setup attached to it which I've never fully understood. Are there any issues using it here? See https://www.newtonsoft.com/jsonschema ("Licensing & Pricing") for more info.

At one point it’d be nice to have errors on invalid values, but viscose can provide that out of the box, I think we don’t need to build validation in bicep itself.

Thanks for pointing this. Looks like it was pulled out of Json.Net(which is under MIT license) and is not free anymore.

I am trying couple other libraries. Let me know if you have any suggestions.

I think I can use this one instead: https://www.nuget.org/packages/NJsonSchema/. It's under MIT license.

As discussed offline, removed the validation as part of this commit:
7c53107

anthony-c-martin · 2021-08-16T16:21:49Z

src/Bicep.LangServer/Handlers/BicepDidChangeWatchedFilesHandler.cs

+            // Retrigger compilation of source files in workspace when local bicepconfig.json file is edited
+            if (bicepConfigFileChangeEvents.Any())
+            {
+                bicepConfigChangeHandler.RetriggerCompilationOfSourceFilesInWorkspace(compilationManager, bicepConfigFileChangeEvents.First(), workspace);


Currently every time we rebuild a semantic model, we reload configuration from disk. Should we cache the deserialized configuration and pass it down to the semantic model, to avoid this?

This is technically a different issue, but it feels like it might be simpler to address under this PR as it may change the design significantly.

Updated here:
7c53107

Let me know if you have any questions/concerns. Thanks!

anthony-c-martin · 2021-08-16T16:23:17Z

src/Bicep.LangServer/Handlers/BicepDidChangeWatchedFilesHandler.cs

+            IEnumerable<FileEvent> bicepConfigFileChangeEvents = fileEvents.Where(x => x.Uri.Path.EndsWith(LanguageConstants.BicepConfigSettingsFileName));
+
+            // Retrigger compilation of source files in workspace when local bicepconfig.json file is edited
+            if (bicepConfigFileChangeEvents.Any())


I think it's OK to assume that any config file change => recompile everything, but is it worth trying to be more precise (e.g. figuring out which files will actually be impacted by a given config file)?

Might not be worth optimizing for a relatively rare scenario, just wanted to raise it.

I made some changes as part of below commit, which should cover this scenario. Recompilation will only be triggered when LanguageId is "bicep":
7c53107

Let me know if I am missing something.

anthony-c-martin · 2021-08-23T14:45:58Z

src/Bicep.LangServer/Providers/BicepCompilationProvider.cs

@@ -41,9 +41,9 @@ public CompilationContext Update(IReadOnlyWorkspace workspace, CompilationContex
            return this.CreateContext(syntaxTreeGrouping, modelLookup);


Do you need to transfer the previous configHelper here?

FWIW, I generally feel like default parameters are worth avoiding in C#, as they can be very easy to miss when refactoring, and in this case - it's very important to get it right. IMO it would be worth making configHelper a non-default param on the CreateContext method and Compilation constructor.

Makes sense. Updated here:
74db298

anthony-c-martin · 2021-08-23T14:54:07Z

src/Bicep.LangServer/BicepCompilationManager.cs

+                        ConfigHelper? configHelper = null;
+
+                        if (!reloadBicepConfig &&
+                        activeContexts.TryGetValue(documentUri, out CompilationContext? compilationContext) &&
+                        compilationContext is not null &&
+                        compilationContext.Compilation.ConfigHelper is ConfigHelper previousConfigHelper)
+                        {
+                            configHelper = previousConfigHelper;
+                        }


Can this be simplified to:

var configHelper = activeContexts.TryGetValue(documentUri)?.Compilation.ConfigHelper; if (reloadBicepConfig) { configHelper = null; }

EDIT: In fact, looks like you already have access to the previous context with the prevContext variable, so you shouldn't need to fetch it back with TryGetValue().

Generally - we need to be super careful accessing this dictionary, as we're inside the AddOrUpdate method of a concurrent dictionary - need to be 100% sure what we're doing is thread-safe.

Simplified as part of this commit:
74db298

bhsubra · 2021-08-25T23:58:01Z

src/Bicep.LangServer/Configuration/BicepConfigChangeHandler.cs

+                if (fileResolver.TryRead(sourceFileUri, out string? bicepFileContents, out ErrorBuilderDelegate _) &&
+                    !string.IsNullOrWhiteSpace(bicepFileContents))
+                {
+                    compilationManager.UpsertCompilation(DocumentUri.From(sourceFileUri), null, bicepFileContents, reloadBicepConfig: true);


Had an offline conversation with @majastrz. I'll be making changes to use RefreshCompilation(..) instead of UpsertCompilation(..) as the latter doesn't handle unsaved changes.

Updated here:
58045fa

anthony-c-martin · 2021-08-27T17:40:08Z

src/Bicep.Core/Semantics/Compilation.cs

+            if (configHelper is null)
+            {
+                ConfigHelper = new ConfigHelper();
+            }
+            else
+            {
+                ConfigHelper = configHelper;
+            }


[nit] Can simplify to just:

ConfigHelper = configHelper ?? new ConfigHelper();

Updated as part of this commit:
b90b61e

anthony-c-martin · 2021-08-27T17:44:39Z

src/Bicep.LangServer/Handlers/BicepCodeActionHandler.cs

+            IEnumerable<IDiagnostic> diagnostics;
+
+            try
+            {
+                diagnostics = compilationContext.Compilation.GetEntrypointSemanticModel().GetAllDiagnostics();
+            }
+            catch
+            {
+                return Task.FromResult(new CommandOrCodeActionContainer());
+            }
+


What's the purpose of this code block?

Hmmm I don't recollect why I added this. Reverted the change here:
765c098

anthony-c-martin · 2021-08-27T17:45:54Z

src/Bicep.LangServer/Handlers/BicepDidChangeWatchedFilesHandler.cs

+            if (bicepConfigFileChangeEvents.Any())
+            {
+                Uri uri = bicepConfigFileChangeEvents.First().Uri.ToUri();
+                BicepConfigChangeHandler.RefreshCompilationOfSourceFilesInWorkspace(compilationManager, uri, workspace, string.Empty);


Use null here - otherwise you can't differentiate between "empty file" and "deleted file"

In fact, what's the meaning of string.Empty? It seems from the implementation of RefreshCompilationOfSourceFilesInWorkspace() that this signifies a file removal - but are you checking whether it's a delete here?

As discussed offline, updated to use null here and added comments:
d3cf19a

anthony-c-martin · 2021-08-27T17:47:47Z

src/Bicep.LangServer/Configuration/BicepConfigChangeHandler.cs

+{
+    public class BicepConfigChangeHandler
+    {
+        public static void RefreshCompilationOfSourceFilesInWorkspace(ICompilationManager compilationManager, Uri bicepConfigUri, IWorkspace workspace, string bicepConfigFileContents)


Make the bicepConfigFileContents param nullable (string?), and change the below check from string.IsNullOrWhiteSpace(bicepConfigFileContents) -> bicepConfigFileContents is null - it's worth being precise to be able to differentiate between "deleted file" and "file with whitespace"

As discussed offline, updated to use null here and added comments:
d3cf19a

anthony-c-martin · 2021-08-27T17:49:33Z

src/Bicep.Core/Workspaces/Workspace.cs

@@ -26,6 +28,13 @@ public IEnumerable<ISourceFile> GetSourceFilesForDirectory(Uri fileUri)
        public ImmutableDictionary<Uri, ISourceFile> GetActiveSourceFilesByUri()
            => activeFiles.ToImmutableDictionary();

+        public void UpsertActiveBicepConfig(BicepConfig? bicepConfig)
+        {
+            activeConfig = bicepConfig;


Isn't it possible for there to be multiple bicepconfig.json files in a workspace? How do you decide which one to replace?

With multiple config files, we should always pick one in current directory or closer to current directory correct?

Passing in folder information as part of this commit:
bd34647

Added few integration tests to test multiple config files scenario. Let me know if you have any questions.

anthony-c-martin · 2021-08-27T18:02:03Z

src/Bicep.LangServer/BicepCompilationManager.cs

@@ -33,6 +34,8 @@ public class BicepCompilationManager : ICompilationManager
        // represents compilations of open bicep files
        private readonly ConcurrentDictionary<DocumentUri, CompilationContext> activeContexts = new ConcurrentDictionary<DocumentUri, CompilationContext>();

+        private ConfigHelper? configHelper;


I think this is going to need some change - some things to be aware of:

This class can handle concurrent requests so has to be race-condition safe; a mutable field without locking feels like it's going to be problematic.

There can be multiple bicepconfig.json files active in the workspace, so storing the value in a single field feels like it can yield incorrect results.

Updated here:
5f750d2

anthony-c-martin · 2021-09-01T15:12:19Z

src/Bicep.Core/Analyzers/Linter/LinterAnalyzer.cs

@@ -77,23 +74,13 @@ public LinterAnalyzer()

        internal IEnumerable<IDiagnostic> Analyze(SemanticModel semanticModel, ConfigHelper? overrideConfig = default)


Now that we have activeConfigHelper set via the constructor, do we need to keep this overload which accepts overrideConfig? If so, do you know which code paths require it? It's always felt like an anti-pattern, as it negates the ability to cache and reuse semantic models for diagnostics.

It was used in tests. I removed passing overrideConfig as part of this commit:
1e2de9b

anthony-c-martin · 2021-09-01T15:12:37Z

src/Bicep.Core/Analyzers/Linter/LinterAnalyzer.cs

        {
-            this.activeConfigHelper = this.defaultConfigHelper;
+            this.activeConfigHelper = configHelper;


[nit] rename activeConfigHelper to just configHelper

Updated as part of this commit:
1e2de9b

anthony-c-martin · 2021-09-01T15:16:33Z

src/Bicep.Core/Configuration/BicepConfig.cs

@@ -2,13 +2,19 @@
 // Licensed under the MIT License.

 using System;
-using System.Collections.Generic;
-using System.Text;

 namespace Bicep.Core.Configuration
 {
    public class BicepConfig


I'm glad you found a way to reuse this strange class 😄

anthony-c-martin · 2021-09-07T16:40:32Z

src/Bicep.Core/Workspaces/Workspace.cs

+            if (activeBicepConfigs.ContainsKey(uri))
+            {
+                activeBicepConfigs.TryRemove(uri, out _);
+            }


You don't need the ContainsKey check - TryRemove does this.

Removed here:
bbb280e

anthony-c-martin · 2021-09-07T16:43:21Z

src/Bicep.LangServer/BicepCompilationManager.cs

+                if (reloadBicepConfig &&
+                    workspace.TryGetSourceFile(documentUri.ToUri(), out ISourceFile? sourceFile) &&
+                    sourceFile is BicepFile)
+                {
+                    UpsertCompilationInternal(documentUri, null, sourceFile, reloadBicepConfig);
+                }


I'd like @majastrz to take a look at the changes to this class - as the mechanism he added for the module registry work may be useful here for triggering changes on a source file with no syntax differences.

This was the mechanism. Although, I'm planning to change the refresh part to take a module reference and have logic to compute which document URIs need to updated and then recompile them in optimal order.

anthony-c-martin · 2021-09-07T16:48:20Z

src/Bicep.LangServer/BicepCompilationManager.cs

+                }
+                else
+                {
+                    string bicepConfigInCurrentDirectory = Path.Combine(Directory.GetCurrentDirectory(), LanguageConstants.BicepConfigSettingsFileName);


This doesn't seem right - won't Directory.GetCurrentDirectory() give you the current working directory? I'd expect there to be some code you can reuse for fetching the correct bicepconfig.json to use for a given Bicep file - would be better to commonize it and use it here rather than reimplementing it.

EDIT: I think this implementation is in ConfigHelper.DiscoverLocalConfigurationFile()

Updated to use ConfigHelper.DiscoverLocalConfigurationFile(..) here:
bbb280e

shenglol · 2021-09-13T22:19:21Z

src/Bicep.Core/Configuration/ConfigHelper.cs

 using System.Reflection;

 namespace Bicep.Core.Configuration
 {
    public class ConfigHelper
    {
-        private const string SettingsFileName = "bicepconfig.json";
+        private const string bicepConfigResourceName = "Bicep.Core.Configuration.bicepconfig.json";
+        private readonly IFileResolver fileResolver;

        /// <summary>
        /// Property exposes the configuration root
        /// that is currently loaded
        /// </summary>
        public IConfigurationRoot Config { get; private set; }


One question (non blocking, not related to this PR) - is there any reason why we are using the raw IConfigurationRoot interface? Would it be possible to use ConfigurationBinder.Bind to bind the IConfigurationRoot to an instance of a strongly typed config class?

The linter rules currently read dynamically from the config:

bicep/src/Bicep.Core/Analyzers/Linter/Rules/NoHardcodedEnvironmentUrlsRule.cs

Lines 40 to 44 in de8b847

this.disallowedHosts = GetArray(nameof(DisallowedHosts), Array.Empty<string>())

.ToImmutableArray();

this.MinimumHostLength = this.disallowedHosts.Value.Min(h => h.Length);

this.excludedHosts = GetArray(nameof(ExcludedHosts), Array.Empty<string>())

.ToImmutableArray();

It would be nice to have a strongly-typed config class (e.g. for the alias work you're doing), but might need to keep some dynamic-ness for linters if we want 3rd parties to be able to author their own.

I'll sync up with you guys offline to see if we can improve something here that will help alias work. Thanks!

anthony-c-martin

Looks good!

anthony-c-martin reviewed Aug 16, 2021

View reviewed changes

bhsubra requested review from majastrz, MarcusFelling and anthony-c-martin August 16, 2021 17:35

anthony-c-martin reviewed Aug 23, 2021

View reviewed changes

bhsubra requested a review from anthony-c-martin August 25, 2021 16:53

bhsubra commented Aug 25, 2021

View reviewed changes

anthony-c-martin reviewed Aug 27, 2021

View reviewed changes

bhsubra force-pushed the dev/bhsubra/WatchForBicepConfigUpdates branch from 54920d8 to 740b7f8 Compare August 30, 2021 05:35

bhsubra requested a review from anthony-c-martin August 31, 2021 16:21

anthony-c-martin reviewed Sep 1, 2021

View reviewed changes

bhsubra requested a review from anthony-c-martin September 1, 2021 18:15

bhsubra force-pushed the dev/bhsubra/WatchForBicepConfigUpdates branch 2 times, most recently from 37e6507 to ff64dd3 Compare September 4, 2021 00:00

anthony-c-martin reviewed Sep 7, 2021

View reviewed changes

bhsubra added 20 commits September 13, 2021 11:54

Cleaned up integration tests

dc28964

Updated to send null when bicepConfigContents is not available

7fd8319

Addressed cr comments

db58634

Reverted changes to CodeActionHandler

d4be063

Some cleanup with comments

afeef32

Removed overrideconfig

a382cc5

Rebase and fix build break

713fe3d

Rebase from main

2fa4313

Addressed cr comments:

7fffb86

Reverted some changes

0a441bd

Deleted some unwanted changes

010026f

Added comment in BicepCompilationManager

74b29e1

Some cleanup

f18b718

Rebased from main and fixed conflicts

02b534e

Attempt to fix test flakiness

9f42ada

Added DoNotParallelize attribute

bf8df63

Removed DoNotParallelize attribute

4f0adef

Attempt to fix test issue

25f639d

Fix test issue

9a5d5c3

Added tests to make sure config file isn't read multiple times

5142ece

bhsubra force-pushed the dev/bhsubra/WatchForBicepConfigUpdates branch from 5376c82 to 5142ece Compare September 13, 2021 18:55

shenglol reviewed Sep 13, 2021

View reviewed changes

majastrz approved these changes Sep 13, 2021

View reviewed changes

anthony-c-martin approved these changes Sep 14, 2021

View reviewed changes

bhsubra merged commit 1b40ff5 into main Sep 14, 2021

bhsubra deleted the dev/bhsubra/WatchForBicepConfigUpdates branch September 14, 2021 14:56

This was linked to issues Sep 14, 2021

Create a configuration data model for bicepconfig.json and monitor edits #4009

Closed

Invalid bicepconfig.json file corrupts bicep files active in workspace #4027

Closed

bhsubra removed a link to an issue Sep 14, 2021

Create a configuration data model for bicepconfig.json and monitor edits #4009

Closed

bhsubra linked an issue Sep 14, 2021 that may be closed by this pull request

Create a configuration data model for bicepconfig.json and monitor edits #4009

Closed

	if (DiscoverLocalConfigurationFile(localFolder) is string localConfig)
	{
	// we must set reloadOnChange to false here - if it set to true, then ConfigurationBuilder will initialize
	// a FileSystem.Watcher instance - which has severe performance impact on non-Windows OSes (https://github.com/dotnet/runtime/issues/42036)
	configBuilder.AddJsonFile(localConfig, optional: true, reloadOnChange: false);
	this.CustomSettingsFileName = localConfig;
	}

		@@ -41,9 +41,9 @@ public CompilationContext Update(IReadOnlyWorkspace workspace, CompilationContex
		return this.CreateContext(syntaxTreeGrouping, modelLookup);

		@@ -77,23 +74,13 @@ public LinterAnalyzer()

		internal IEnumerable<IDiagnostic> Analyze(SemanticModel semanticModel, ConfigHelper? overrideConfig = default)

	this.disallowedHosts = GetArray(nameof(DisallowedHosts), Array.Empty<string>())
	.ToImmutableArray();
	this.MinimumHostLength = this.disallowedHosts.Value.Min(h => h.Length);
	this.excludedHosts = GetArray(nameof(ExcludedHosts), Array.Empty<string>())
	.ToImmutableArray();

Watch for creation/deletion/modification of local bicepconfig.json file #4038

Watch for creation/deletion/modification of local bicepconfig.json file #4038

Conversation

bhsubra commented Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthony-c-martin Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthony-c-martin Aug 16, 2021 • edited Loading

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

anthony-c-martin Aug 16, 2021 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthony-c-martin Aug 23, 2021 • edited Loading

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

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

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

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

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

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 Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthony-c-martin left a comment

Choose a reason for hiding this comment

bhsubra commented Aug 16, 2021 •

edited

Loading

anthony-c-martin Aug 16, 2021 •

edited

Loading

anthony-c-martin Aug 16, 2021 •

edited

Loading

anthony-c-martin Aug 16, 2021 •

edited

Loading

anthony-c-martin Aug 23, 2021 •

edited

Loading

shenglol Sep 13, 2021 •

edited

Loading