feat: 支持前端显示和编辑sortName #33
Conversation
Summary of ChangesHello @Starrah, 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! 此拉取请求旨在增强音乐管理功能，允许用户在前端界面直接查看和修改音乐的 Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the functionality to display and edit the sortName property for music tracks in the frontend, allowing for manual correction of automatically generated sort names, especially for non-Japanese songs. However, two critical security vulnerabilities have been identified: a Path Traversal vulnerability in the new API endpoint due to unvalidated route parameters, and a potential Denial of Service (application crash) when processing XML files that lack the newly introduced sortName tag. Additionally, the implementation has several areas for improvement, including a NullReferenceException in MusicXml.cs if the sortName XML node is missing, the new API endpoint in MusicController.cs returning an empty string instead of a proper error when music is not found, and a lack of error handling in the frontend's sync function.
| const sync = (key: keyof MusicXmlWithABJacket, method: Function) => async () => { | ||
| if (!info.value) return; | ||
| info.value!.modified = true; | ||
| await method(info.value.id!, info.value.assetDir, (info.value as any)[key]!); | ||
| const value = (info.value as any)[key]; | ||
| const result = (await method(info.value.id!, info.value.assetDir, value)).data; | ||
| if (key === "sortName" && typeof result === "string" && result !== value) { | ||
| // 如果调用的是sortName接口，且返回的字符串（经过格式化后的实际内容）和传过去的值不同的话，则覆盖之 | ||
| info.value!.sortName = result; | ||
| } | ||
| } |
There was a problem hiding this comment.
sync 函数目前没有处理API调用失败的情况。如果像 EditMusicSortName 这样的API调用因为找不到资源而返回404错误（如上一个评论建议的），await会抛出异常，导致一个未处理的 promise rejection。这可能会使应用处于不稳定状态。建议在 sync 函数中添加 try-catch 块来优雅地处理这些错误，并向用户显示错误信息。
| const sync = (key: keyof MusicXmlWithABJacket, method: Function) => async () => { | |
| if (!info.value) return; | |
| info.value!.modified = true; | |
| await method(info.value.id!, info.value.assetDir, (info.value as any)[key]!); | |
| const value = (info.value as any)[key]; | |
| const result = (await method(info.value.id!, info.value.assetDir, value)).data; | |
| if (key === "sortName" && typeof result === "string" && result !== value) { | |
| // 如果调用的是sortName接口，且返回的字符串（经过格式化后的实际内容）和传过去的值不同的话，则覆盖之 | |
| info.value!.sortName = result; | |
| } | |
| } | |
| const sync = (key: keyof MusicXmlWithABJacket, method: Function) => async () => { | |
| if (!info.value) return; | |
| info.value!.modified = true; | |
| const value = (info.value as any)[key]; | |
| try { | |
| const result = (await method(info.value.id!, info.value.assetDir, value)).data; | |
| if (key === "sortName" && typeof result === "string" && result !== value) { | |
| // 如果调用的是sortName接口，且返回的字符串（经过格式化后的实际内容）和传过去的值不同的话，则覆盖之 | |
| info.value!.sortName = result; | |
| } | |
| } catch (e) { | |
| console.error(`Failed to sync ${key}:`, e); | |
| message.error(t('error.unknown')); | |
| } | |
| } |
There was a problem hiding this comment.
是不是其实也没啥，在浏览器里unhandled promise rejection本质上和把错误console.error到控制台是一样的，（又不是node， unhandled promise rejection会崩）我是懒得改了
| public string EditMusicSortName(int id, [FromBody] string value, string assetDir) | ||
| { | ||
| var music = settings.GetMusic(id, assetDir); | ||
| if (music != null) | ||
| { | ||
| music.SortName = value; | ||
| return music.SortName; // 因为music.SortName的setter中会对内容做格式化、确保符合要求。所以把转化的结果返回前端供前端更新。 | ||
| } | ||
|
|
||
| return ""; | ||
| } |
There was a problem hiding this comment.
The assetDir parameter is directly obtained from the route and used to retrieve music data. This is highly susceptible to a Path Traversal vulnerability, as an attacker could use .. sequences to access or manipulate files outside the intended directory. Additionally, when a music track is not found, the API currently returns an empty string "". This can lead to unexpected frontend behavior, potentially clearing the sortName field. It's more RESTful and robust to return NotFound() (HTTP 404) to clearly indicate that the resource was not found. This change will also require adding a try-catch block in the frontend's sync function to handle API errors.
public ActionResult<string> EditMusicSortName(int id, [FromBody] string value, string assetDir)
{
// TODO: Implement validation for 'assetDir' to prevent Path Traversal vulnerability.
var music = settings.GetMusic(id, assetDir);
if (music != null)
{
music.SortName = value;
return music.SortName; // 因为music.SortName的setter中会对内容做格式化、确保符合要求。所以把转化的结果返回前端供前端更新。
}
return NotFound();
}
1 participant
支持前端显示和编辑sortName，编辑的sortName会被自动处理确保符合要求。
PS：动机：我自己写洛天依的中文歌比如《蝴蝶》，然后会被MCM自动生成sortName为
コチョウ，有点难绷x 现在我至少可以导入后自己手动改为HUDIE了。