Skip to content

Commit

Permalink
Converted list in Alias service. Fixes #2056
Browse files Browse the repository at this point in the history
  • Loading branch information
@tidyui
tidyui committed Apr 3, 2024
1 parent eab6a58 commit 3959378
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 1 deletion.
2 changes: 1 addition & 1 deletion core/Piranha/Services/Internal/AliasService.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ private async Task<IEnumerable<AliasUrlCacheEntry>> GetAliasUrls(Guid siteId)
{
Id = x.Id,
AliasUrl = x.AliasUrl
});
}).ToList();

_cache.Set($"Piranha_AliasUrls_{siteId}", aliasUrls);
}
Expand Down
2 changes: 2 additions & 0 deletions test/Piranha.Tests/Services/AliasTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,8 @@ public async Task Delete()

Assert.NotNull(model);

model = await api.Aliases.GetByAliasUrlAsync(ALIAS_4);

await api.Aliases.DeleteAsync(model);
}
}
Expand Down

5 comments on commit 3959378

@hedronn
Copy link

@hedronn hedronn commented on 3959378 Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Håkan sorry on closer inspection it looks like the exception is due to the _jsonSettings for Newtonsoft in the following method (DistributedCache: Get(stringkey).

If you see the output (Immediate Window below) from two calls one without the _jsonSettings and one with you can see the Deserialization fails with the _jsonSettings passed in the JsonConvert.DeserializeObject call (see below output examples)

Piranha.Cache.DistributedCache

/// <inheritdoc />
public T Get<T>(string key)
{
    var json = _cache.GetString(key);

    if (!string.IsNullOrEmpty(json))
    {
        return JsonConvert.DeserializeObject<T>(json, _jsonSettings);
    }
    return default(T);
}

[Immediate Window]

?JsonConvert.DeserializeObject(json);
Count = 3
[0]: {Piranha.Services.AliasService.AliasUrlCacheEntry}
[1]: {Piranha.Services.AliasService.AliasUrlCacheEntry}
[2]: {Piranha.Services.AliasService.AliasUrlCacheEntry}

?JsonConvert.DeserializeObject(json, _jsonSettings);
Exception thrown: 'Newtonsoft.Json.JsonSerializationException' in Newtonsoft.Json.dll
Exception thrown: 'Newtonsoft.Json.JsonSerializationException' in Newtonsoft.Json.dll

As you can see from above the first call without _jsonSettings returns the data successfully, but with _jsonSettings an exception is thrown in the Newtonsoft.Json.dll

@tidyui
Copy link
Member Author

@tidyui tidyui commented on 3959378 Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds weird since the test did confirm throwing the exception before the update, but the test passed fine after ToList() had been added to the code.

@hedronn
Copy link

@hedronn hedronn commented on 3959378 Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting...do you have a unit test for Piranha.Cache.DistributedCache class method public T Get(string key) where the key passed returns JSON as a collection of Piranha.Services.AliasService.AliasUrlCacheEntry types and does that test pass?

@tidyui
Copy link
Member Author

@tidyui tidyui commented on 3959378 Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test modified in the commit is run without cache, with memory cache and with distributed cache. When adding the additional api-call I verified the exception before the fix and then verified that it did not occur after the fix.

@hedronn
Copy link

@hedronn hedronn commented on 3959378 Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test modified in the commit is run without cache, with memory cache and with distributed cache. When adding the additional api-call I verified the exception before the fix and then verified that it did not occur after the fix.

Thanks Håkan sounds good. I will wait until the next Nuget package release, as I am not in an immediate rush to upgrade, so will check this again in a future update. Thanks again for all your help investigating and resolving this issue.

Please sign in to comment.