-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Replace middleware with simple endpoints #17373
Changes from 9 commits
df10660
217bf0e
6302024
081cf78
ab8db2a
8777c2f
85ec062
47e3187
ce1f203
432cb1d
2a83296
d3204ca
fe193a6
1be4ffa
3f099ac
854bec7
e596b25
2160d1b
773538c
58324ec
d5b10c9
8e1de2c
48da77c
274c530
7c63037
0d9a46e
3a64079
b27516e
57fc7ca
b137ab4
aa3c4ce
54a9874
d3d2e0d
69b5100
3b84be9
eaaaea0
dfeea37
5e322b8
e34fc45
4250747
79f7e81
203fe20
f857e4a
54471f2
ecb1781
9936f87
5fc5aeb
65f0e1c
00497b6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,126 @@ | ||
using System.Text; | ||
using Microsoft.AspNetCore.Builder; | ||
using Microsoft.AspNetCore.Http; | ||
using Microsoft.AspNetCore.Mvc; | ||
using Microsoft.AspNetCore.Routing; | ||
using Microsoft.Extensions.Caching.Memory; | ||
using Microsoft.Extensions.Configuration; | ||
using Microsoft.Extensions.DependencyInjection; | ||
using Microsoft.Extensions.Primitives; | ||
using OrchardCore.Environment.Shell.Configuration; | ||
using OrchardCore.Facebook.Settings; | ||
using OrchardCore.Settings; | ||
|
||
#nullable enable | ||
|
||
namespace OrchardCore.Facebook.Endpoints; | ||
|
||
public static class GetSdkEndpoints | ||
{ | ||
public static IEndpointRouteBuilder AddSdkEndpoints(this IEndpointRouteBuilder builder) | ||
{ | ||
builder.MapGet("/OrchardCore.Facebook/sdk/fbsdk.js", HandleFbsdkScriptRequestAsync) | ||
.AllowAnonymous() | ||
.DisableAntiforgery(); | ||
|
||
builder.MapGet("/OrchardCore.Facebook/sdk/fb.js", HandleFbScriptRequestAsync) | ||
.AllowAnonymous() | ||
.DisableAntiforgery(); | ||
|
||
return builder; | ||
} | ||
|
||
private static uint GetHashCode(byte[] bytes) | ||
{ | ||
uint hash = 0; | ||
foreach (byte b in bytes) | ||
{ | ||
hash += b; | ||
hash *= 17; | ||
} | ||
return hash; | ||
} | ||
|
||
private static IResult HandleFbsdkScriptRequestAsync([FromQuery(Name = "lang")] string language, [FromQuery(Name = "sdkf")] string sdkFilename, HttpContext context, IMemoryCache cache) | ||
{ | ||
// Set the cache timeout to the maximum allowed length of one year | ||
// max-age is needed because immutable is not widly supported | ||
context.Response.Headers.CacheControl = $"public, max-age=31536000, immutable"; | ||
|
||
string scriptCacheKey = $"~/OrchardCore.Facebook/sdk/fbsdk.js?sdkf={sdkFilename}&lang={language}"; | ||
|
||
var scriptBytes = cache.Get(scriptCacheKey) as byte[]; | ||
if (scriptBytes == null) | ||
{ | ||
// Note: Update script version in ResourceManagementOptionsConfiguration.cs after editing | ||
scriptBytes = Encoding.UTF8.GetBytes($@"(function(d){{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pretty sure it could use the same "preamble/end" caching mechanism I showed today. And then nothing needs to use memory cache. Unless we decide to return the result in a single byte[]. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For this endpoint is there any downside to using the The other two endpoints get the advantage of being able to hash the entire byte array and thus revalidation. |
||
var js, id = 'facebook-jssdk'; if (d.getElementById(id)) {{ return; }} | ||
js = d.createElement('script'); js.id = id; js.async = true; | ||
js.src = ""https://connect.facebook.net/{language.Replace('-', '_')}/{sdkFilename}""; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not safe (encoding). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed again: Refactored to use dependency injection. |
||
d.getElementsByTagName('head')[0].appendChild(js); | ||
}} (document));"); | ||
|
||
cache.Set(scriptCacheKey, scriptBytes); | ||
} | ||
|
||
return Results.Bytes(scriptBytes, "application/javascript"); | ||
} | ||
|
||
private static async Task<IResult> HandleFbScriptRequestAsync(HttpContext context, ISiteService siteService, IMemoryCache cache, IShellConfiguration shellConfiguration) | ||
{ | ||
var settings = await siteService.GetSettingsAsync<FacebookSettings>(); | ||
|
||
if (string.IsNullOrWhiteSpace(settings.AppId)) | ||
{ | ||
return Results.Forbid(); | ||
} | ||
|
||
context.Response.Headers.CacheControl = shellConfiguration.GetValue( | ||
"StaticFileOptions:CacheControl", | ||
// Fallback value | ||
$"public, max-age={TimeSpan.FromDays(30).TotalSeconds}"); | ||
|
||
// Assumes IfNoneMatch has only one ETag for performance | ||
var requestETag = context.Request.Headers.IfNoneMatch; | ||
if (!StringValues.IsNullOrEmpty(requestETag) && cache.Get(requestETag) != null) | ||
{ | ||
context.Response.Headers.ETag = requestETag; | ||
|
||
return Results.StatusCode(304); | ||
} | ||
|
||
string scriptCacheKey = $"/OrchardCore.Facebook/sdk/fb.js.{settings.AppId}.{settings.Version}"; | ||
|
||
var scriptBytes = (byte[]?)cache.Get(scriptCacheKey); | ||
if (scriptBytes == null) | ||
{ | ||
// Generate script | ||
var options = $"{{ appId:'{settings.AppId}',version:'{settings.Version}'"; | ||
options = string.IsNullOrWhiteSpace(settings.FBInitParams) | ||
? string.Concat(options, "}") | ||
: string.Concat(options, ",", settings.FBInitParams, "}"); | ||
scriptBytes = Encoding.UTF8.GetBytes($"window.fbAsyncInit = function(){{ FB.init({options});}};"); | ||
|
||
cache.Set(scriptCacheKey, scriptBytes); | ||
} | ||
|
||
// False positive: No comparison is taking place here | ||
#pragma warning disable RS1024 | ||
// Uses a custom GetHashCode because Object.GetHashCode differs across processes | ||
StringValues eTag = $"\"{GetHashCode(scriptBytes)}\""; | ||
#pragma warning restore RS1024 | ||
|
||
// Mark that the eTag corresponds to a fresh file | ||
cache.Set(eTag, true); | ||
|
||
context.Response.Headers.ETag = eTag; | ||
|
||
// Can be true if the cache was reset after the last client request | ||
if (requestETag.Equals(eTag)) | ||
{ | ||
return Results.StatusCode(304); | ||
} | ||
|
||
return Results.Bytes(scriptBytes, "application/javascript"); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,25 +1,38 @@ | ||
using System.Globalization; | ||
using Microsoft.Extensions.Options; | ||
using OrchardCore.Facebook.Settings; | ||
using OrchardCore.ResourceManagement; | ||
using OrchardCore.Settings; | ||
|
||
namespace OrchardCore.Facebook; | ||
|
||
public sealed class ResourceManagementOptionsConfiguration : IConfigureOptions<ResourceManagementOptions> | ||
{ | ||
private static readonly ResourceManifest _manifest; | ||
private readonly ResourceManifest _manifest; | ||
private readonly ISiteService _siteService; | ||
|
||
static ResourceManagementOptionsConfiguration() | ||
public ResourceManagementOptionsConfiguration(ISiteService siteService) | ||
{ | ||
_siteService = siteService; | ||
|
||
_manifest = new ResourceManifest(); | ||
|
||
_manifest | ||
.DefineScript("fb") | ||
.SetDependencies("fbsdk") | ||
.SetUrl("~/OrchardCore.Facebook/sdk/fb.js"); | ||
} | ||
|
||
public async void Configure(ResourceManagementOptions options) | ||
{ | ||
var settings = await _siteService.GetSettingsAsync<FacebookSettings>(); | ||
var language = CultureInfo.CurrentUICulture.Name; | ||
|
||
_manifest | ||
.DefineScript("fbsdk") | ||
.SetUrl("~/OrchardCore.Facebook/sdk/fbsdk.js"); | ||
} | ||
// v parameter is for cache busting | ||
.SetUrl($"~/OrchardCore.Facebook/sdk/fbsdk.js?sdkf={settings.SdkJs}&lang={language}&v=1.0"); | ||
|
||
public void Configure(ResourceManagementOptions options) => options.ResourceManifests.Add(_manifest); | ||
options.ResourceManifests.Add(_manifest); | ||
} | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
using System.Text; | ||
using Fluid; | ||
using Microsoft.AspNetCore.Builder; | ||
using Microsoft.AspNetCore.Http; | ||
using Microsoft.AspNetCore.Routing; | ||
using Microsoft.Extensions.Caching.Memory; | ||
using Microsoft.Extensions.Configuration; | ||
using Microsoft.Extensions.DependencyInjection; | ||
using Microsoft.Extensions.Options; | ||
using Microsoft.Extensions.Primitives; | ||
using Microsoft.Net.Http.Headers; | ||
using OrchardCore.DisplayManagement.Liquid; | ||
using OrchardCore.Environment.Shell.Configuration; | ||
|
||
#nullable enable | ||
|
||
namespace OrchardCore.Liquid.Endpoints.Scripts; | ||
|
||
public static class GetIntellisenseEndpoint | ||
{ | ||
public static IEndpointRouteBuilder AddGetIntellisenseScriptEndpoint(this IEndpointRouteBuilder builder) | ||
{ | ||
builder.MapGet("OrchardCore.Liquid/Scripts/liquid-intellisense.js", HandleRequest) | ||
.AllowAnonymous() | ||
.DisableAntiforgery(); | ||
|
||
return builder; | ||
} | ||
|
||
private static byte[] GenerateScript(IServiceProvider serviceProvider) | ||
{ | ||
var templateOptions = serviceProvider.GetRequiredService<IOptions<TemplateOptions>>(); | ||
var liquidViewParser = serviceProvider.GetRequiredService<LiquidViewParser>(); | ||
var filters = string.Join(',', templateOptions.Value.Filters.Select(x => $"'{x.Key}'")); | ||
var tags = string.Join(',', liquidViewParser.RegisteredTags.Select(x => $"'{x.Key}'")); | ||
var script = $@"[{filters}].forEach(value=>{{if(!liquidFilters.includes(value)){{ liquidFilters.push(value);}}}}); | ||
[{tags}].forEach(value=>{{if(!liquidTags.includes(value)){{ liquidTags.push(value);}}}});"; | ||
|
||
return Encoding.UTF8.GetBytes(script); | ||
} | ||
|
||
private static uint GetHashCode(byte[] bytes) | ||
{ | ||
uint hash = 0; | ||
foreach (byte b in bytes) | ||
{ | ||
hash += b; | ||
hash *= 17; | ||
} | ||
return hash; | ||
} | ||
|
||
private static IResult HandleRequest(HttpContext context, IMemoryCache cache, IShellConfiguration shellConfiguration) | ||
{ | ||
context.Response.Headers.CacheControl = shellConfiguration.GetValue( | ||
"StaticFileOptions:CacheControl", | ||
// Fallback value | ||
$"public, max-age={TimeSpan.FromDays(30).TotalSeconds}"); | ||
|
||
// Assumes IfNoneMatch has only one ETag for performance | ||
var requestETag = context.Request.Headers.IfNoneMatch; | ||
if (!StringValues.IsNullOrEmpty(requestETag) && cache.Get(requestETag) != null) | ||
{ | ||
context.Response.Headers.ETag = requestETag; | ||
|
||
return Results.StatusCode(304); | ||
} | ||
|
||
const string scriptCacheKey = "OrchardCore.Liquid/Scripts/liquid-intellisense.js"; | ||
|
||
var scriptBytes = (byte[]?)cache.Get(scriptCacheKey); | ||
if (scriptBytes == null) | ||
{ | ||
scriptBytes = GenerateScript(context.RequestServices); | ||
|
||
cache.Set(scriptCacheKey, scriptBytes); | ||
} | ||
|
||
// False positive: No comparison is taking place here | ||
#pragma warning disable RS1024 | ||
// Uses a custom GetHashCode because Object.GetHashCode differs across processes | ||
StringValues eTag = $"\"{GetHashCode(scriptBytes)}\""; | ||
#pragma warning restore RS1024 | ||
|
||
// Mark that the eTag corresponds to a fresh file | ||
cache.Set(eTag, true); | ||
|
||
context.Response.Headers.ETag = eTag; | ||
|
||
// Can be true if the cache was reset after the last client request | ||
if (requestETag.Equals(eTag)) | ||
{ | ||
return Results.StatusCode(304); | ||
} | ||
|
||
return Results.Bytes(scriptBytes, "application/javascript"); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the dotnet component that does it already: https://learn.microsoft.com/en-us/dotnet/api/system.hashcode.addbytes?view=net-9.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This made me think some more about how we should handle etag here. What will happen is that when the cache is stale on the client, it will send a request with the etag it has, which represents the version of the content it has for this resource (a hash can be considered a version indifferently in this case). And in the case of FB scripts (and others) they will not vary ever, until we actually ship another version of Orchard. So we can use a constant value for the etag, that will get a new value when we change the script. This etag could be done manually, or initialized based on the content on first use.
I am not sure about culture specific requests though, I haven't checked if the script depends on the requested culture, or the current OC's culture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also play with the url of the script to have different ones per version and culture. This way the cache would be infinite. And no etag to maintain. We could still cache the content to serve the same to all clients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be risky as the language codes can differ by two chars. E.g. "us" and "su" add to the same value. Thus the multiplication
hash *= 17
is needed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1- The hashcode algorithm in dotnet is probably better than yours (euphemism), and faster. You should really trust it. And if you don't: https://sharplab.io/#v2:C4LgTgrgdgNAJiA1AHwAICYAMBYAUBgRj1UwAJUCA6AFQFMAPYAbjzwDcBDMUgCwNIC8pKLQDupABIcAzjwDCAezi0AFAEoWuPpQCCcOACEAnsFrSVAUSgBjJQEsoAc0oBVagDEAHJQDitYMam5gBEPLQANuEKALQQ0sFqGqy4nNw86ILCYpIy8kqqSVrouvqBZpY29k6uHt5+ASbloRFR0dIQCYXEBACcKtrUClKyisrqhRR96TRDuaMFGkA===
2- Since the script are actually static per deployment we can make up a static key for the etag not based on a hash, but something like
1.us
(version/culture).3- We don't need an etag since we can cache forever if the url has the version and culture in it (can be arguments in the querystring
?v=1
). Keeping the vary-by header based on accept-language should tell the client to cache independently.If it's getting too complicated feel free to request a pair programming session.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but HashCode purposely won't work across process. There is System.IO.Hashing for non-cryptographic hashing if you want to introduce a dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to this endpoint to use infinite caching and versioning instead of etag and cache validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caching won't either. So technically not required to have a stable hashcode. It's stable as long as the process lives. Otherwise use xxHash. (in System.IO.Hashing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implimented xxHash hashing. This enables cache revalidation to work across process/restarts thanks to these lines: