Skip to content

Commit a124764

Browse files
committed
Report read-only collection properties with NCDI002 (#87)
A get-only collection property (ICollection<T>) binds at startup but cannot be hot reloaded safely in place, so the generator now reports it with the specific NCDI002 diagnostic recommending a settable property on the class and a get-only property on the interface. Two code fixes are offered: add a setter, or apply [SkipConfigProperty]. README documents the pattern and clarifies the limitation only affects raw injected types (not IOptionsSnapshot/IOptionsMonitor).
1 parent ac0c3d6 commit a124764

9 files changed

Lines changed: 747 additions & 9 deletions

File tree

Lines changed: 227 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,227 @@
1+
namespace Neovolve.Configuration.DependencyInjection.CodeFixes.UnitTests
2+
{
3+
using System.Collections.Generic;
4+
using System.Linq;
5+
using System.Threading;
6+
using System.Threading.Tasks;
7+
using FluentAssertions;
8+
using Microsoft.CodeAnalysis;
9+
using Microsoft.CodeAnalysis.CodeActions;
10+
using Microsoft.CodeAnalysis.CodeFixes;
11+
using Microsoft.CodeAnalysis.CSharp;
12+
using Microsoft.CodeAnalysis.CSharp.Syntax;
13+
using Xunit;
14+
15+
public class ReadOnlyCollectionCodeFixProviderTests
16+
{
17+
private const string AddSetterTitle = "Add a setter to support hot reload";
18+
19+
private const string SkipPropertyTitle = "Exclude the property from hot reload with [SkipConfigProperty]";
20+
21+
private static readonly DiagnosticDescriptor _descriptor = new(
22+
"NCDI002",
23+
"Read-only collection configuration property cannot be hot reloaded",
24+
"Configuration property '{0}' is a read-only collection",
25+
"Neovolve.Configuration",
26+
DiagnosticSeverity.Warning,
27+
isEnabledByDefault: true);
28+
29+
[Fact]
30+
public async Task OffersBothFixesForGetOnlyCollectionProperty()
31+
{
32+
const string source = @"
33+
namespace Sample
34+
{
35+
using System.Collections.Generic;
36+
using System.Collections.ObjectModel;
37+
38+
public sealed class RequiredDataConfig
39+
{
40+
public ICollection<string> RequiredData { get; } = new Collection<string>();
41+
}
42+
}";
43+
44+
var actions = await GetActions(source, "RequiredData");
45+
46+
actions.Select(action => action.Title).Should().Contain(new[] { AddSetterTitle, SkipPropertyTitle });
47+
}
48+
49+
[Fact]
50+
public async Task AddsSetterToGetOnlyProperty()
51+
{
52+
const string source = @"
53+
namespace Sample
54+
{
55+
using System.Collections.Generic;
56+
using System.Collections.ObjectModel;
57+
58+
public sealed class RequiredDataConfig
59+
{
60+
public ICollection<string> RequiredData { get; } = new Collection<string>();
61+
}
62+
}";
63+
64+
var fixedText = await ApplyFix(source, "RequiredData", AddSetterTitle);
65+
66+
fixedText.Should().Contain("public ICollection<string> RequiredData { get; set; }");
67+
}
68+
69+
[Fact]
70+
public async Task PreservesInitializerWhenAddingSetter()
71+
{
72+
const string source = @"
73+
namespace Sample
74+
{
75+
using System.Collections.Generic;
76+
using System.Collections.ObjectModel;
77+
78+
public sealed class RequiredDataConfig
79+
{
80+
public ICollection<string> RequiredData { get; } = new Collection<string>();
81+
}
82+
}";
83+
84+
var fixedText = await ApplyFix(source, "RequiredData", AddSetterTitle);
85+
86+
fixedText.Should().Contain("= new Collection<string>();");
87+
}
88+
89+
[Fact]
90+
public async Task AppliesSkipConfigPropertyAttribute()
91+
{
92+
const string source = @"
93+
namespace Sample
94+
{
95+
using System.Collections.Generic;
96+
using System.Collections.ObjectModel;
97+
98+
public sealed class RequiredDataConfig
99+
{
100+
public ICollection<string> RequiredData { get; } = new Collection<string>();
101+
}
102+
}";
103+
104+
var fixedText = await ApplyFix(source, "RequiredData", SkipPropertyTitle);
105+
106+
fixedText.Should().Contain("[SkipConfigProperty]");
107+
fixedText.Should().Contain("using Neovolve.Configuration.DependencyInjection.Generated;");
108+
fixedText.Should().Contain("public ICollection<string> RequiredData { get; } = new Collection<string>();");
109+
}
110+
111+
[Fact]
112+
public async Task AppliesSkipConfigPropertyAttributeToExpressionBodiedProperty()
113+
{
114+
const string source = @"
115+
namespace Sample
116+
{
117+
using System.Collections.Generic;
118+
119+
public sealed class RequiredDataConfig
120+
{
121+
private readonly ICollection<string> _data = new List<string>();
122+
public ICollection<string> RequiredData => _data;
123+
}
124+
}";
125+
126+
var fixedText = await ApplyFix(source, "RequiredData", SkipPropertyTitle);
127+
128+
fixedText.Should().Contain("[SkipConfigProperty]");
129+
}
130+
131+
[Fact]
132+
public async Task DoesNotOfferAddSetterWhenSetterAlreadyPresent()
133+
{
134+
const string source = @"
135+
namespace Sample
136+
{
137+
using System.Collections.Generic;
138+
139+
public sealed class RequiredDataConfig
140+
{
141+
public ICollection<string> RequiredData { get; set; } = new List<string>();
142+
}
143+
}";
144+
145+
var actions = await GetActions(source, "RequiredData");
146+
147+
actions.Select(action => action.Title).Should().NotContain(AddSetterTitle);
148+
}
149+
150+
[Fact]
151+
public async Task DoesNotOfferAddSetterForExpressionBodiedProperty()
152+
{
153+
const string source = @"
154+
namespace Sample
155+
{
156+
using System.Collections.Generic;
157+
158+
public sealed class RequiredDataConfig
159+
{
160+
private readonly ICollection<string> _data = new List<string>();
161+
public ICollection<string> RequiredData => _data;
162+
}
163+
}";
164+
165+
var actions = await GetActions(source, "RequiredData");
166+
167+
actions.Select(action => action.Title).Should().NotContain(AddSetterTitle);
168+
}
169+
170+
private static async Task<IReadOnlyList<CodeAction>> GetActions(string source, string propertyName)
171+
{
172+
var workspace = new AdhocWorkspace();
173+
174+
var project = workspace.CurrentSolution
175+
.AddProject("Test", "Test", LanguageNames.CSharp)
176+
.WithCompilationOptions(
177+
new CSharpCompilationOptions(
178+
OutputKind.DynamicallyLinkedLibrary,
179+
nullableContextOptions: NullableContextOptions.Enable))
180+
.WithParseOptions(new CSharpParseOptions(LanguageVersion.Latest));
181+
182+
var document = project.AddDocument("Test.cs", source);
183+
184+
var root = await document.GetSyntaxRootAsync(CancellationToken.None);
185+
root.Should().NotBeNull();
186+
187+
var declaration = root!.DescendantNodes()
188+
.OfType<PropertyDeclarationSyntax>()
189+
.Single(node => node.Identifier.ValueText == propertyName);
190+
191+
var diagnostic = Diagnostic.Create(_descriptor, Location.Create(root.SyntaxTree, declaration.Span),
192+
propertyName);
193+
194+
var provider = new ReadOnlyCollectionCodeFixProvider();
195+
var actions = new List<CodeAction>();
196+
197+
var context = new CodeFixContext(
198+
document,
199+
diagnostic,
200+
(action, _) => actions.Add(action),
201+
CancellationToken.None);
202+
203+
await provider.RegisterCodeFixesAsync(context);
204+
205+
return actions;
206+
}
207+
208+
private static async Task<string> ApplyFix(string source, string propertyName, string title)
209+
{
210+
var actions = await GetActions(source, propertyName);
211+
212+
var action = actions.Single(candidate => candidate.Title == title);
213+
214+
var operations = await action.GetOperationsAsync(CancellationToken.None);
215+
var applyChanges = operations.OfType<ApplyChangesOperation>().Single();
216+
217+
var changedDocument = applyChanges.ChangedSolution.Projects
218+
.Single()
219+
.Documents
220+
.Single();
221+
222+
var text = await changedDocument.GetTextAsync();
223+
224+
return text.ToString();
225+
}
226+
}
227+
}

0 commit comments

Comments
 (0)