Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add As/Put method to ISite to allow caching #14372

Merged
merged 11 commits into from
Nov 24, 2023
Merged

Add As/Put method to ISite to allow caching #14372

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
41c8f0f
Add As/Put method to ISite to allow caching
MikeAlhayek Sep 19, 2023
1f83860
Find tests
MikeAlhayek Sep 19, 2023
55657d2
cleanup
MikeAlhayek Sep 19, 2023
9eb7a12
Update SiteSettings.cs
MikeAlhayek Sep 20, 2023
5141f39
Fix build
MikeAlhayek Sep 20, 2023
ee28846
Use ConcurrentDictionary instead of Dictionary
MikeAlhayek Oct 31, 2023
2e3ab6d
Merge remote-tracking branch 'origin/main' into ma/as-in-isite
jtkech Nov 20, 2023
5c804b5
Introduce Document IsMutable
jtkech Nov 20, 2023
76c9663
Merge remote-tracking branch 'origin/main' into ma/as-in-isite
jtkech Nov 23, 2023
0164938
IsReadOnly
jtkech Nov 23, 2023
31d0255
tweaks
jtkech Nov 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/OrchardCore.Modules/OrchardCore.Media/Processing/MediaTokenOptionsConfiguration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using Microsoft.Extensions.Options;
using OrchardCore.Entities;
using OrchardCore.Settings;

namespace OrchardCore.Media.Processing
Expand Down
62 changes: 62 additions & 0 deletions src/OrchardCore.Modules/OrchardCore.Settings/SiteSettings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,88 @@
using System.Collections.Generic;
using Microsoft.AspNetCore.Routing;
using Newtonsoft.Json.Linq;
using OrchardCore.Documents;
using OrchardCore.Entities;

