Conversation
Contributor
Reviewer's GuideAdds a reset mechanism for the JsonStringLocalizer missing-manifest cache, wires it into JsonStringLocalizerFactory and CacheManager, and updates tests to validate cache clearing behavior and adjust missing-item handling logic. Sequence diagram for cache clearing and JsonStringLocalizer missing-manifest cache resetsequenceDiagram
actor Application
participant CacheManager
participant Cache as MemoryCache
participant Provider as IServiceProvider
participant Factory as IStringLocalizerFactory
participant JsonFactory as JsonStringLocalizerFactory
participant Localizer as JsonStringLocalizer
Application->>CacheManager: Clear(key)
alt Cache is MemoryCache
CacheManager->>Cache: Compact(100)
CacheManager->>Provider: GetServices(IStringLocalizerFactory)
loop for each factory
Provider-->>CacheManager: factory
CacheManager->>Factory: type check
alt factory is JsonStringLocalizerFactory
CacheManager->>JsonFactory: Reset()
JsonFactory->>Localizer: ResetMissingMainifestCache()
Localizer->>Localizer: _missingManifestCache.Clear()
else other factory types
CacheManager-->>Factory: no action
end
end
else Cache not MemoryCache
CacheManager-->>Application: handle other cache types
end
CacheManager-->>Application: Clear completed
Class diagram for JsonStringLocalizer cache reset integrationclassDiagram
class JsonStringLocalizer {
- ConcurrentDictionary~string, object?~ _missingManifestCache
+ LocalizedString this[string name]
+ LocalizedString this[string name, params object[] arguments]
+ IEnumerable~LocalizedString~ GetAllStrings(bool includeParentCultures)
- string? GetStringFromJson(string name)
- List~LocalizedString~ MergeResolveLocalizers(IEnumerable~LocalizedString~ localizedStrings, IEnumerable~LocalizedString~ baseResourceStrings)
- void HandleMissingResourceItem(string name)
+ void ResetMissingMainifestCache()
}
class JsonStringLocalizerFactory {
- JsonLocalizationOptions _jsonLocalizationOptions
- ILocalizationMissingItemHandler _localizationMissingItemHandler
- string _typeName
- JsonStringLocalizer _localizer
+ JsonStringLocalizerFactory(ILoggerFactory loggerFactory, IOptions~JsonLocalizationOptions~ localizationOptions, ILocalizationMissingItemHandler localizationMissingItemHandler)
+ string GetResourcePrefix(string baseResourceName, string baseNamespace, string resourceLocation, Assembly resourceAssembly)
+ ResourceManagerStringLocalizer CreateResourceManagerStringLocalizer(Assembly assembly, string baseName)
+ void Reset()
}
class CacheManager {
- object Cache
- IServiceProvider Provider
+ void Clear(object key)
}
class ResourceManagerStringLocalizerFactory
class ResourceManagerStringLocalizer
class IStringLocalizerFactory
class ILoggerFactory
class JsonLocalizationOptions
class ILocalizationMissingItemHandler
class IServiceProvider {
+ IEnumerable~IStringLocalizerFactory~ GetServices()
}
class MemoryCache {
+ void Compact(double percentage)
}
JsonStringLocalizerFactory --|> ResourceManagerStringLocalizerFactory
ResourceManagerStringLocalizerFactory ..|> IStringLocalizerFactory
JsonStringLocalizerFactory o--> JsonStringLocalizer : creates
JsonStringLocalizerFactory --> JsonLocalizationOptions
JsonStringLocalizerFactory --> ILocalizationMissingItemHandler
JsonStringLocalizerFactory --> ILoggerFactory
CacheManager --> IServiceProvider : Provider
CacheManager --> MemoryCache : Cache
CacheManager ..> IStringLocalizerFactory : resolves
CacheManager ..> JsonStringLocalizerFactory : calls Reset
JsonStringLocalizer ..> ResourceManagerStringLocalizer
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new method name
ResetMissingMainifestCachecontains a typo inMainifest; consider renaming it (and the factory’sResetcall site) toResetMissingManifestCachebefore this becomes part of the public API surface. - The logic in
HandleMissingResourceItemhas changed so that_missingManifestCacheis only updated whenIgnoreLocalizerMissingis true (previously it was always updated); please confirm whether this behavioral change is intentional or adjust so caching remains consistent with prior behavior. - Caching a single
JsonStringLocalizerinstance inJsonStringLocalizerFactoryand resetting only that instance inReset()may miss other localizers created by the factory (especially in multi-type or concurrent scenarios); consider a design that tracks and resets all relevant localizer instances instead of just the last one.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new method name `ResetMissingMainifestCache` contains a typo in `Mainifest`; consider renaming it (and the factory’s `Reset` call site) to `ResetMissingManifestCache` before this becomes part of the public API surface.
- The logic in `HandleMissingResourceItem` has changed so that `_missingManifestCache` is only updated when `IgnoreLocalizerMissing` is true (previously it was always updated); please confirm whether this behavioral change is intentional or adjust so caching remains consistent with prior behavior.
- Caching a single `JsonStringLocalizer` instance in `JsonStringLocalizerFactory` and resetting only that instance in `Reset()` may miss other localizers created by the factory (especially in multi-type or concurrent scenarios); consider a design that tracks and resets all relevant localizer instances instead of just the last one.
## Individual Comments
### Comment 1
<location path="src/BootstrapBlazor/Localization/Json/JsonStringLocalizer.cs" line_range="115" />
<code_context>
+ /// <para lang="en">Reset cache method</para>
+ /// <para>v<version>10.5.0</version></para>
+ /// </summary>
+ public void ResetMissingMainifestCache() => _missingManifestCache.Clear();
+
private string? GetStringFromJson(string name)
</code_context>
<issue_to_address>
**issue (typo):** Typo in method name `ResetMissingMainifestCache` may cause confusion and makes the API harder to discover.
The name is misspelled as `Mainifest` instead of `Manifest`. Since this is a public API, consider renaming it to `ResetMissingManifestCache` for clarity and IDE discoverability. If renaming is breaking, you could add the correctly spelled method and keep this one as an `[Obsolete]` shim.
Suggested implementation:
```csharp
private readonly ConcurrentDictionary<string, object?> _missingManifestCache = [];
/// <summary>
/// <para lang="zh">清除缓存方法</para>
/// <para lang="en">Reset cache method</para>
/// <para>v<version>10.5.0</version></para>
/// </summary>
[Obsolete("Use ResetMissingManifestCache instead.")]
public void ResetMissingMainifestCache() => ResetMissingManifestCache();
/// <summary>
/// <para lang="zh">清除缓存方法</para>
/// <para lang="en">Reset cache method</para>
/// <para>v<version>10.5.0</version></para>
/// </summary>
public void ResetMissingManifestCache() => _missingManifestCache.Clear();
private string? GetStringFromJson(string name)
```
1. Search the solution for any usages of `ResetMissingMainifestCache` and update them to call `ResetMissingManifestCache` instead, so that the obsolete shim is only for external callers.
2. Ensure the project has `using System;` in this file (or globally) so that the `[Obsolete]` attribute resolves correctly.
</issue_to_address>
### Comment 2
<location path="src/BootstrapBlazor/Localization/Json/JsonStringLocalizer.cs" line_range="183-186" />
<code_context>
{
localizationMissingItemHandler.HandleMissingItem(name, typeName, CultureInfo.CurrentUICulture.Name);
- if (jsonLocalizationOptions.IgnoreLocalizerMissing == false)
+ if (jsonLocalizationOptions.IgnoreLocalizerMissing)
+ {
+ _missingManifestCache.TryAdd($"name={name}&culture={CultureInfo.CurrentUICulture.Name}", null);
+ return;
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Reversed logic for `IgnoreLocalizerMissing` changes behavior and stops caching missing entries when logging is enabled.
This change inverts the meaning of `IgnoreLocalizerMissing` and alters behavior: when `true` we now only cache and return (no log), and when `false` we log but no longer cache. That removes caching in the non-ignore case, which previously logged and updated `_missingManifestCache`. If the goal is only to suppress logging when the flag is `true`, keep the cache update unconditional and gate only the log:
```csharp
localizationMissingItemHandler.HandleMissingItem(...);
if (!jsonLocalizationOptions.IgnoreLocalizerMissing && Logger.IsEnabled(LogLevel.Information))
{
Logger.LogInformation(...);
}
_missingManifestCache.TryAdd(...);
```
</issue_to_address>
### Comment 3
<location path="src/BootstrapBlazor/Localization/Json/JsonStringLocalizerFactory.cs" line_range="114" />
<code_context>
- protected override ResourceManagerStringLocalizer CreateResourceManagerStringLocalizer(Assembly assembly, string baseName) => new JsonStringLocalizer(assembly, _typeName!, baseName, _jsonLocalizationOptions, _loggerFactory.CreateLogger<JsonStringLocalizer>(), ResourceNamesCache, _localizationMissingItemHandler);
+ protected override ResourceManagerStringLocalizer CreateResourceManagerStringLocalizer(Assembly assembly, string baseName)
+ {
+ _localizer = new JsonStringLocalizer(assembly, _typeName!, baseName, _jsonLocalizationOptions, _loggerFactory.CreateLogger<JsonStringLocalizer>(), ResourceNamesCache, _localizationMissingItemHandler);
+ return _localizer;
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Tracking only the last created `JsonStringLocalizer` means `Reset()` will not affect earlier instances and may be racy.
Because the factory only keeps a single `_localizer` reference and `Reset()` delegates to it, any previously created `JsonStringLocalizer` instances (for other `baseName`/assembly or consumers) will never be reset. In a concurrent scenario, `_localizer` may also be overwritten while another thread calls `Reset()` expecting to target a different instance.
To align `Reset()` with the intended cache-clearing semantics, consider either:
- Tracking all created localizers (e.g., in a thread-safe collection) and resetting them all, or
- Centralizing cache reset in a shared/global cache, or
- Clearly scoping the factory so that only one localizer instance can exist per scope/lifetime.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| protected override ResourceManagerStringLocalizer CreateResourceManagerStringLocalizer(Assembly assembly, string baseName) => new JsonStringLocalizer(assembly, _typeName!, baseName, _jsonLocalizationOptions, _loggerFactory.CreateLogger<JsonStringLocalizer>(), ResourceNamesCache, _localizationMissingItemHandler); | ||
| protected override ResourceManagerStringLocalizer CreateResourceManagerStringLocalizer(Assembly assembly, string baseName) | ||
| { | ||
| _localizer = new JsonStringLocalizer(assembly, _typeName!, baseName, _jsonLocalizationOptions, _loggerFactory.CreateLogger<JsonStringLocalizer>(), ResourceNamesCache, _localizationMissingItemHandler); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Tracking only the last created JsonStringLocalizer means Reset() will not affect earlier instances and may be racy.
Because the factory only keeps a single _localizer reference and Reset() delegates to it, any previously created JsonStringLocalizer instances (for other baseName/assembly or consumers) will never be reset. In a concurrent scenario, _localizer may also be overwritten while another thread calls Reset() expecting to target a different instance.
To align Reset() with the intended cache-clearing semantics, consider either:
- Tracking all created localizers (e.g., in a thread-safe collection) and resetting them all, or
- Centralizing cache reset in a shared/global cache, or
- Clearly scoping the factory so that only one localizer instance can exist per scope/lifetime.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7812 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 764 764
Lines 34117 34115 -2
Branches 4697 4697
=========================================
- Hits 34117 34115 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a reset mechanism for the JSON localizer’s internal “missing manifest” cache and wires it into CacheManager.Clear(), along with a unit test and a package version bump to 10.5.0-beta03.
Changes:
- Add
ResetMissingMainifestCache()toJsonStringLocalizerand expose aReset()hook onJsonStringLocalizerFactory. - Invoke the factory reset from
CacheManager.Clear()to clear localization-related caches during full cache clears. - Extend unit tests to validate the reset behavior; bump package version.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTest/Localization/JsonStringLocalizerTest.cs | Adds a test that triggers a missing key and verifies the internal missing-cache is cleared after ICacheManager.Clear(). |
| src/BootstrapBlazor/Services/CacheManager.cs | Calls into JSON localizer factory reset after compacting the memory cache. |
| src/BootstrapBlazor/Localization/Json/JsonStringLocalizerFactory.cs | Stores a created localizer instance and adds a Reset() method that clears the localizer’s missing cache. |
| src/BootstrapBlazor/Localization/Json/JsonStringLocalizer.cs | Adds a public reset method for the missing-manifest cache and adjusts missing-item handling logic. |
| src/BootstrapBlazor/BootstrapBlazor.csproj | Version bump from 10.5.0-beta02 to 10.5.0-beta03. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/BootstrapBlazor/Localization/Json/JsonStringLocalizerFactory.cs
Outdated
Show resolved
Hide resolved
# Conflicts: # src/BootstrapBlazor/BootstrapBlazor.csproj
ArgoZhang
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #7811
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add cache reset support for JSON-based string localization and integrate it with the global cache clearing mechanism.
New Features:
Bug Fixes:
Tests: