Skip to content

Commit 6ed2bb2

Browse files
Copilotadamsitnik
andauthored
[MEVD] Fix VectorStoreKeyAttribute.IsAutoGenerated to be usable as a compile-time attribute argument (#13698)
### Motivation and Context `VectorStoreKeyAttribute.IsAutoGenerated` was typed as `bool?`, which C# does not permit as a compile-time attribute argument. This made the following valid-looking code fail to compile: ```csharp [VectorStoreKey(StorageName = "name", IsAutoGenerated = true)] public Guid Key { get; set; } ``` ### Description - **`VectorStoreKeyAttribute.IsAutoGenerated`**: Changed from `public bool?` to a `public bool` property. Its setter stores the value into a new `internal bool? IsAutoGeneratedNullable` backing property. The getter returns `IsAutoGeneratedNullable.GetValueOrDefault()` rather than throwing; an inline comment explains this is a workaround for the C# compiler restriction that prevents `Nullable<bool>` from being used as a compile-time attribute argument. The `bool?` backing preserves the "not explicitly set" vs. "explicitly set" distinction the framework relies on. - **`CollectionModelBuilder`**: Updated to read `keyAttribute.IsAutoGeneratedNullable` (the `bool?` backing) instead of the former `bool?` public property, keeping the existing `?? SupportsKeyAutoGeneration(...)` fallback behaviour intact. - **`KernelFunctionFromMethod`**: Fixed a pre-existing operator precedence bug in a pattern match expression (`value is not JsonElement or JsonDocument or JsonNode`) that caused CS9336 redundant-pattern errors on the .NET 10 compiler. Added parentheses to correctly express the intended negation of all three types: `value is not (JsonElement or JsonDocument or JsonNode)`. - **`CollectionModelBuilderTests`**: Added four new unit tests covering all `IsAutoGenerated` attribute scenarios: explicitly set to `true`, explicitly set to `false`, omitted on a `Guid` key (fallback to `SupportsKeyAutoGeneration` returns `true`), and omitted on an `int` key (fallback returns `false`). ```csharp // Now compiles and works correctly [VectorStoreKey(IsAutoGenerated = true)] public Guid Key { get; set; } [VectorStoreKey(IsAutoGenerated = false)] public Guid Key { get; set; } [VectorStoreKey] // IsAutoGeneratedNullable remains null, fallback to SupportsKeyAutoGeneration() public Guid Key { get; set; } ``` ### Contribution Checklist - [x] The code builds clean without any errors or warnings - [ ] The PR follows the [SK Contribution Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md) and the [pre-submission formatting script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts) raises no violations - [x] All unit tests pass, and I have added new tests where possible - [ ] I didn't break anyone 😄 <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>[MEVD] Can't set IsAutoGenerated property of VectorStoreKeyAttribute at compile time</issue_title> > <issue_description>The following code fails to compile: > > ```csharp > [VectorStoreKey(StorageName = "name", IsAutoGenerated = true)] > public Guid Key { get; set; } > ``` > > Because the `IsAutoGenerated` is `bool?`: > > https://github.com/microsoft/semantic-kernel/blob/082e28e52ec2b45ad8cf287176d844150617e8b5/dotnet/src/VectorData/VectorData.Abstractions/RecordAttributes/VectorStoreKeyAttribute.cs#L30 > > Which in C# maps to `System.Nullable<bool>` (a struct) and can't be set at compile time. > > Could we make it just a `bool` like what it's mapped to at the end? > > https://github.com/microsoft/semantic-kernel/blob/082e28e52ec2b45ad8cf287176d844150617e8b5/dotnet/src/VectorData/VectorData.Abstractions/ProviderServices/KeyPropertyModel.cs#L18 > > Context: https://github.com/dotnet/extensions/pull/7423/changes#r2974567816 > > cc @roji </issue_description> > > <agent_instructions>1. Make [this](https://github.com/microsoft/semantic-kernel/blob/082e28e52ec2b45ad8cf287176d844150617e8b5/dotnet/src/VectorData/VectorData.Abstractions/RecordAttributes/VectorStoreKeyAttribute.cs#L30) property be a `bool` rather than `bool?` property. > 2. Implement the getter and setter for it. > 3. The setter should set an internal `bool?` property that should be used instead of the public property everywhere in the repo. [Example]https://github.com/microsoft/semantic-kernel/blob/082e28e52ec2b45ad8cf287176d844150617e8b5/dotnet/src/VectorData/VectorData.Abstractions/ProviderServices/CollectionModelBuilder.cs#L162) > 3. Proof of concept code that I've used to verify it's going to get us what we need: > > ```csharp > using System.Reflection; > > namespace ConsoleApp15 > { > public class Program > { > static void Main() > { > TestAttribute setToTrue = typeof(Program).GetMethod(nameof(SetToTrue))!.GetCustomAttribute<TestAttribute>()!; > Console.WriteLine($"IsAutoGenerated_Core for [Test(IsAutoGenerated = true)]: {setToTrue.IsAutoGenerated_Core}"); > > TestAttribute setToFalse = typeof(Program).GetMethod(nameof(SetToFalse))!.GetCustomAttribute<TestAttribute>()!; > Console.WriteLine($"IsAutoGenerated_Core for [Test(IsAutoGenerated = false)]: {setToFalse.IsAutoGenerated_Core}"); > > TestAttribute notSet = typeof(Program).GetMethod(nameof(NotSet))!.GetCustomAttribute<TestAttribute>()!; > Console.WriteLine($"IsAutoGenerated_Core for [Test]: {notSet.IsAutoGenerated_Core}"); > } > > [Test(IsAutoGenerated = true)] > public static void SetToTrue() { } > > [Test(IsAutoGenerated = false)] > public static void SetToFalse() { } > > [Test] > public static void NotSet() { } > } > > [AttributeUsage(AttributeTargets.All)] > public class TestAttribute : Attribute > { > public bool IsAutoGenerated > { > get => throw new InvalidOperationException(); // provided to satisfy compiler requirements > set => IsAutoGenerated_Core = value; > } > > internal bool? IsAutoGenerated_Core { get; set; } > } > } > ```</agent_instructions> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > </comments> > </details> <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes #13697 <!-- START COPILOT CODING AGENT TIPS --> --- 💬 Send tasks to Copilot coding agent from [Slack](https://gh.io/cca-slack-docs) and [Teams](https://gh.io/cca-teams-docs) to turn conversations into code. Copilot posts an update in your thread when it's finished. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
1 parent 082e28e commit 6ed2bb2

4 files changed

Lines changed: 103 additions & 3 deletions

File tree

dotnet/src/SemanticKernel.Core/Functions/KernelFunctionFromMethod.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -720,7 +720,7 @@ private static (Func<KernelFunction, Kernel, KernelArguments, CancellationToken,
720720
return value;
721721
}
722722

723-
if (converter is not null && value is not JsonElement or JsonDocument or JsonNode)
723+
if (converter is not null && value is not (JsonElement or JsonDocument or JsonNode))
724724
{
725725
try
726726
{

dotnet/src/VectorData/VectorData.Abstractions/ProviderServices/CollectionModelBuilder.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ protected virtual void ProcessTypeProperties(Type type, VectorStoreCollectionDef
159159
if (clrProperty.GetCustomAttribute<VectorStoreKeyAttribute>() is { } keyAttribute)
160160
{
161161
var keyProperty = new KeyPropertyModel(clrProperty.Name, clrProperty.PropertyType);
162-
keyProperty.IsAutoGenerated = keyAttribute.IsAutoGenerated ?? this.SupportsKeyAutoGeneration(keyProperty.Type);
162+
keyProperty.IsAutoGenerated = keyAttribute.IsAutoGeneratedNullable ?? this.SupportsKeyAutoGeneration(keyProperty.Type);
163163
this.KeyProperties.Add(keyProperty);
164164
storageName = keyAttribute.StorageName;
165165
property = keyProperty;

dotnet/src/VectorData/VectorData.Abstractions/RecordAttributes/VectorStoreKeyAttribute.cs

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,21 @@ public sealed class VectorStoreKeyAttribute : Attribute
2626
/// </summary>
2727
/// <remarks>
2828
/// The availability of auto-generated properties - as well as the .NET types supported for them - varies across provider implementations.
29+
/// The getter returns <see langword="false" /> when the value has not been explicitly set; use <see cref="IsAutoGeneratedNullable" /> to distinguish
30+
/// between "explicitly set to false" and "not set".
31+
/// The getter does not throw, even though it cannot distinguish "explicitly set to false" and "not set"; this is a workaround for a C# compiler
32+
/// limitation that does not allow <see cref="Nullable{T}"/> to be used as an attribute argument.
2933
/// </remarks>
30-
public bool? IsAutoGenerated { get; set; }
34+
public bool IsAutoGenerated
35+
{
36+
// The getter returns GetValueOrDefault() rather than throwing, as a workaround for a C# compiler limitation:
37+
// Nullable<bool> cannot be used as a compile-time attribute argument, so the public property must be bool.
38+
get => this.IsAutoGeneratedNullable.GetValueOrDefault();
39+
set => this.IsAutoGeneratedNullable = value;
40+
}
41+
42+
/// <summary>
43+
/// Gets whether this key property's value is auto-generated or not, or <see langword="null" /> if not set.
44+
/// </summary>
45+
internal bool? IsAutoGeneratedNullable { get; private set; }
3146
}

dotnet/test/VectorData/VectorData.UnitTests/CollectionModelBuilderTests.cs

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -423,6 +423,91 @@ internal static bool IsVectorPropertyTypeValidCore(Type type, [NotNullWhen(false
423423
];
424424
}
425425

426+
[Fact]
427+
public void IsAutoGenerated_attribute_true_overrides_default()
428+
{
429+
// Guid key with explicit IsAutoGenerated = true; the attribute should override SupportsKeyAutoGeneration.
430+
var model = new GuidKeyModelBuilder().Build(
431+
typeof(RecordWithGuidKeyAutoGeneratedTrue), typeof(Guid), definition: null, defaultEmbeddingGenerator: null);
432+
433+
Assert.True(model.KeyProperty.IsAutoGenerated);
434+
}
435+
436+
[Fact]
437+
public void IsAutoGenerated_attribute_false_overrides_default()
438+
{
439+
// Guid key with explicit IsAutoGenerated = false; the attribute should override SupportsKeyAutoGeneration,
440+
// which would otherwise return true for Guid.
441+
var model = new GuidKeyModelBuilder().Build(
442+
typeof(RecordWithGuidKeyAutoGeneratedFalse), typeof(Guid), definition: null, defaultEmbeddingGenerator: null);
443+
444+
Assert.False(model.KeyProperty.IsAutoGenerated);
445+
}
446+
447+
[Fact]
448+
public void IsAutoGenerated_omitted_falls_back_to_SupportsKeyAutoGeneration_true()
449+
{
450+
// Guid key with no IsAutoGenerated attribute set; should fall back to SupportsKeyAutoGeneration, which returns true for Guid.
451+
var model = new GuidKeyModelBuilder().Build(
452+
typeof(RecordWithGuidKeyNoIsAutoGenerated), typeof(Guid), definition: null, defaultEmbeddingGenerator: null);
453+
454+
Assert.True(model.KeyProperty.IsAutoGenerated);
455+
}
456+
457+
[Fact]
458+
public void IsAutoGenerated_omitted_falls_back_to_SupportsKeyAutoGeneration_false()
459+
{
460+
// int key with no IsAutoGenerated attribute set; should fall back to SupportsKeyAutoGeneration, which returns false for non-Guid.
461+
var model = new GuidKeyModelBuilder().Build(
462+
typeof(RecordWithIntKeyNoIsAutoGenerated), typeof(int), definition: null, defaultEmbeddingGenerator: null);
463+
464+
Assert.False(model.KeyProperty.IsAutoGenerated);
465+
}
466+
467+
public class RecordWithGuidKeyAutoGeneratedTrue
468+
{
469+
[VectorStoreKey(IsAutoGenerated = true)]
470+
public Guid Id { get; set; }
471+
}
472+
473+
public class RecordWithGuidKeyAutoGeneratedFalse
474+
{
475+
[VectorStoreKey(IsAutoGenerated = false)]
476+
public Guid Id { get; set; }
477+
}
478+
479+
public class RecordWithGuidKeyNoIsAutoGenerated
480+
{
481+
[VectorStoreKey]
482+
public Guid Id { get; set; }
483+
}
484+
485+
public class RecordWithIntKeyNoIsAutoGenerated
486+
{
487+
[VectorStoreKey]
488+
public int Id { get; set; }
489+
}
490+
491+
private sealed class GuidKeyModelBuilder()
492+
: CollectionModelBuilder(new CollectionModelBuildingOptions
493+
{
494+
SupportsMultipleVectors = true,
495+
RequiresAtLeastOneVector = false
496+
})
497+
{
498+
protected override bool IsDataPropertyTypeValid(Type type, [NotNullWhen(false)] out string? supportedTypes)
499+
{
500+
supportedTypes = null;
501+
return true;
502+
}
503+
504+
protected override bool IsVectorPropertyTypeValid(Type type, [NotNullWhen(false)] out string? supportedTypes)
505+
{
506+
supportedTypes = null;
507+
return true;
508+
}
509+
}
510+
426511
private sealed class FakeEmbeddingGenerator<TInput, TEmbedding> : IEmbeddingGenerator<TInput, TEmbedding>
427512
where TEmbedding : Embedding
428513
{

0 commit comments

Comments
 (0)