Skip to content

fix: return ERR_PERMISSION when export fails due to permission denied#464

Open
Spockkk0225 wants to merge 8 commits into
mainfrom
fix/wrong-error-code-for-permission-428
Open

fix: return ERR_PERMISSION when export fails due to permission denied#464
Spockkk0225 wants to merge 8 commits into
mainfrom
fix/wrong-error-code-for-permission-428

Conversation

@Spockkk0225
Copy link
Copy Markdown
Collaborator

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 of ERR_PERMISSION (1001). Additionally, the assert statement in test_export_no_permission was incorrectly indented inside the pytest.raises context manager.

See #428 for details.

Changes

  • src/utils/writer/writer.cc: Detect Permission denied in OpenOutputStream error message and return ERR_PERMISSION (1001) instead of ERR_IO_ERROR (1014)
  • src/compiler/function/csv_export_function.cpp: Throw PermissionDeniedException when status code is ERR_PERMISSION
  • tools/python_bind/tests/test_db_import_export.py: Fix wrong indent — move assert outside the pytest.raises context manager

Closes #428

- 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
@Spockkk0225 Spockkk0225 requested a review from lnfjpt June 3, 2026 06:38
lnfjpt
lnfjpt previously approved these changes Jun 3, 2026
Copy link
Copy Markdown
Collaborator

@lnfjpt lnfjpt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread src/utils/writer/writer.cc Outdated
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
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this err_msg.find() is very ambiguous. @copilot Can you help review this one? and propose possible solutions?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

@longbinlai longbinlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: err_msg.find("Permission denied") 的问题

同意 @longbinlai 的判断——用字符串匹配来分类错误码非常脆弱:

  1. 依赖 Arrow 的 message 文本:如果 Arrow 升级后措辞变了(比如改成 "permission denied" 小写,或者本地化),这个检查就失效了
  2. 跨平台不可靠:Windows 上的 permission error message 与 POSIX 不同
  3. 误匹配风险:如果路径名恰好包含 "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

这个方案是结构化的错误传播,不依赖任何字符串格式。

Copilot AI requested a review from longbinlai June 3, 2026 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] wrong error code for ERR_PERMISSION

4 participants