哒哒#1
Conversation
… MetingAPI 的兼容错误，增加可用指令，允许在不切换会话默认音乐源时直接调用对应音乐源，BUG 修复
There was a problem hiding this comment.
Pull request overview
This pull request updates the MetingAPI music plugin to version 1.2.2, removing support for Kugou and Kuwo music sources while adding support for QQ Music cards and PHP-based MetingAPI. The changes significantly simplify the codebase by removing extensive error handling, URL validation, and resource management logic, though this introduces several critical bugs and security vulnerabilities.
Changes:
- Added support for PHP-based MetingAPI (configurable via
api_type) - Added QQ Music card feature using JSON message components with signature API
- Removed support for Kugou and Kuwo music sources
- Simplified download and audio processing logic, removing most error handling and security checks
- Added new command aliases for source switching and direct platform-specific search commands
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 23 comments.
| File | Description |
|---|---|
| main.py | Major refactoring: Added music card support, removed security validations (SSRF protection), simplified error handling, removed resource cleanup mechanisms, and added new command handlers |
| _conf_schema.json | Added configuration options for api_type, use_music_card, and api_sign_url; removed Kugou and Kuwo from hints |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async def _get_session(self, session_id: str) -> SessionData: | ||
| """获取会话状态（线程安全） | ||
|
|
||
| Args: | ||
| session_id: 会话 ID | ||
|
|
||
| Returns: | ||
| SessionData: 会话状态对象 | ||
| """ | ||
| await self._ensure_initialized() | ||
| async with self._sessions_lock: | ||
| if session_id not in self._sessions: | ||
| self._sessions[session_id] = SessionData(self.get_default_source()) | ||
| default_source = str(self._get_config("default_source", "netease")) | ||
| self._sessions[session_id] = SessionData(default_source) | ||
| return self._sessions[session_id] |
There was a problem hiding this comment.
Session cleanup logic has been removed. The old code had periodic cleanup of old sessions (older than MAX_SESSION_AGE) to prevent memory leaks from abandoned sessions. The new code removes this cleanup task entirely, meaning sessions will accumulate in memory indefinitely.
Re-implement periodic session cleanup or add a mechanism to remove stale sessions to prevent memory leaks in long-running instances.
| def _get_config(self, key: str, default=None): | ||
| if not self.config: | ||
| return default | ||
|
|
||
| value = self.config.get(key, default) | ||
| if validator is not None and not validator(value): | ||
| return default | ||
|
|
||
| return value | ||
| return self.config.get(key, default) |
There was a problem hiding this comment.
The _get_config method no longer performs any validation on configuration values. The old code had a validator parameter to check if values were within acceptable ranges and types. Now any value from the config is used directly, which could cause runtime errors if users provide invalid values.
For example, search_result_count could be negative or non-integer, segment_duration could be 0 or negative, etc. Add validation to ensure configuration values are within acceptable ranges and types.
| with open(stage.__file__, "r", encoding="utf-8") as f: | ||
| content = f.read() | ||
| if "Comp.Json" not in content: | ||
| logger.warning( | ||
| "检测到当前 AstrBot 版本可能不支持 JSON 消息组件。请更新 AstrBot 版本，否则音乐卡片可能无法发送。" | ||
| ) |
There was a problem hiding this comment.
The version compatibility check is fragile and could produce false positives. Reading the stage module's source file and searching for "Comp.Json" as a string is unreliable because:
- The string search could match in comments or string literals
- Module refactoring could change the implementation without changing functionality
- Reading source files directly is not a standard way to check API compatibility
Consider using hasattr() to check if the required functionality exists at runtime, or comparing version numbers if the framework provides version information.
| with open(stage.__file__, "r", encoding="utf-8") as f: | |
| content = f.read() | |
| if "Comp.Json" not in content: | |
| logger.warning( | |
| "检测到当前 AstrBot 版本可能不支持 JSON 消息组件。请更新 AstrBot 版本，否则音乐卡片可能无法发送。" | |
| ) | |
| comp = getattr(stage, "Comp", None) | |
| has_json_support = comp is not None and hasattr(comp, "Json") | |
| if not has_json_support: | |
| logger.warning( | |
| "检测到当前 AstrBot 版本可能不支持 JSON 消息组件。请更新 AstrBot 版本，否则音乐卡片可能无法发送。" | |
| ) |
| async def _download_song(self, url: str) -> str | None: | ||
| temp_path = get_astrbot_temp_path() | ||
| if not os.path.exists(temp_path): | ||
| os.makedirs(temp_path) | ||
|
|
||
| Args: | ||
| url: 歌曲 URL | ||
| sender_id: 发送者 ID | ||
| temp_file = os.path.join(temp_path, f"{TEMP_FILE_PREFIX}{uuid.uuid4()}.tmp") | ||
|
|
||
| Returns: | ||
| str | None: 临时文件路径，失败返回 None | ||
| """ | ||
| if not self._http_session: | ||
| raise DownloadError("HTTP session 未初始化") | ||
|
|
||
| temp_dir = tempfile.gettempdir() | ||
| safe_sender_id = "".join(c for c in str(sender_id) if c.isalnum() or c in "._-") | ||
|
|
||
| download_success = False | ||
| max_retries = 3 | ||
| retry_count = 0 | ||
| temp_file = None | ||
| detected_format = None | ||
|
|
||
| while retry_count < max_retries: | ||
| try: | ||
| async with self._download_semaphore: | ||
| logger.debug( | ||
| f"开始下载歌曲 (尝试 {retry_count + 1}/{max_retries}): {url}" | ||
| ) | ||
|
|
||
| current_url = url | ||
| redirect_count = 0 | ||
| max_redirects = 5 | ||
|
|
||
| while redirect_count < max_redirects: | ||
| is_valid, reason = await self._validate_url(current_url) | ||
| if not is_valid: | ||
| raise UnsafeURLError(f"URL 验证失败: {reason}") | ||
|
|
||
| async with self._http_session.get( | ||
| current_url, allow_redirects=False | ||
| ) as resp: | ||
| if resp.status in (301, 302, 307, 308): | ||
| redirect_url = resp.headers.get("Location", "") | ||
| if not redirect_url: | ||
| raise DownloadError("重定向响应缺少 Location 头") | ||
|
|
||
| current_url = urljoin(current_url, redirect_url) | ||
| logger.debug(f"跟随重定向: {current_url}") | ||
| redirect_count += 1 | ||
| continue | ||
|
|
||
| if resp.status != 200: | ||
| raise DownloadError(f"下载失败，状态码: {resp.status}") | ||
|
|
||
| content_type = resp.headers.get("Content-Type", "") | ||
| if not self._is_audio_content(content_type): | ||
| raise AudioFormatError( | ||
| f"不支持的 Content-Type: {content_type}" | ||
| ) | ||
|
|
||
| max_file_size = self.get_max_file_size() | ||
| total_size = 0 | ||
| first_chunk = None | ||
| temp_file = os.path.join( | ||
| temp_dir, | ||
| f"{TEMP_FILE_PREFIX}{safe_sender_id}_{uuid.uuid4()}.tmp", | ||
| ) | ||
|
|
||
| with open(temp_file, "wb") as f: | ||
| try: | ||
| async for chunk in resp.content.iter_chunked( | ||
| CHUNK_SIZE | ||
| ): | ||
| if first_chunk is None and chunk: | ||
| first_chunk = chunk | ||
| detected_format = _detect_audio_format( | ||
| first_chunk | ||
| ) | ||
| if not detected_format: | ||
| raise AudioFormatError( | ||
| "文件头检测失败，不是有效的音频文件" | ||
| ) | ||
|
|
||
| f.write(chunk) | ||
| total_size += len(chunk) | ||
| if total_size > max_file_size: | ||
| raise DownloadError( | ||
| f"文件过大，已超过 {max_file_size} 字节" | ||
| ) | ||
| except aiohttp.ClientPayloadError as e: | ||
| raise DownloadError(f"连接中断: {e}") from e | ||
|
|
||
| file_size = os.path.getsize(temp_file) | ||
| if file_size == 0: | ||
| raise DownloadError("下载的文件为空") | ||
|
|
||
| file_ext = _get_extension_from_format(detected_format) | ||
| final_file = temp_file + file_ext | ||
| os.rename(temp_file, final_file) | ||
| temp_file = final_file | ||
|
|
||
| logger.info( | ||
| f"歌曲下载成功，文件大小: {file_size} 字节，格式: {detected_format}" | ||
| ) | ||
| download_success = True | ||
| return temp_file | ||
|
|
||
| raise DownloadError(f"重定向次数超过限制: {max_redirects}") | ||
|
|
||
| except (aiohttp.ClientError, aiohttp.ClientPayloadError) as e: | ||
| retry_count += 1 | ||
| logger.error( | ||
| f"下载歌曲时网络错误 (尝试 {retry_count}/{max_retries}): {e}" | ||
| async with self._download_semaphore: | ||
| if not self._http_session: | ||
| return None | ||
| async with self._http_session.get(url, allow_redirects=True) as resp: | ||
| if resp.status != 200: | ||
| return None | ||
| content = await resp.read() | ||
|
|
||
| detected_format = _detect_audio_format(content[:1024]) | ||
| ext_map = { | ||
| "mp3": ".mp3", | ||
| "wav": ".wav", | ||
| "ogg": ".ogg", | ||
| "flac": ".flac", | ||
| "mp4": ".m4a", | ||
| } | ||
| ext = ( | ||
| ext_map.get(str(detected_format), ".mp3") | ||
| if detected_format | ||
| else ".mp3" | ||
| ) | ||
| if retry_count >= max_retries: | ||
| raise DownloadError(f"网络错误: {e}") from e | ||
| await asyncio.sleep(1) | ||
| except (DownloadError, UnsafeURLError, AudioFormatError): | ||
| raise | ||
| except Exception as e: | ||
| logger.error(f"下载歌曲时发生错误: {e}", exc_info=True) | ||
| raise DownloadError(f"下载失败: {e}") from e | ||
| finally: | ||
| if not download_success and temp_file and os.path.exists(temp_file): | ||
| try: | ||
| os.remove(temp_file) | ||
| logger.debug("清理临时文件") | ||
| except Exception: | ||
| pass | ||
|
|
||
| return None | ||
| with open(temp_file, "wb") as f: | ||
| f.write(content) | ||
| os.rename(temp_file, temp_file + ext) | ||
| return temp_file + ext |
There was a problem hiding this comment.
The max_file_size configuration option is no longer enforced. The old code checked if total_size exceeded max_file_size during download and raised an error. The new code removes this check entirely, which means users could potentially download files much larger than the configured limit, leading to memory and storage issues.
Either remove the max_file_size configuration option if it's no longer needed, or implement size checking during the download process.
| yield event.chain_result([Record(path)]) | ||
| if os.path.exists(path): | ||
| os.remove(path) |
There was a problem hiding this comment.
The temporary segment files are deleted immediately after sending at line 378-379, but if the send operation fails or the Record component hasn't finished reading the file yet, this could cause errors. The old code was more careful about cleanup timing.
Consider ensuring the Record component has finished processing the file before deletion, or handle potential file-not-found errors gracefully.
| for i, start in enumerate(range(0, len(audio), seg_ms), 1): | ||
| path = f"{temp_file}_{i}.wav" | ||
| audio[start : start + seg_ms].export(path, format="wav") | ||
| yield event.chain_result([Record(path)]) |
There was a problem hiding this comment.
The Record component is instantiated incorrectly. Based on the import statement at line 13, Record is imported from astrbot.api.message_components. However, at line 377, the code uses Record(path) which assumes the constructor takes a file path directly.
Looking at the old code that was removed, it used Record.fromFileSystem(segment_file). The new code should verify the correct way to instantiate Record with a file path.
| yield event.chain_result([Record(path)]) | |
| yield event.chain_result([Record.fromFileSystem(path)]) |
| async with self._http_session.get(url, allow_redirects=True) as resp: | ||
| if resp.status != 200: | ||
| return None | ||
| content = await resp.read() |
There was a problem hiding this comment.
Memory leak: Downloaded song files are loaded entirely into memory with await resp.read(). For large audio files (up to 50MB by default configuration), this can consume significant memory. Multiple concurrent downloads could exhaust available memory.
Consider using streaming download with resp.content.iter_chunked() to write chunks directly to disk instead of loading the entire file into memory first.
| with open(temp_file, "wb") as f: | ||
| f.write(content) | ||
| os.rename(temp_file, temp_file + ext) | ||
| return temp_file + ext |
There was a problem hiding this comment.
Temporary file not cleaned up on download error. If an exception occurs after the file is created at line 352-353 but before the successful return at line 355 (e.g., os.rename fails), the temp_file will be left on disk. Over time, this could accumulate significant disk space usage.
Wrap the file operations in a try-finally block to ensure cleanup, or use a context manager to handle the temporary file.
| with open(temp_file, "wb") as f: | |
| f.write(content) | |
| os.rename(temp_file, temp_file + ext) | |
| return temp_file + ext | |
| final_path = None | |
| try: | |
| with open(temp_file, "wb") as f: | |
| f.write(content) | |
| final_path = temp_file + ext | |
| os.rename(temp_file, final_path) | |
| return final_path | |
| finally: | |
| if final_path is None and os.path.exists(temp_file): | |
| os.remove(temp_file) |
| self._http_session = aiohttp.ClientSession( | ||
| timeout=REQUEST_TIMEOUT, | ||
| headers={ | ||
| "Referer": "https://astrbot.app/", | ||
| "User-Agent": f"AstrBot/{VERSION}", | ||
| "UAK": "AstrBot/plugin_meting", | ||
| }, | ||
| ) | ||
| self._initialized = True |
There was a problem hiding this comment.
The terminate() method has been completely removed. This means the HTTP session will not be properly closed when the plugin is stopped, potentially leaving open connections and causing resource leaks. The old code had a terminate() method that closed the session and cleaned up resources.
Implement a terminate() or cleanup method to properly close the aiohttp session and release resources when the plugin is stopped.
| async def _ensure_initialized(self): | ||
| """确保插件已初始化（惰性初始化）""" | ||
| if self._initialized: | ||
| return | ||
| self._http_session = aiohttp.ClientSession( | ||
| timeout=REQUEST_TIMEOUT, | ||
| headers={ | ||
| "Referer": "https://astrbot.app/", | ||
| "User-Agent": f"AstrBot/{VERSION}", | ||
| "UAK": "AstrBot/plugin_meting", | ||
| }, | ||
| ) | ||
| self._initialized = True |
There was a problem hiding this comment.
Potential race condition in session initialization. The _ensure_initialized method checks self._initialized without a lock at line 92, then only acquires the sessions_lock later. If two requests come in simultaneously, both could pass the first check and attempt initialization. While the second request won't re-initialize due to the second check, this pattern is fragile.
Use a dedicated initialization lock (as the old code did with _init_lock) to ensure thread-safe initialization, or use asyncio.Lock properly around the entire initialization check and process.
Warning
提醒
· 该版本移除了对 酷我音乐 和 酷狗音乐 的支持。
⚙️ 更改