Skip to content

Commit

Permalink
Properly persist computed colums for same type
Browse files Browse the repository at this point in the history
We had a bug where we cached the list of properties to persist by type, without regard for the variability in PK/RK names which can be different when persisting the same entity using various key strategies for indexing purposes.

By considering these two properties as part of the cache key, we allow proper persistence as originally intended to avoid duplicating PK/RK props.

Fixes #252
  • Loading branch information
kzu committed Oct 4, 2023
1 parent 4c7537a commit 6ecef07
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 2 deletions.
4 changes: 2 additions & 2 deletions src/TableStorage/EntityPropertiesMapper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ namespace Devlooped
{
class EntityPropertiesMapper
{
static readonly ConcurrentDictionary<Type, PropertyInfo[]> propertiesMap = new();
static readonly ConcurrentDictionary<(Type, string), PropertyInfo[]> propertiesMap = new();
static readonly ConcurrentDictionary<Type, IDictionary<string, string>> edmAnnotations = new();
static readonly KeyValuePair<string, string> edmNone = new("", "");

Expand All @@ -22,7 +22,7 @@ private EntityPropertiesMapper() { }

public IDictionary<string, object> ToProperties<T>(T entity, string? partitionKeyProperty = default, string? rowKeyProperty = default) where T: notnull
{
var properties = propertiesMap.GetOrAdd(entity.GetType(), type => type
var properties = propertiesMap.GetOrAdd((entity.GetType(), $"{partitionKeyProperty}|{rowKeyProperty}"), key => key.Item1
.GetProperties()
.Where(prop =>
prop.GetCustomAttribute<BrowsableAttribute>()?.Browsable != false &&
Expand Down
40 changes: 40 additions & 0 deletions src/Tests/RepositoryTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,46 @@ public async Task CanGetFromEdmEntityTypes()
Assert.IsType<long>(generic["Count"]);
}

[Fact]
public async Task CanPersistPropertiesFromComputedRowKeys()
{
var storage = CloudStorageAccount.DevelopmentStorageAccount;
var repo1 = TableRepository.Create<Dependency>(
storage,
tableName: "Dependency",
partitionKey: d => d.Repository,
rowKey: d => $"{d.Name}|{d.Version}");

var repo2 = TableRepository.Create<Dependency>(
storage,
tableName: "Dependency",
partitionKey: d => d.Name,
rowKey: d => $"{d.Version}|{d.Repository}");

var dep = new Dependency("org", "repo", "npm", "foo", "1.0");

await repo1.PutAsync(dep);
await repo2.PutAsync(dep);

var entities = TableRepository.Create(storage, "Dependency");

var entity = await entities.GetAsync("repo", "foo|1.0");
Assert.NotNull(entity);
// Since the PK is the Repository property, it's not persisted as a property.
Assert.Null(entity[nameof(Dependency.Repository)]);
// But the name is, since it's a computed row key.
Assert.Equal("foo", entity[nameof(Dependency.Name)]);

entity = await entities.GetAsync("foo", "1.0|repo");
Assert.NotNull(entity);
// Conversely, since the PK here is the Name property, the Repository should be persisted.
Assert.Equal("repo", entity[nameof(Dependency.Repository)]);
// And the Name shouldn't, since it's the PK
Assert.Null(entity[nameof(Dependency.Name)]);
}

record Dependency(string Organization, string Repository, string Type, string Name, string Version);

class MyEntity
{
public MyEntity(string id) => Id = id;
Expand Down

0 comments on commit 6ecef07

Please sign in to comment.