Skip to content

Conversation

@izanhzh
Copy link
Collaborator

@izanhzh izanhzh commented Nov 13, 2024

fix: #4652

在单元测试中我增加了一个ValidateFixEndlessLoop方法,撤销掉我的代码调整,运行这个单元测试即可重现死循环问题

@bb-auto
Copy link

bb-auto bot commented Nov 13, 2024

Thanks for your PR, @izanhzh. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Nov 13, 2024

Reviewer's Guide by Sourcery

This PR fixes an endless loop issue in the JsonStringLocalizer by adding a type check to prevent recursive calls. The implementation includes new test cases to verify the fix and modifications to the localizer's string retrieval logic.

Sequence diagram for string retrieval in JsonStringLocalizer

sequenceDiagram
    participant Client
    participant JsonStringLocalizer
    participant Utility
    participant Localizer
    Client->>JsonStringLocalizer: Request string
    JsonStringLocalizer->>Utility: GetStringLocalizerFromService
    Utility-->>JsonStringLocalizer: Return Localizer
    JsonStringLocalizer->>JsonStringLocalizer: Check if Localizer is JsonStringLocalizer
    alt Localizer is not JsonStringLocalizer
        JsonStringLocalizer->>Localizer: GetLocalizerValueFromCache
        Localizer-->>JsonStringLocalizer: Return value
    else Localizer is JsonStringLocalizer
        JsonStringLocalizer-->>Client: Return null or default
    end
Loading

Class diagram for JsonStringLocalizer and related classes

classDiagram
    class JsonStringLocalizer {
        - string? ret
        + LocalizedString this[string name]
        + IEnumerable<LocalizedString> GetAllStrings(bool includeParentCultures)
    }
    class MockLocalizerFactory {
        + IStringLocalizer Create(Type resourceSource)
        + IStringLocalizer Create(string baseName, string location)
    }
    class MockLocalizerFactory2 {
        - IServiceProvider _serviceProvider
        + MockLocalizerFactory2(IServiceProvider serviceProvider)
        + IStringLocalizer Create(Type resourceSource)
        + IStringLocalizer Create(string baseName, string location)
    }
    class MockStringLocalizer {
        + LocalizedString this[string name]
    }
    JsonStringLocalizer --> MockLocalizerFactory
    JsonStringLocalizer --> MockLocalizerFactory2
    MockLocalizerFactory2 --> IServiceProvider
    MockLocalizerFactory2 --> MockLocalizerFactory
    MockLocalizerFactory --> MockStringLocalizer
    MockLocalizerFactory2 --> MockStringLocalizer
Loading

File-Level Changes

Change Details Files
Added type check to prevent recursive calls in string localization
  • Added condition to check if localizer is not JsonStringLocalizer type
  • Modified string retrieval logic in indexer method
  • Updated GetAllStrings method with the same type check
src/BootstrapBlazor/Localization/Json/JsonStringLocalizer.cs
Added new test case to verify the endless loop fix
  • Created ValidateFixEndlessLoop test method
  • Added MockLocalizerFactory2 class for testing multiple localizer scenarios
  • Implemented service provider based factory selection logic
test/UnitTest/Localization/JsonStringLocalizerTest.cs

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@bb-auto bb-auto bot requested a review from ArgoZhang November 13, 2024 03:58
sourcery-ai[bot]
sourcery-ai bot previously approved these changes Nov 13, 2024
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @izanhzh - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Please provide more context in the PR description about the endless loop issue being fixed and how the changes address it. This helps future maintainers understand the changes without having to reference external links.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@codecov
Copy link

codecov bot commented Nov 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (a8fa755) to head (97b1dca).
Report is 122 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #4653   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          620       620           
  Lines        27419     27419           
  Branches      3928      3928           
=========================================
  Hits         27419     27419           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ArgoZhang ArgoZhang changed the title fix(JsonStringLocalizer):Endless loop fix(JsonStringLocalizer): Endless loop Nov 14, 2024
@ArgoZhang ArgoZhang changed the title fix(JsonStringLocalizer): Endless loop fix(JsonStringLocalizer): add type check prevent endless loop Nov 14, 2024
@ArgoZhang ArgoZhang enabled auto-merge (squash) November 14, 2024 06:19
@ArgoZhang ArgoZhang disabled auto-merge November 14, 2024 06:32
@ArgoZhang ArgoZhang merged commit b73c3ec into main Nov 14, 2024
4 checks passed
@ArgoZhang ArgoZhang deleted the fix-json-string-localizer branch November 14, 2024 06:33
@ArgoZhang ArgoZhang added this to the v9.0.0 milestone Dec 24, 2024
@ArgoZhang ArgoZhang added the enhancement New feature or request label Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(JsonStringLocalizer): add type check prevent endless loop

3 participants