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.

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

Revert driver initialization logic and adapt Notifications cache #16677

Merged
merged 1 commit into from
Sep 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -38,20 +38,22 @@ public override async Task<IDisplayResult> DisplayAsync(Navbar model, BuildDispl
return null;
}

var result = Initialize<UserNotificationNavbarViewModel>("UserNotificationNavbar", async model =>
{
var userId = _httpContextAccessor.HttpContext.User.FindFirstValue(ClaimTypes.NameIdentifier);
var notifications = (await _session.Query<Notification, NotificationIndex>(x => x.UserId == userId && !x.IsRead, collection: NotificationConstants.NotificationCollection)
.OrderByDescending(x => x.CreatedAtUtc)
.Take(_notificationOptions.TotalUnreadNotifications + 1)
.ListAsync()).ToList();
var result = Initialize<UserNotificationNavbarViewModel>("UserNotificationNavbar")
.Processing<UserNotificationNavbarViewModel>(async model =>
Copy link
Member

Choose a reason for hiding this comment

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

@sebastienros if we can't get the Cache() to work on the on the ShapeResult, shouldn't we remove it then?

Also, since we are changing Initilize().Processing() should probably already take the same TModel instead of having to pass that again. It just seems weird to me to have to pass the type multiple times in the same chain.

Copy link
Member Author

Choose a reason for hiding this comment

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

if we can't get the Cache() to work on the on the ShapeResult, shouldn't we remove it then

This works

should probably already take the same TModel

I didn't want to introduce a breaking change, or more involved one, as it would require a new ShapeResult and other related changes. This change works.

Copy link
Member

Choose a reason for hiding this comment

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

So the caching work, but to avoid initializing on every request, we have to delegate the initialize logic to the Processing method. Correct? If so, I am not sure the API is intuitive enough. My first instinct would be to initialize as I would normally do in the call back.

It would been much better if we did not have to explain the use of Processing method in this case as it complicate the code.

{
var userId = _httpContextAccessor.HttpContext.User.FindFirstValue(ClaimTypes.NameIdentifier);
var notifications = (await _session.Query<Notification, NotificationIndex>(x => x.UserId == userId && !x.IsRead, collection: NotificationConstants.NotificationCollection)
.OrderByDescending(x => x.CreatedAtUtc)
.Take(_notificationOptions.TotalUnreadNotifications + 1)
.ListAsync()).ToList();

model.Notifications = notifications;
model.MaxVisibleNotifications = _notificationOptions.TotalUnreadNotifications;
model.TotalUnread = notifications.Count;
model.Notifications = notifications;
model.MaxVisibleNotifications = _notificationOptions.TotalUnreadNotifications;
model.TotalUnread = notifications.Count;

}).Location("Detail", "Content:9")
.Location("DetailAdmin", "Content:9");
})
.Location("Detail", "Content:9")
.Location("DetailAdmin", "Content:9");

if (_notificationOptions.AbsoluteCacheExpirationSeconds > 0 || _notificationOptions.SlidingCacheExpirationSeconds > 0)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,31 @@ public class DisplayDriverBase
protected string Prefix { get; set; } = string.Empty;

/// <summary>
/// Creates a new strongly typed shape and initializes it if it needs to be rendered.
/// Creates a new strongly typed shape.
/// </summary>
public ShapeResult Initialize<TModel>() where TModel : class
{
return Initialize<TModel>(shape => { });
}

/// <summary>
/// Creates a new strongly typed shape.
/// </summary>
public ShapeResult Initialize<TModel>(string shapeType) where TModel : class
{
return Initialize<TModel>(shapeType, shape => { });
}

/// <summary>
/// Creates a new strongly typed shape and initializes it before it is displayed.
/// </summary>
public ShapeResult Initialize<TModel>(Action<TModel> initialize) where TModel : class
{
return Initialize<TModel>(typeof(TModel).Name, initialize);
}

