Rewrite culture handling logic #2068
Conversation
xingsy97
force-pushed
the
refactor-culture-main
branch
from
d0e4d1d
to
7b20491
Compare
| @@ -160,7 +160,7 @@ private Task ProcessNegotiationRequest(IOwinContext owinContext, HostContext con | |||
| // add OriginalPath and QueryString when the clients protocol is higher than 2.0, earlier ASP.NET SignalR clients does not support redirect URL with query parameters | |||
| if (!string.IsNullOrEmpty(clientProtocol) && Version.TryParse(clientProtocol, out var version) && version >= ClientSupportQueryStringVersion) | |||
| { | |||
| var clientRequestId = _connectionRequestIdProvider.GetRequestId(); | |||
| var clientRequestId = _connectionRequestIdProvider.GetRequestId(""); | |||
There was a problem hiding this comment.
so for aspnet the id always start with -?
There was a problem hiding this comment.
use a prefix "aspnet"
| @@ -149,6 +154,12 @@ protected override Task OnClientConnectedAsync(OpenConnectionMessage message) | |||
| connection.ServiceConnection = this; | |||
|
|
|||
| connection.Features.Set<IConnectionMigrationFeature>(null); | |||
|
|
|||
| if (!_cultureInfoManager.TryApplyCulture(connection.RequestId)) | |||
There was a problem hiding this comment.
if (_cultureInfoManager.TryGetCultureFeature(connection.RequestId, out var feature)){
connection.GetHttpContext().Features.Set<IRequestCultureFeature>(feature)
}There was a problem hiding this comment.
connection.GetHttpContext().Features.Set<IRequestCultureFeature>(feature) doesn't take effect
I use CultureInfo.CurrentCulture=XXX and CultureInfo.CureentUICulture=XXX
xingsy97
force-pushed
the
refactor-culture-main
branch
3 times, most recently
from
7972e63
to
2152f07
Compare
| { | ||
| return Convert.ToBase64String(BitConverter.GetBytes(Stopwatch.GetTimestamp())); | ||
| return $"{traceIdentifier}-{Convert.ToBase64String(BitConverter.GetBytes(Stopwatch.GetTimestamp()))}"; |
There was a problem hiding this comment.
traceIdentifier is in such format 0HN7T92QVLV5T:00000009 https://github.com/aspnet/KestrelHttpServer/blob/master/src/Kestrel.Core/Internal/Infrastructure/CorrelationIdGenerator.cs#L19, and could be customized using middlewares.
Do we want : in the requestId? might the generated requestId too long?
There was a problem hiding this comment.
now I use return $"{traceIdentifier}:{Stopwatch.GetTimestamp().ToString("X")}";
base64 encoding is not needed for this id will be UrlEncoded before inserted into query string
use hex to shorten the length
src/Microsoft.Azure.SignalR.Common/Interfaces/IClientConnectionManager.cs
Outdated
Show resolved
Hide resolved
| ); | ||
|
|
||
| _cultureInfoManager.TryAddCultureFeature(clientRequestId, cultureFeature); |
There was a problem hiding this comment.
is it possible to check if culture feature is unchanged? If it is unchanged, there is no need to add and apply
There was a problem hiding this comment.
is it that only blazor app is impacted by the culture?
There was a problem hiding this comment.
1\ yes, we can check. However if we don't add the feature, a not found error log will be triggered when trying to apply the culture feature.
2\ added blazor check
xingsy97
force-pushed
the
refactor-culture-main
branch
from
8fd466e
to
47b8960
Compare
xingsy97
changed the title
xingsy97
force-pushed
the
refactor-culture-main
branch
from
e48755d
to
9983800
Compare
xingsy97
force-pushed
the
refactor-culture-main
branch
from
3f24099
to
aaad24b
Compare
|
Add some UTs? |
| { | ||
| return Convert.ToBase64String(BitConverter.GetBytes(Stopwatch.GetTimestamp())); | ||
| // Before filled into query string, this id will be process by "WebUtility.UrlEncode(...)". So base64 encoding is not needed. |
There was a problem hiding this comment.
let's replace : with - to make it more readable, also use string.IsNullorEmpty(traceidentifier) ? {Stopwatch.GetTimestamp().ToString("X") : $"{traceIdentifier}:{Stopwatch.GetTimestamp().ToString("X")}" instead of append aspnet prefix
| public bool IsDefaultFeature(IRequestCultureFeature feature) | ||
| { | ||
| // this is the default feature value in blazor when no culture feature is configured by app server | ||
| return feature.RequestCulture.Culture == CultureInfo.DefaultThreadCurrentCulture && feature.RequestCulture.UICulture == CultureInfo.DefaultThreadCurrentUICulture && feature.Provider is AcceptLanguageHeaderRequestCultureProvider; |
There was a problem hiding this comment.
is AcceptLanguageHeaderRequestCultureProvider does not mean it is default though
Based on #2067
Fix #2038