fix: return ERR_PERMISSION when export fails due to permission denied#464
fix: return ERR_PERMISSION when export fails due to permission denied#464Spockkk0225 wants to merge 8 commits into
Conversation
- Detect 'Permission denied' in OpenOutputStream error and return ERR_PERMISSION (1001) instead of ERR_IO_ERROR (1014) in writer.cc - Throw PermissionDeniedException in csv_export_function.cpp when status code is ERR_PERMISSION - Fix wrong indent in test_export_no_permission (assert was inside the pytest.raises context manager) Closes #428
| StatusCode::ERR_IO_ERROR, | ||
| "Failed to open file stream: " + stream_result.status().ToString()); | ||
| auto err_msg = stream_result.status().ToString(); | ||
| auto code = err_msg.find("Permission denied") != std::string::npos |
There was a problem hiding this comment.
I think this err_msg.find() is very ambiguous. @copilot Can you help review this one? and propose possible solutions?
There was a problem hiding this comment.
full error message:
"message": "Failed to open file stream: IOError: Failed to open local file '/tmp/pytest-of-shaoyu/pytest-25/test_export_no_permission0/no_perm/out.csv'. Detail: [errno 13] Permission denied"
There was a problem hiding this comment.
On the current branch this no longer relies on err_msg.find(). It now uses arrow::internal::ErrnoFromStatus(stream_result.status()) and only maps EACCES/EPERM to ERR_PERMISSION, which makes the check much more explicit than substring matching. If we want to make it even clearer, the next step would be to extract that into a small helper like ClassifyOpenOutputStreamError(status) and reuse it across export paths.
longbinlai
left a comment
There was a problem hiding this comment.
Review: err_msg.find("Permission denied") 的问题
同意 @longbinlai 的判断——用字符串匹配来分类错误码非常脆弱:
- 依赖 Arrow 的 message 文本:如果 Arrow 升级后措辞变了(比如改成 "permission denied" 小写,或者本地化),这个检查就失效了
- 跨平台不可靠:Windows 上的 permission error message 与 POSIX 不同
- 误匹配风险:如果路径名恰好包含 "Permission denied" 字样也会命中
建议方案:使用 Arrow 的 ErrnoFromStatus API
Arrow 18 的 arrow::internal::ErrnoFromStatus(status) 可以直接从 status 的 StatusDetail 中提取 errno 值,无需解析字符串:
#include <arrow/util/io_util.h>
auto stream_result = fileSystem_->OpenOutputStream(schema_.paths[0]);
if (!stream_result.ok()) {
auto& st = stream_result.status();
int err = arrow::internal::ErrnoFromStatus(st);
auto code = (err == EACCES || err == EPERM)
? StatusCode::ERR_PERMISSION
: StatusCode::ERR_IO_ERROR;
return neug::Status(code, "Failed to open file stream: " + st.ToString());
}EACCES(13) 覆盖文件权限拒绝EPERM(1) 覆盖操作不允许(如只读文件系统)ErrnoFromStatus在 detail 不是 errno 类型时返回 0,安全地 fallback 到ERR_IO_ERROR- 跨平台友好,Arrow 在 Windows 上也有对应的
WinErrorFromStatus
这个方案是结构化的错误传播,不依赖任何字符串格式。
Summary
Fix wrong error code when COPY TO fails due to permission denied.
Problem
When exporting data to a directory without write permission, the error code returned is
ERR_IO_ERROR(1014) instead ofERR_PERMISSION(1001). Additionally, theassertstatement intest_export_no_permissionwas incorrectly indented inside thepytest.raisescontext manager.See #428 for details.
Changes
src/utils/writer/writer.cc: DetectPermission deniedinOpenOutputStreamerror message and returnERR_PERMISSION(1001) instead ofERR_IO_ERROR(1014)src/compiler/function/csv_export_function.cpp: ThrowPermissionDeniedExceptionwhen status code isERR_PERMISSIONtools/python_bind/tests/test_db_import_export.py: Fix wrong indent — moveassertoutside thepytest.raisescontext managerCloses #428