/// <summary>
/// Creates a new strongly typed shape and initializes it if it needs to be rendered.
/// Creates a new strongly typed shape and initializes it before it is displayed.
/// </summary>
public ShapeResult Initialize<TModel>(string shapeType, Action<TModel> initialize) where TModel : class
{
Expand All @@ -28,7 +44,7 @@ public ShapeResult Initialize<TModel>(string shapeType, Action<TModel> initializ
}

/// <summary>
/// Creates a new strongly typed shape and initializes it if it needs to be rendered.
/// Creates a new strongly typed shape and initializes it before it is displayed.
/// </summary>
public ShapeResult Initialize<TModel>(Func<TModel, ValueTask> initializeAsync) where TModel : class
{
Expand All @@ -39,7 +55,7 @@ public ShapeResult Initialize<TModel>(Func<TModel, ValueTask> initializeAsync) w
}

/// <summary>
/// Creates a new strongly typed shape and initializes it if it needs to be rendered.
/// Creates a new strongly typed shape and initializes it before it is displayed.
/// </summary>
public ShapeResult Initialize<TModel>(string shapeType, Func<TModel, ValueTask> initializeAsync) where TModel : class
{
Expand All @@ -59,7 +75,7 @@ public ShapeResult Copy<TModel>(string shapeType, TModel model) where TModel : c
}

/// <summary>
/// Creates a new loosely typed shape and initializes it if it needs to be rendered.
/// Creates a new loosely typed shape and initializes it before it is displayed.
/// </summary>
public ShapeResult Dynamic(string shapeType, Func<dynamic, Task> initializeAsync)
{
Expand All @@ -71,7 +87,7 @@ public ShapeResult Dynamic(string shapeType, Func<dynamic, Task> initializeAsync
}

/// <summary>
/// Creates a new loosely typed shape and initializes it if it needs to be rendered.
/// Creates a new loosely typed shape and initializes it before it is displayed.
/// </summary>
public ShapeResult Dynamic(string shapeType, Action<dynamic> initialize)
{
Expand All @@ -86,7 +102,7 @@ public ShapeResult Dynamic(string shapeType, Action<dynamic> initialize)
}

/// <summary>
/// If the shape needs to be rendered, it is created automatically from its type name.
/// When the shape is displayed, it is created automatically from its type name.
/// </summary>
public ShapeResult Dynamic(string shapeType)
{
Expand Down Expand Up @@ -126,7 +142,7 @@ public ShapeResult Factory(string shapeType, Func<IBuildShapeContext, IShape> sh
}

/// <summary>
/// If the shape needs to be rendered, it is created by the delegate.
/// If the shape needs to be displayed, it is created by the delegate.
/// </summary>
/// <remarks>
/// This method is ultimately called by all drivers to create a shape. It's made virtual
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,12 @@ public async Task<IHtmlContent> ExecuteAsync(DisplayContext context)
}

// Now find the actual binding to render, taking alternates into account.
var actualBinding = await GetShapeBindingAsync(shapeMetadata.Type, shapeMetadata.Alternates, shapeTable)
?? throw new Exception($"The shape type '{shapeMetadata.Type}' is not found");
var actualBinding = await GetShapeBindingAsync(shapeMetadata.Type, shapeMetadata.Alternates, shapeTable);

if (actualBinding == null)
{
throw new InvalidOperationException($"The shape type '{shapeMetadata.Type}' is not found for the theme '{theme?.Id}'");
}

await shapeMetadata.ProcessingAsync.InvokeAsync((action, displayContext) => action(displayContext.Shape), displayContext, _logger);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ public ShapeMetadata()
{
}

private List<Action<ShapeDisplayContext>> _displaying;
private List<Func<IShape, Task>> _processing;
private List<Action<ShapeDisplayContext>> _displayed;

public string Type { get; set; }
public string DisplayType { get; set; }
public string Position { get; set; }
Expand All @@ -28,40 +32,46 @@ public ShapeMetadata()
public bool IsCached => _cacheContext != null;
public IHtmlContent ChildContent { get; set; }

// The casts in (IReadOnlyList<T>)_displaying ?? [] are important as they convert [] to Array.Empty.
// It would use List<T> otherwise which is not what we want here, we don't want to allocate.

/// <summary>
/// Event use for a specific shape instance.
/// </summary>
[JsonIgnore]
public IReadOnlyList<Action<ShapeDisplayContext>> Displaying { get; private set; } = [];
public IReadOnlyList<Action<ShapeDisplayContext>> Displaying => (IReadOnlyList<Action<ShapeDisplayContext>>)_displaying ?? [];

/// <summary>
/// Event use for a specific shape instance.
/// </summary>
[JsonIgnore]
public IReadOnlyList<Func<IShape, Task>> ProcessingAsync { get; private set; } = [];
public IReadOnlyList<Func<IShape, Task>> ProcessingAsync => (IReadOnlyList<Func<IShape, Task>>)_processing ?? [];

/// <summary>
/// Event use for a specific shape instance.
/// </summary>
[JsonIgnore]
public IReadOnlyList<Action<ShapeDisplayContext>> Displayed { get; private set; } = [];
public IReadOnlyList<Action<ShapeDisplayContext>> Displayed => (IReadOnlyList<Action<ShapeDisplayContext>>)_displayed ?? [];

[JsonIgnore]
public IReadOnlyList<string> BindingSources { get; set; } = [];

public void OnDisplaying(Action<ShapeDisplayContext> context)
{
Displaying = [.. Displaying, context];
_displaying ??= new List<Action<ShapeDisplayContext>>();
_displaying.Add(context);
}

public void OnProcessing(Func<IShape, Task> context)
{
ProcessingAsync = [.. ProcessingAsync, context];
_processing ??= new List<Func<IShape, Task>>();
_processing.Add(context);
}

public void OnDisplayed(Action<ShapeDisplayContext> context)
{
Displayed = [.. Displayed, context];
_displayed ??= new List<Action<ShapeDisplayContext>>();
_displayed.Add(context);
}

/// <summary>
Expand Down
48 changes: 42 additions & 6 deletions src/OrchardCore/OrchardCore.DisplayManagement/Views/ShapeResult.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,25 +17,37 @@ public class ShapeResult : IDisplayResult
private string _cacheId;
private readonly string _shapeType;
private readonly Func<IBuildShapeContext, ValueTask<IShape>> _shapeBuilder;
private readonly Func<IShape, Task> _processing;
private readonly Func<IShape, Task> _initializing;
private Action<CacheContext> _cache;
private string _groupId;
private Action<ShapeDisplayContext> _displaying;
private Func<IShape, Task> _processing;
private Func<Task<bool>> _renderPredicateAsync;

/// <summary>
/// Creates a new instance of <see cref="ShapeResult"/>.
/// </summary>
/// <param name="shapeType">The Shape type used for the created Shape.</param>
/// <param name="shapeBuilder">A delegate that creates the shape instance.</param>
public ShapeResult(string shapeType, Func<IBuildShapeContext, ValueTask<IShape>> shapeBuilder)
: this(shapeType, shapeBuilder, null)
{
}

public ShapeResult(string shapeType, Func<IBuildShapeContext, ValueTask<IShape>> shapeBuilder, Func<IShape, Task> processing)
/// <summary>
/// Creates a new instance of <see cref="ShapeResult"/>.
/// </summary>
/// <param name="shapeType">The Shape type used for the created Shape.</param>
/// <param name="shapeBuilder">A delegate that creates the shape instance.</param>
/// <param name="initializing">A delegate that is executed after the shape is created.</param>
public ShapeResult(string shapeType, Func<IBuildShapeContext, ValueTask<IShape>> shapeBuilder, Func<IShape, Task> initializing)
{
// The shape type is necessary before the shape is created as it will drive the placement
// resolution which itself can prevent the shape from being created.

_shapeType = shapeType;
_shapeBuilder = shapeBuilder;
_processing = processing;
_initializing = initializing;
}

public Task ApplyAsync(BuildDisplayContext context)
Expand Down Expand Up @@ -118,13 +130,17 @@ private async Task ApplyImplementationAsync(BuildShapeContext context, string di
newShapeMetadata.Column = placement.GetColumn();
newShapeMetadata.Type = _shapeType;

// Invoke the initialization code first when all Displaying events are invoked.
// These Displaying methods are used to create alternates for instance, so the
// Shape needs to have required properties available first.

_initializing?.Invoke(Shape);

if (_displaying != null)
{
newShapeMetadata.OnDisplaying(_displaying);
}

// The _processing callback is used to delay execution of costly initialization
// that can be prevented by caching.
if (_processing != null)
{
newShapeMetadata.OnProcessing(_processing);
Expand Down Expand Up @@ -226,7 +242,7 @@ public ShapeResult Location(string displayType, string location)
}

/// <summary>
/// Sets the location to use for a matching display type.
/// Sets the delegate to be executed when the shape is being displayed.
/// </summary>
public ShapeResult Displaying(Action<ShapeDisplayContext> displaying)
{
Expand All @@ -235,6 +251,26 @@ public ShapeResult Displaying(Action<ShapeDisplayContext> displaying)
return this;
}

/// <summary>
/// Sets the delegate to be executed when the shape is rendered (not cached).
/// </summary>
public ShapeResult Processing(Func<IShape, Task> processing)
{
_processing = processing;

return this;
}

/// <summary>
/// Sets the delegate to be executed when the shape is rendered (not cached).
/// </summary>
public ShapeResult Processing<T>(Func<T, Task> processing)
{
_processing = shape => processing?.Invoke((T)shape);

return this;
}

/// <summary>
/// Sets the shape name regardless its 'Differentiator'.
/// </summary>
Expand Down
18 changes: 13 additions & 5 deletions test/OrchardCore.Tests/DisplayManagement/DynamicCacheTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ public async Task ShapeResultsAreRenderedOnceWhenCached()
var cacheTag = "mytag";

var initializedCalled = 0;
var processedCalled = 0;
var bindCalled = 0;

var displayManager = _serviceProvider.GetService<IHtmlDisplay>();
Expand All @@ -165,11 +166,16 @@ public async Task ShapeResultsAreRenderedOnceWhenCached()
ShapeResult CreateShapeResult() => new ShapeResult(
shapeType,
shapeBuilder: ctx => factory.CreateAsync<MyModel>(shapeType, model => model.MyProperty = 7),
processing: shape =>
initializing: shape =>
{
initializedCalled++;
return Task.CompletedTask;
}).Location("Content").Cache("mycontent", ctx => ctx.WithExpiryAfter(TimeSpan.FromSeconds(1)).AddTag(cacheTag));
}).Location("Content").Cache("mycontent", ctx => ctx.WithExpiryAfter(TimeSpan.FromSeconds(1)).AddTag(cacheTag))
.Processing(shape =>
{
processedCalled++;
return Task.CompletedTask;
});

var shapeResult = CreateShapeResult();
var contentShape = await factory.CreateAsync("Content");
Expand All @@ -186,7 +192,7 @@ public async Task ShapeResultsAreRenderedOnceWhenCached()
Assert.Equal(1, bindCalled);
Assert.Equal(1, initializedCalled);

for (var i = 0; i < 10; i++)
for (var i = 1; i <= 10; i++)
{
// Create new ShapeResult.
shapeResult = CreateShapeResult();
Expand All @@ -197,7 +203,8 @@ public async Task ShapeResultsAreRenderedOnceWhenCached()

// Shape is not rendered twice.
Assert.Equal(1, bindCalled);
Assert.Equal(1, initializedCalled);
Assert.Equal(1, processedCalled);
Assert.Equal(i + 1, initializedCalled);
Assert.Equal("Hi there!", result.ToString());
}

Expand All @@ -214,7 +221,8 @@ public async Task ShapeResultsAreRenderedOnceWhenCached()

// Shape is processed and rendered again.
Assert.Equal(2, bindCalled);
Assert.Equal(2, initializedCalled);
Assert.Equal(2, processedCalled);
Assert.Equal(12, initializedCalled);
Assert.Equal("Hi there!", result.ToString());
}

Expand Down