优化代码，增加 Ruby4 CI#245
Conversation
- 删除 Config 中的 multi_region 选项，HostManager 始终使用多区域 hosts - 调整 Auth 模块中类方法的位置，避免存在多处 `class << self; ... end` - 测试覆盖率工具由 codecov 切换为 simplecov-cobertura，codecov 已不再维护 - 新增 base64 运行时依赖，Ruby 3.4 已经将 base64 从标准库中移除 - CI 增加 Ruby 4.0 测试矩阵 - 测试使用的 bucket 支持通过环境变量配置
![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 removes the multi_region configuration flag, making multi-region support the default behavior across the SDK. Key changes include refactoring HostManager to remove conditional logic, updating gemspec dependencies (adding base64 and transitioning to simplecov-cobertura), and modernizing the test suite to use environment-defined bucket names. Feedback identifies several reliability issues in the refactored HostManager methods, specifically the removal of bucket presence checks and the use of fragile inline rescue logic, which could lead to NoMethodError or unhandled exceptions if API responses are incomplete.
| def up_host(bucket, opts = {}) | ||
| if !multi_region_support? | ||
| "#{extract_protocol(opts)}://up.qiniup.com" | ||
| elsif bucket | ||
| host = hosts(bucket) | ||
| "#{extract_protocol(opts)}://" + host.dig('up', 'acc', 'main', 0) rescue "#{extract_protocol(opts)}://" + host.dig('up', 'src', 'main', 0) | ||
| end | ||
| host = hosts(bucket) | ||
| "#{extract_protocol(opts)}://" + host.dig('up', 'acc', 'main', 0) rescue "#{extract_protocol(opts)}://" + host.dig('up', 'src', 'main', 0) | ||
| end |
There was a problem hiding this comment.
移除 bucket 检查并简化逻辑引入了回归风险和潜在的运行时错误：
- 回归风险：以前如果
bucket为nil，该方法会返回nil。现在它会调用hosts(nil)，这可能会触发 API 错误并抛出异常。 - 脆弱的 Rescue 逻辑：行内的
rescue仅能处理第一个dig返回nil时（导致与字符串相加产生TypeError）的情况。如果第二个dig也返回nil，产生的TypeError将不会被捕获，从而导致程序崩溃。 - 健壮性：建议使用字符串插值并提供显式的回退逻辑，这样更安全且代码意图更清晰。
def up_host(bucket, opts = {})
return nil if bucket.nil?
host = hosts(bucket)
up_host = host.dig('up', 'acc', 'main', 0) || host.dig('up', 'src', 'main', 0)
"#{extract_protocol(opts)}://#{up_host}" if up_host
end| def fetch_host(bucket, opts = {}) | ||
| if !multi_region_support? | ||
| "#{extract_protocol(opts)}://iovip.qbox.me" | ||
| elsif bucket | ||
| host = hosts(bucket) | ||
| "#{extract_protocol(opts)}://" + host.dig('io', 'acc', 'main', 0) rescue "#{extract_protocol(opts)}://" + host.dig('io', 'src', 'main', 0) | ||
| end | ||
| host = hosts(bucket) | ||
| "#{extract_protocol(opts)}://" + host.dig('io', 'acc', 'main', 0) rescue "#{extract_protocol(opts)}://" + host.dig('io', 'src', 'main', 0) | ||
| end |
There was a problem hiding this comment.
与 up_host 类似，此方法现在缺少对 bucket 是否为 nil 的检查，并且使用了脆弱的 rescue 逻辑。如果 API 响应中同时缺少 acc 和 src 主机配置，程序将会崩溃。
def fetch_host(bucket, opts = {})
return nil if bucket.nil?
host = hosts(bucket)
fetch_host = host.dig('io', 'acc', 'main', 0) || host.dig('io', 'src', 'main', 0)
"#{extract_protocol(opts)}://#{fetch_host}" if fetch_host
end| def up_hosts(bucket, opts = {}) | ||
| if multi_region_support? | ||
| host = hosts(bucket)['up'] | ||
| multi_region_hosts = [] | ||
| multi_region_hosts |= host.dig('acc', 'main') || [] | ||
| multi_region_hosts |= host.dig('src', 'main') || [] | ||
| return multi_region_hosts | ||
| else | ||
| raise 'HostManager#up_hosts: multi_region must be enabled' | ||
| end | ||
| host = hosts(bucket)['up'] | ||
| up_hosts = [] | ||
| up_hosts |= host.dig('acc', 'main') || [] | ||
| up_hosts |= host.dig('src', 'main') || [] | ||
| up_hosts | ||
| end |
There was a problem hiding this comment.
如果 hosts(bucket) 的响应不包含 'up' 键，或者 bucket 为 nil，此方法会抛出 NoMethodError（因为 host 为 nil）。建议增加卫语句并为 host 变量提供默认的空哈希以提高代码的健壮性。
def up_hosts(bucket, opts = {})
return [] if bucket.nil?
host = hosts(bucket)['up'] || {}
up_hosts = []
up_hosts |= host.dig('acc', 'main') || []
up_hosts |= host.dig('src', 'main') || []
up_hosts
end| def global(bucket, opts = {}) | ||
| if multi_region_support? | ||
| !!hosts(bucket)['global'] | ||
| else | ||
| raise 'HostManager#global: multi_region must be enabled' | ||
| end | ||
| !!hosts(bucket)['global'] | ||
| end |
There was a problem hiding this comment.
建议为 bucket 增加卫语句，以避免在缺少存储桶名称时进行不必要的 API 调用，并防止潜在的异常抛出。
def global(bucket, opts = {})
return false if bucket.nil?
!!hosts(bucket)['global']
end![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| host = hosts(bucket) | ||
| "#{extract_protocol(opts)}://" + host.dig('up', 'acc', 'main', 0) rescue "#{extract_protocol(opts)}://" + host.dig('up', 'src', 'main', 0) | ||
| end | ||
| host = hosts(bucket) |
There was a problem hiding this comment.
up_host and fetch_host used to short-circuit when bucket was nil (elsif bucket in the old implementation). After this change they always call hosts(bucket), so Config.up_host(nil)/fetch_host(nil) now issues a UC query with an empty bucket and raises Host query is failed instead of returning nil (or the legacy fixed host when multi_region was disabled). Since these methods are public API, this is a breaking behavior change unless you add an explicit guard or raise a clearer argument error.
| BUCKET_NA0: ${{ secrets.BUCKET_NA0 }} | ||
| BUCKET_BC: ${{ secrets.BUCKET_BC }} | ||
|
|
||
| - name: After_success |
There was a problem hiding this comment.
The PR switches the test stack from codecov to simplecov-cobertura, but CI still runs bash <(curl -s https://codecov.io/bash). That means the workflow still depends on the deprecated Codecov uploader, and the newly generated Cobertura report is not actually consumed anywhere. If the intent is to stop relying on Codecov, this step needs to be updated or removed as part of the same change.
| pp = Auth::PutPolicy.new(@bucket, key) | ||
| expect(pp.instance_variable_get(:@global)).to be false | ||
| uphosts = pp.instance_variable_get(:@uphosts) | ||
| expect(uphosts.size).to eq 2 |
There was a problem hiding this comment.
This assertion hard-codes the host list size to exactly 2 even though the API contract here is really “contains at least one upload-* host and one up-* host.” If UC starts returning additional upload endpoints for the bucket, the test will fail even though the SDK behavior is still correct. Dropping the exact-size check would make this integration spec much less brittle.
1 participant
class << self; ... end