Roslyn analyzer setup included in Altinn.App.Core to catch configuration errors based on static analysis #651

martinothamar · 2024-05-16T00:30:17Z

Description

This PR includes a Roslyn analyzer setup that we can use for various things.
To start, this analyzes the applicationmetadata.json file and reports errors when it isn't able to resolve classRef to its corresponding C# class

The analyzer (and its' Newtonsoft.Json dependency) is packaged alongside Altinn.App.Core in the analyzers folder

I use the 4.1.0 version of the Roslyn libraries for the analyzer. It's supported by almost all of the VS 2022 versions and has been included in the .NET SDK since 6.0 I think.

Referencing locally

<ProjectReference Include="../../../app-lib-dotnet/src/Altinn.App.Core/Altinn.App.Core.csproj" />
<ProjectReference Include="../../../app-lib-dotnet/src/Altinn.App.Api/Altinn.App.Api.csproj">
    <CopyToOutputDirectory>lib\$(TargetFramework)\*.xml</CopyToOutputDirectory>
</ProjectReference>
<ProjectReference Include="../../../app-lib-dotnet/src/Altinn.App.Analyzers/Altinn.App.Analyzers.csproj" PrivateAssets="all" ReferenceOutputAssembly="false" OutputItemType="Analyzer" />

Related Issue(s)

N/A

Verification

Your code builds clean without any errors or warnings
Manual testing done (required)
Relevant automated test added (if you find this hard, leave it and we'll help out)
All tests run green

Documentation

User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

src/Altinn.App.Analyzers/MetadataAnalyzer.cs

ivarne

Cool...

We should probably make these Errors instead of Warnings unless we find a way to make CI build run with warnings as errors, because lots of apps has many warnings and these pretty serious issues will just drown.

src/Altinn.App.Analyzers/MetadataAnalyzer.cs

martinothamar · 2024-05-21T09:11:22Z

We should probably make these Errors instead of Warnings unless we find a way to make CI build run with warnings as errors, because lots of apps has many warnings and these pretty serious issues will just drown.

If we want to deploy this before v9 (I would like to), then we can't make them errors I think.

ivarne · 2024-05-21T09:21:17Z

I think we can make compile errors in broken apps?

src/Altinn.App.Analyzers/MetadataAnalyzer.cs

src/Altinn.App.Analyzers/ApplicationMetadataFileReader.cs

src/Altinn.App.Analyzers/MetadataAnalyzer.cs

martinothamar · 2024-05-25T13:56:26Z

This PR is ready for proper review now!

Testing infra was as usual not trivial at all, and took the majority of time, but I'm really happy with the result.
There is a global xunit collection to run all tests serially. The initialization of the fixture (building the Roslyn workspace etc) takes relatively long compared to the actual tests so I think that's the right tradeoff? See output below

This is a "hot" build, building the workspace took 2s and the tests that use them take take less than a second at least. This approach also gives us some flexibility as we can make physical changes to the project files safely.
The only diagnostic not tested is the ProjectFileNotFound one. I considered the new IFileProvider stuff to be able to fake the file system access/checks but that would bring in more dependencies into the analyzer which is a bit of a hassle. Could also build our own as the amount of code isn't huge. The output of the diagnostics also use Verify snapshots. I've used the Verify.SourgeGenerators package before but since we only want to verify snapshots here I opted for copying out only the Diagnostic JSON converter which was used there. To verify error handling I opted for injectable delegates that are invoked from the analyzer project. I added some comments to the AltinnTestAppFixture and the ProjectModification files to explain more of the testing approach.

Another change is using PolySharp to polyfill some stuff such as records. That enabled me to use a pattern that I'm really fond of, especially in places where precise error handling is very important. There is a ApplicationMetadataResult type that functions as a sort of union type using a closed type hierarchy. I use an analyzer to suppress exhaustiveness check diagnostics. Code:

Callee: https://github.com/Altinn/app-lib-dotnet/pull/651/files#diff-481c24765c42685ba9fdf20ea3d628bbfada6d7c92ccc1e915fb8fed7d8f9269R6-R59
Caller: https://github.com/Altinn/app-lib-dotnet/pull/651/files#diff-7c717ef7bb4564e83aed692d56aca3056beef2b6b1bcfc66af0137c4663bd344R67-R98

Note that the caller doesn't need a default case in the switch statement, yet all error conditions are propery handled.

I also tried to add proper locations to diagnostics by capturing lineinfo from Newtonsoft.Json while serializing, seems to work for the current tests.

@ivarne re

I think we can make compile errors in broken apps?

I think this is a reasonable perspective for sure, but another would be that making those error severity would change build-time behavior. So some apps that are broken now still build, but that changes if we make diagnostics error severity right now. Another approach would be to keep them as warning now, but convert them to errors for v9. Those who want to be strict can turn on WarningsAsErrors, so we still enable that in this scenario. We agreed that we wanted to be strict about breaking changes, so that's why I opted for the current approach. I'm open different solutions, but we should probably consider these subtleties

martinothamar · 2024-05-25T14:53:13Z

Since we're trying to improve DevEx with these changes, I thought I'd verify the perf hit of adding static analysis (since this will run per compilation for apps). Added a benchmarking harness, with running the MetadataAnalyzer as a first benchmark. Benchmark results:

CPU profile:

martinothamar · 2024-05-27T21:38:53Z

A new case that popped up - a user had mistakenly created a Setting.json file in the ui/ folder as opposed to Settings.json 😄

ivarne · 2024-05-27T21:42:49Z

A new case that popped up - a user had mistakenly created a Setting.json file in the ui/ folder as opposed to Settings.json 😄

Or even funnier, they could name it settings.json instead of Settings.json and see that everything work locally on windows, but have the config missing in docker in production. (I have done that)

src/Altinn.App.Analyzers/MetadataAnalyzer.cs

+        foreach (var dataType in metadata.DataTypes.Value)
+        {
+            if (dataType.AppLogic.Value is not { } appLogic)
+                continue;
+
+            var classRef = appLogic.ClassRef;
+
+            var classRefSymbol = context.Compilation.GetTypeByMetadataName(classRef.Value);
+
+            if (classRefSymbol is null)
+            {
+                context.ReportDiagnostic(
+                    Diagnostic.Create(
+                        Diagnostics.ApplicationMetadata.DataTypeClassRefInvalid,
+                        GetLocation(filePath, classRef, sourceText),
+                        classRef.Value,
+                        dataType.Id.Value
+                    )
+                );
+            }
+        }


src/Altinn.App.Analyzers/MetadataAnalyzer.cs

+            foreach (var layoutSet in layoutSets)
+            {
+                if (layoutSet.Id.Value.Equals(showLayout.Value, StringComparison.Ordinal))
+                {
+                    foundLayoutSet = layoutSet;
+                }
+            }


src/Altinn.App.Analyzers/MetadataAnalyzer.cs

+            foreach (var dataTypeMetadata in appMetadataContent.Value.DataTypes.Value)
+            {
+                if (dataTypeMetadata.Id.Value.Equals(dataType, StringComparison.Ordinal))
+                {
+                    foundDataTypeInfo = dataTypeMetadata;
+                }
+            }


src/Altinn.App.Analyzers/ApplicationMetadata/ApplicationMetadataJsonReader.cs

+        catch (Exception ex)
+        {
+            return new ApplicationMetadataParseResult.Error(ex);
+        }


src/Altinn.App.Analyzers/ApplicationMetadata/ApplicationMetadataFileReader.cs

+        catch (Exception ex)
+        {
+            if (ex is OperationCanceledException)
+                return new ApplicationMetadataResult.Cancelled();
+            return new ApplicationMetadataResult.CouldNotReadFile(ex, file);
+        }


src/Altinn.App.Analyzers/Layouts/LayoutSetsFileReader.cs

+        catch (Exception ex)
+        {
+            if (ex is OperationCanceledException)
+                return new LayoutSetsResult.Cancelled();
+            return new LayoutSetsResult.CouldNotParse(ex, sourceText, file);
+        }


src/Altinn.App.Analyzers/MetadataAnalyzer.cs

+        catch (Exception ex)
+        {
+            if (ex is OperationCanceledException)
+                return;
+            additionalFileAnalysisContext.ReportDiagnostic(
+                Diagnostic.Create(Diagnostics.UnknownError, Location.None, ex.Message, ex.StackTrace)
+            );
+        }


src/Altinn.App.Analyzers/MetadataAnalyzer.cs

+        catch (Exception ex)
+        {
+            if (ex is OperationCanceledException)
+                return;
+            compilationAnalysisContext.ReportDiagnostic(
+                Diagnostic.Create(Diagnostics.UnknownError, Location.None, ex.Message, ex.StackTrace)
+            );
+        }


src/Altinn.App.Analyzers/Json/JsonExtensions.cs

+                long v => v == 0 ? 1 : (int)Math.Floor(Math.Log10(v) + 1),
+                uint v => v == 0 ? 1 : (int)Math.Floor(Math.Log10(v) + 1),
+                ulong v => v == 0 ? 1 : (int)Math.Floor(Math.Log10(v) + 1),
+                float v => v == 0 ? 1 : (int)Math.Floor(Math.Log10(v) + 1),


src/Altinn.App.Analyzers/Json/JsonExtensions.cs

+                uint v => v == 0 ? 1 : (int)Math.Floor(Math.Log10(v) + 1),
+                ulong v => v == 0 ? 1 : (int)Math.Floor(Math.Log10(v) + 1),
+                float v => v == 0 ? 1 : (int)Math.Floor(Math.Log10(v) + 1),
+                double v => v == 0 ? 1 : (int)Math.Floor(Math.Log10(v) + 1),


sonarqubecloud · 2024-06-11T17:20:25Z

Quality Gate failed

Failed conditions
62.7% Coverage on New Code (required ≥ 65%)
49.55% Condition Coverage on New Code (required ≥ 65%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

martinothamar force-pushed the feat/roslyn-analyzer-included branch from 9f70ad3 to 685d70a Compare May 16, 2024 00:31

github-advanced-security bot found potential problems May 16, 2024

View reviewed changes

src/Altinn.App.Analyzers/MetadataAnalyzer.cs Fixed Show fixed Hide fixed

martinothamar self-assigned this May 16, 2024

martinothamar added the feature Label Pull requests with new features. Used when generation releasenotes label May 16, 2024

ivarne reviewed May 21, 2024

View reviewed changes

src/Altinn.App.Analyzers/MetadataAnalyzer.cs Outdated Show resolved Hide resolved

src/Altinn.App.Analyzers/MetadataAnalyzer.cs Outdated Show resolved Hide resolved

martinothamar force-pushed the feat/roslyn-analyzer-included branch from fbd306b to 4a882a4 Compare May 23, 2024 12:16

martinothamar changed the title ~~Roslyn analyzer setup included in Altinn.App.Core~~ Roslyn analyzer setup included in Altinn.App.Core to catch configuration errors based on static analysis May 23, 2024

github-advanced-security bot found potential problems May 24, 2024

View reviewed changes

martinothamar force-pushed the feat/roslyn-analyzer-included branch from ec604a7 to cd1ef91 Compare May 25, 2024 12:25

martinothamar force-pushed the feat/roslyn-analyzer-included branch 2 times, most recently from a2d11a4 to bfcd9f4 Compare May 27, 2024 21:13

martinothamar force-pushed the feat/roslyn-analyzer-included branch from 97b606f to b0a5c70 Compare May 28, 2024 21:39

github-advanced-security bot found potential problems May 28, 2024

View reviewed changes

martinothamar force-pushed the feat/roslyn-analyzer-included branch from b0a5c70 to b8e63bc Compare May 30, 2024 10:25

martinothamar added 10 commits June 11, 2024 19:11

Initial analyzer setup, with example diagnostics

3393138

Diagnostics to warnings, metadata category

2ce254d

Fix packaging, diagnostics

a18f380

TODO comment

966098e

Add location for diagnostic

779bcd1

Downgrade Roslyn version, include Newtonsoft.Json in nupkg

cfe674e

Attempt to cleanup csproj

572031d

Testing setup

cf1f080

Slight refactoring, adjust accessibility

8c477c9

Normalize slashes in diagnostic snapshots

339e42b

martinothamar added 4 commits June 11, 2024 19:11

csproj formatting thing, some comments

e444948

Add benchmark code

bc0a41a

Add 'OnEntry'

87ae016

Some refactoring, a couple more diagnostics, layout-sets analysis

6ccb000

martinothamar force-pushed the feat/roslyn-analyzer-included branch from b8e63bc to 6ccb000 Compare June 11, 2024 17:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Roslyn analyzer setup included in Altinn.App.Core to catch configuration errors based on static analysis #651

Roslyn analyzer setup included in Altinn.App.Core to catch configuration errors based on static analysis #651

martinothamar commented May 16, 2024 •

edited

Loading

ivarne left a comment

martinothamar commented May 21, 2024

ivarne commented May 21, 2024

martinothamar commented May 25, 2024

martinothamar commented May 25, 2024

martinothamar commented May 27, 2024

ivarne commented May 27, 2024

sonarqubecloud bot commented Jun 11, 2024

Roslyn analyzer setup included in Altinn.App.Core to catch configuration errors based on static analysis #651

Are you sure you want to change the base?

Roslyn analyzer setup included in Altinn.App.Core to catch configuration errors based on static analysis #651

Conversation

martinothamar commented May 16, 2024 • edited Loading

Description

Related Issue(s)

Verification

Documentation

ivarne left a comment

Choose a reason for hiding this comment

martinothamar commented May 21, 2024

ivarne commented May 21, 2024

martinothamar commented May 25, 2024

martinothamar commented May 25, 2024

martinothamar commented May 27, 2024

ivarne commented May 27, 2024

sonarqubecloud bot commented Jun 11, 2024

Quality Gate failed

martinothamar commented May 16, 2024 •

edited

Loading