-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Refactoring: Add extension method for retrieval of language ISO codes if that's all we need #20324
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
Conversation
…bjects if that's all we need.
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.
Pull Request Overview
This PR introduces a performance optimization by adding a new method GetAllIsoCodesAsync() that retrieves only ISO culture codes from the database instead of full language objects when that's all that's needed.
- Adds
GetAllIsoCodesAsync()method to the language service and repository - Updates multiple locations that were fetching full language objects just to extract ISO codes
- Includes comprehensive test coverage for the new functionality
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Umbraco.Core/Services/ILanguageService.cs | Adds interface definition for GetAllIsoCodesAsync() with default implementation and documentation improvements |
| src/Umbraco.Core/Services/LanguageService.cs | Implements GetAllIsoCodesAsync() method that delegates to repository |
| src/Umbraco.Core/Persistence/Repositories/ILanguageRepository.cs | Adds GetAllIsoCodes() method interface with default implementation |
| src/Umbraco.Infrastructure/Persistence/Repositories/Implement/LanguageRepository.cs | Implements optimized database query to fetch only ISO codes |
| tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/LanguageServiceTests.cs | Adds integration test for new GetAllIsoCodesAsync() method |
| src/Umbraco.Core/Services/ContentValidationServiceBase.cs | Updates to use new optimized method |
| src/Umbraco.Core/Services/ContentPublishingService.cs | Updates two locations to use new optimized method |
| src/Umbraco.Core/Services/ContentEditingService.cs | Updates to use new optimized method |
| src/Umbraco.Core/Routing/UrlProviderExtensions.cs | Updates to use new optimized method |
| src/Umbraco.Core/Routing/PublishedUrlInfoProvider.cs | Updates to use new optimized method |
| src/Umbraco.Cms.Api.Management/Factories/UserGroupPresentationFactory.cs | Refactors to use existing GetIsoCodesByIdsAsync() method instead of fetching all languages |
| src/Umbraco.Cms.Api.Management/Factories/DictionaryPresentationFactory.cs | Updates to use new optimized method |
| src/Umbraco.Examine.Lucene/BackOfficeExamineSearcher.cs | Updates to use new optimized method |
| tests/Umbraco.Tests.Integration/Umbraco.Examine.Lucene/UmbracoExamine/ExamineExternalIndexSearcherTest.cs | Updates to use new optimized method |
src/Umbraco.Core/Persistence/Repositories/ILanguageRepository.cs
Outdated
Show resolved
Hide resolved
|
@AndyButland I like the syntax change throughout the code changes, where However, well intended as this is, it actually incurs a performance penalty if I'm not entirely mistaken.
The new Like I said, the syntax change is nice. Would it make sense to retain the default implementation of Another alternative would be to implement |
…extension method.
…/github.com/umbraco/Umbraco-CMS into v16/improvement/optimize-content-validation # Conflicts: # src/Umbraco.Core/Services/ILanguageService.cs
|
Ah yes, that's a good point @kjac - thanks for catching that and apologies for the oversight. I've followed your extension method suggestion. So no performance improvement now, but likely worth having the syntax improvement as you suggest. |
kjac
left a comment
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.
Looks good 💪
Description
There are a number of places where we retrieve all languages from the database, when we only need all ISO codes.
This PR introduces a new method that will only retrieve that field from the database, and uses it wherever we currently get all languages just to select only the ISO code field.
So a small optimization to reduce the amount of data we pull from the database.