namespace OrchardCore.Settings
{
// When updating class also update SiteSettingsDeploymentSource and SettingsStep.
public class SiteSettings : DocumentEntity, ISite
{
private JObject _properties = new();
Copy link
Member

Choose a reason for hiding this comment

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

no a simple dictionary?

Copy link
Member

Choose a reason for hiding this comment

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

The simple dictionary is below line 86

Here it is related to line 14 that defines a new Properties to override the Entity.Properties.


public new JObject Properties
{
get => _properties;
set
{
_properties = value ?? new JObject();
Copy link
Member

Choose a reason for hiding this comment

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

You could just not mutate _properties if the value is null in place of creating a new JObject, and then maybe not clear the cache.

I didn't have the time to try it on my side but are the new Properties well used in both way, for example populated from an existing SiteSettings in the database?

Yes, I never took the time to manage an IsReadOnly flag for a Document, but just for info, when we load a Document to be updated we call somewhere .GetOrCreateMutableAsync(), but when we get a cached Document (for sharing) we call somewhere .GetOrCreateImmutableAsync() which returns a document that normally should not be mutated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a reason why to not set it to a new object? Yes there are many way to a a accomplish the same result. The base class sets it to a new Object and usually we use Put and As without having to worry if Properties is null. It is best to always initialize it.

Copy link
Member

Choose a reason for hiding this comment

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

No problem to init it to a new JObject, here I meant something like this

    private JObject _properties = new();

    public new JObject Properties
    {
        get => _properties;
        set
        {
            if (value is not null)
            {
                _properties = value;
            }

            _cache.Clear();
        }
    }

But here I think we could just use this as done in Entity.

public new JObject Properties { get; set; } = new();

Not even sure that here we need a setter, so maybe

public new JObject Properties { get; } = new();

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we have to use a setting since we are overriding a property that define a setter.

If we ignore Properties = null; the user will not be able to clear Properties. This will change the behavior. I think currently it's correct. Setting it to null will just initialize it to a new object that is ready to put/get settings.

Copy link
Member

@jtkech jtkech Sep 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Okay for the setter.

In my example we still clear the cache, Hmm, if the value is null you could do _properties.RemoveAll() instead.

I would need to focus on it a little more but,

If we Get an immutable Document (only one cached instance for sharing) it should not be mutated, and if we Load a mutable Document for updating (one instance per scope) normally it should follow the current pattern to only update their related part in Properties.

So not sure we need to be able to clear this cache and then to override Properties.

Copy link
Member

Choose a reason for hiding this comment

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

So, also not sure that we need a Put at this level, hmm unless if some updaters need to read a property while others update it, the entries would need to be cleared.

I talk about making a Document IsReadOnly or not, not so hard to do but at least we could do it for SiteSettings, meaning that in GetSiteSettingsAsync() the Document would be marked as IsReadOnly (internal and not serializable bool), here the cache would operate, and in LoadSiteSettingsAsync() we would set IsReadOnly to false, here the cache would not operate.

Copy link
Member

Choose a reason for hiding this comment

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

I may introduce the support of this flag in the DocumentManager itself when I will have time.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jtkech do we need to wait until adding support for the DocumentManager? I don't think we should block this PR for this. I think the PR as is provides a valid value. Maybe we take this one and if you add IsReadOnly support we can modify what is needed then.

_cache.Clear();
}
}

public string BaseUrl { get; set; }

public string Calendar { get; set; }

public int MaxPagedCount { get; set; }

public int MaxPageSize { get; set; }

public int PageSize { get; set; }

public string TimeZoneId { get; set; }

public ResourceDebugMode ResourceDebugMode { get; set; }

public string SiteName { get; set; }

public string SiteSalt { get; set; }

public string PageTitleFormat { get; set; }

public string SuperUser { get; set; }

public bool UseCdn { get; set; }

public string CdnBaseUrl { get; set; }

public RouteValueDictionary HomeRoute { get; set; } = new RouteValueDictionary();

public bool AppendVersion { get; set; } = true;

public CacheMode CacheMode { get; set; }

public T As<T>() where T : new()
{
var name = typeof(T).Name;

if (_cache.TryGetValue(name, out var obj) && obj is T value)
{
return value;
}

var settings = this.As<T>(name);
MikeAlhayek marked this conversation as resolved.
Show resolved Hide resolved

_cache[name] = settings;

return settings;
}

public ISite Put<T>(T settings) where T : new()
{
var name = typeof(T).Name;

if (settings is not null)
{
this.Put(name, settings);
}

_cache.Remove(name);

return this;
}

private readonly Dictionary<string, object> _cache = new();
}
}
58 changes: 38 additions & 20 deletions src/OrchardCore/OrchardCore.Infrastructure.Abstractions/Settings/ISite.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,43 @@
using Microsoft.AspNetCore.Routing;
using OrchardCore.Entities;

namespace OrchardCore.Settings
namespace OrchardCore.Settings;

public interface ISite : IEntity
{
public interface ISite : IEntity
{
string SiteName { get; set; }
string PageTitleFormat { get; set; }
string SiteSalt { get; set; }
string SuperUser { get; set; }
string Calendar { get; set; }
string TimeZoneId { get; set; }
ResourceDebugMode ResourceDebugMode { get; set; }
bool UseCdn { get; set; }
string CdnBaseUrl { get; set; }
int PageSize { get; set; }
int MaxPageSize { get; set; }
int MaxPagedCount { get; set; }
string BaseUrl { get; set; }
RouteValueDictionary HomeRoute { get; set; }
bool AppendVersion { get; set; }
CacheMode CacheMode { get; set; }
}
string SiteName { get; set; }

string PageTitleFormat { get; set; }

string SiteSalt { get; set; }

string SuperUser { get; set; }

string Calendar { get; set; }

string TimeZoneId { get; set; }

ResourceDebugMode ResourceDebugMode { get; set; }

bool UseCdn { get; set; }

string CdnBaseUrl { get; set; }

int PageSize { get; set; }

int MaxPageSize { get; set; }

int MaxPagedCount { get; set; }

string BaseUrl { get; set; }

RouteValueDictionary HomeRoute { get; set; }

bool AppendVersion { get; set; }

CacheMode CacheMode { get; set; }

T As<T>() where T : new();

ISite Put<T>(T aspect) where T : new();
}
11 changes: 5 additions & 6 deletions test/OrchardCore.Tests/Modules/OrchardCore.Media/MediaTokenTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using OrchardCore.Media;
using OrchardCore.Media.Processing;
using OrchardCore.Settings;
using OrchardCore.Tests.Utilities;
using SixLabors.ImageSharp.Web.Processors;

