feat: make Unicode handling in external API calls configurable #4277
Conversation
- Add DevPreserveUnicodeInExternalCalls config option (default: false) - Refactor external provider to use NameForExternal() method on auxArtist - Remove redundant Name field from auxArtist struct - Update all external API calls (image, URL, biography, similar, top songs, MBID) to use configurable Unicode handling - Add comprehensive tests for both Unicode-preserving and normalized behaviors - Refactor tests to use constants and improved structure with BeforeEach blocks Fixes issue where Spotify integration failed to find artist images for artists with Unicode characters (e.g., en dash) in their names. Signed-off-by: Deluan <deluan@navidrome.org>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors Unicode handling for external API calls to better support artist names with Unicode characters by introducing a new configuration flag. Key changes include updating the auxArtist struct and its Name() method in the provider, adding comprehensive tests for both Unicode-preserving and normalized behaviors, and introducing a new configuration option in configuration.go.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| core/external/provider_artistimage_test.go | Added tests to validate Unicode handling based on config setting |
| core/external/provider.go | Refactored Unicode logic to use Name() method on auxArtist struct |
| conf/configuration.go | Added new configuration option with appropriate default settings |
| conf.Server.DevPreserveUnicodeInExternalCalls = false | ||
|
|
There was a problem hiding this comment.
[nitpick] The configuration option 'DevPreserveUnicodeInExternalCalls' is being redundantly set to false within the same test case. Removing the duplicate assignment will help reduce confusion and improve code clarity.
| conf.Server.DevPreserveUnicodeInExternalCalls = false |
There was a problem hiding this comment.
Summary of Changes
Hello @deluan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses an issue where external API calls for artist information could fail due to inconsistent Unicode character handling. My changes introduce a new configuration option to make Unicode handling configurable, refactor the core logic for artist name resolution, and update all relevant external API integrations to use this new, centralized mechanism. The goal is to improve compatibility with external services while maintaining backward compatibility by defaulting to the current normalization behavior.
Highlights
- Configurable Unicode Handling: I've introduced a new
DevPreserveUnicodeInExternalCallsconfiguration option. This allows users to explicitly control whether Unicode characters in artist names are preserved or normalized when making calls to external APIs. By default, it maintains the existing normalization behavior. - Refactored Artist Name Resolution: I've centralized the logic for handling artist names by refactoring the
auxArtiststruct. It now includes a newName()method that encapsulates the decision-making process, returning either the original or normalized artist name based on the new configuration setting. - Broad API Integration: All relevant external API calls within the system, including those for fetching artist images, URLs, biographies, similar artists, and top songs, have been updated to utilize the new
Name()method. This ensures consistent and configurable Unicode behavior across all external interactions. - Comprehensive Testing: I've added new, detailed Ginkgo/Gomega tests specifically for the Unicode handling logic. These tests cover both scenarios: when Unicode characters are preserved and when they are normalized, ensuring the feature works as expected and prevents regressions.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a configuration option to preserve Unicode characters in external API calls, resolving an issue with artist image retrieval. The code refactors the auxArtist struct and includes comprehensive tests for both Unicode-preserving and normalized behaviors.
Signed-off-by: Deluan <deluan@navidrome.org>
0aff106 to
bd875d3
Compare
Signed-off-by: Deluan <deluan@navidrome.org>
Extended the configurable Unicode handling to album names, matching the pattern already implemented for artist names. This ensures consistent behavior when DevPreserveUnicodeInExternalCalls is enabled for both artist and album external API calls. Changes: - Removed Name field from auxAlbum struct, added Name() method with Unicode logic - Updated getAlbum, UpdateAlbumInfo, populateAlbumInfo, and AlbumImage functions - Added comprehensive tests for album Unicode handling (preserve and normalize) - Fixed typo in artist image test description
Summary
This PR fixes and refactors Unicode handling in external API calls for artist names in Navidrome, making the Unicode-preserving behavior configurable via a new Dev config option.
Problem
Spotify integration was failing to find artist images for artists with Unicode characters in their names (ex: en dash in names like "Run–D.M.C."). The external provider was normalizing Unicode characters before making API calls, causing mismatches with external services that expect the original Unicode characters.
Solution
Configuration
DevPreserveUnicodeInExternalCallsconfig option (default: false)Code Changes
auxArtiststruct to remove redundantNamefieldNameForExternal()method that encapsulates Unicode handling logicTests
BeforeEachblocks andWhencontextsDeferCleanup(configtest.SetupConfig())Files Modified
conf/configuration.go: Added new config optioncore/external/provider.go: Refactored Unicode handling logiccore/external/provider_artistimage_test.go: Added comprehensive testsBackward Compatibility
Testing
make lintall)make test PKG=./core/external/...)Fixes #4227