-
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
base: main
Are you sure you want to change the base?
Conversation
@sebastienros please review. |
return builder; | ||
} | ||
|
||
private static uint GetHashCode(byte[] bytes) |
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.
Use the dotnet component that does it already: https://learn.microsoft.com/en-us/dotnet/api/system.hashcode.addbytes?view=net-9.0
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.
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.
The hashcode algorithm in dotnet is probably better
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.
HashCode purposely won't work across process
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:
// Can be true if the cache was reset after the last client request
if (requestETag.Equals(eTag))
{
return Results.StatusCode(304);
}
private static async Task<IResult> HandleFbsdkScriptRequestAsync(HttpContext context, ISiteService siteService, IMemoryCache cache) | ||
{ | ||
context.Response.Headers.Vary = "Accept-Language"; | ||
var shellConfiguration = context.RequestServices.GetRequiredService<IShellConfiguration>(); |
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 value should be initialized in the constructor, or lazily initialized. It won't change until the app is restarted and can be cached.
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.
Refactored to use dependency injection.
This got me thinking a bunch, why I wanted to chat. I am wondering if it would be easier to treat this route as a View, that would render a script. It could then use query strings. That would also simplify its definition. Could use either Razor Slices or Fluid to convert a minimal api endpoint to a view. They both have minimal api Result for that. |
I'm happy to chat anytime just ping me on discord @jbytes1027. That may be a simpler approach in some ways but the current changes do work without issue and I don't see them needing to be changed anytime soon. |
return builder; | ||
} | ||
|
||
private static uint GetHashCode(byte[] bytes) |
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.
HashCode purposely won't work across process
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)
scriptBytes = Encoding.UTF8.GetBytes($@"(function(d){{ | ||
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
For this endpoint is there any downside to using the IMemoryCache
? The beginning and end would still be cached in memory of course, just without IMemoryCache
. Also splitting it up makes it less readable.
The other two endpoints get the advantage of being able to hash the entire byte array and thus revalidation.
Fix #15629:
Manually tested to ensure returned scripts are the same and caching logic works.