namespace OrchardCore.Tests.Modules.OrchardCore.Media
Expand Down Expand Up @@ -71,15 +72,13 @@ private IServiceProvider CreateServiceProvider()
{
var services = new ServiceCollection();

var mockSiteService = Mock.Of<ISiteService>(ss =>
ss.GetSiteSettingsAsync() == Task.FromResult(
Mock.Of<ISite>(s => s.Properties == JObject.FromObject(new { MediaTokenSettings = _mediaTokenSettings }))
)
);
var mockSite = SiteMockHelper.GetSite(_mediaTokenSettings);

var mockSiteService = Mock.Of<ISiteService>(ss => ss.GetSiteSettingsAsync() == Task.FromResult(mockSite.Object));

services.AddMemoryCache();

services.AddSingleton<ISiteService>(mockSiteService);
services.AddSingleton(mockSiteService);
services.AddSingleton<IImageWebProcessor, TokenCommandProcessor>();
services.AddSingleton<IImageWebProcessor, TokenCommandProcessor>();
services.AddSingleton<IImageWebProcessor, ResizeWebProcessor>();
Expand Down
10 changes: 4 additions & 6 deletions test/OrchardCore.Tests/OrchardCore.Users/RegistrationControllerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using OrchardCore.DisplayManagement.Notify;
using OrchardCore.Email;
using OrchardCore.Settings;
using OrchardCore.Tests.Utilities;
using OrchardCore.Users;
using OrchardCore.Users.Controllers;
using OrchardCore.Users.Events;
Expand Down Expand Up @@ -133,15 +134,12 @@ private static RegistrationController SetupRegistrationController(RegistrationSe
.Returns<string>(e =>
{
var user = users.SingleOrDefault(u => (u as User).Email == e);

return Task.FromResult(user);
});

var mockSiteService = Mock.Of<ISiteService>(ss =>
ss.GetSiteSettingsAsync() == Task.FromResult(
Mock.Of<ISite>(s => s.Properties == JObject.FromObject(new { RegistrationSettings = registrationSettings }))
)
);
var mockSite = SiteMockHelper.GetSite(registrationSettings);

var mockSiteService = Mock.Of<ISiteService>(ss => ss.GetSiteSettingsAsync() == Task.FromResult(mockSite.Object));
var mockSmtpService = Mock.Of<ISmtpService>(x => x.SendAsync(It.IsAny<MailMessage>()) == Task.FromResult(SmtpResult.Success));
var mockStringLocalizer = new Mock<IStringLocalizer<RegistrationController>>();
mockStringLocalizer.Setup(l => l[It.IsAny<string>()])
Expand Down
23 changes: 23 additions & 0 deletions test/OrchardCore.Tests/Utilities/SiteMockHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
using OrchardCore.Settings;

namespace OrchardCore.Tests.Utilities;

public class SiteMockHelper
{
public static Mock<ISite> GetSite<T>(T obj) where T : new()
{
var properties = new JObject
{
[obj.GetType().Name] = JObject.FromObject(obj)
};

var mockSite = new Mock<ISite>();
mockSite.Setup(x => x.Properties)
.Returns(properties);

mockSite.Setup(x => x.As<T>())
.Returns(obj);

return mockSite;
}
}