Skip to content

optimize permission dependencies#521

Open
YangSen-qn wants to merge 2 commits intoqiniu:masterfrom
YangSen-qn:master
Open

optimize permission dependencies#521
YangSen-qn wants to merge 2 commits intoqiniu:masterfrom
YangSen-qn:master

Conversation

@YangSen-qn
Copy link
Copy Markdown
Contributor

No description provided.

@qiniu-prow qiniu-prow bot added the size/L label Apr 9, 2026
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request downgrades the project JDK to 1.8, updates dependencies, and refactors network type detection in AndroidNetwork.java to support modern Android APIs while reducing the required permission footprint. Feedback identifies a potential NullPointerException due to a missing null check on the context object, a regression in WIFI detection for devices below API 23, and the use of an overly broad Throwable catch block.

Copy link
Copy Markdown

@xgopilot xgopilot bot left a comment

Choose a reason for hiding this comment

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

This PR is a solid improvement — it modernises the network detection API, reduces declared permissions, and structures the tiered API-level strategy clearly. A few correctness and accuracy issues are worth addressing before merge.

@YangSen-qn
Copy link
Copy Markdown
Contributor Author

@xgopilot /compact

@xgopilot
Copy link
Copy Markdown

xgopilot bot commented Apr 9, 2026

Code Review — PR #521: optimize permission dependencies

Overall the direction is correct: removing over-broad permissions (ACCESS_WIFI_STATE, READ_EXTERNAL_STORAGE, WRITE_EXTERNAL_STORAGE) and adopting NetworkCapabilities for API 29+ is the right approach. Found a few issues below.


1. .idea/misc.xml should not be in this PR

library/src/main/AndroidManifest.xml is fine, but .idea/misc.xml changes the IDE project JDK from liberica-11liberica-1.8. The project toolchain uses Java 11 for compilation (confirmed in build config), so this is a local dev environment change that shouldn't be committed. It will cause problems for contributors using Java 11 toolchains. This file should be in .gitignore or reverted.


2. catch (Throwable t) is too broadAndroidNetwork.java:100

} catch (Throwable t) {
    t.printStackTrace();
    return null;
}

Throwable catches Error subclasses (OutOfMemoryError, StackOverflowError, AssertionError), which should not be silently swallowed. The original catch (Exception e) was appropriate here and should be kept.


3. VPN transport returns NETWORK_CLASS_UNKNOWN ("none")AndroidNetwork.java:204-206

if (caps.hasTransport(NetworkCapabilities.TRANSPORT_VPN)) {
    return Constants.NETWORK_CLASS_UNKNOWN;
}

Returning "none" for VPN-connected devices means users tunneling traffic over VPN will appear as having no network. A VPN connection always underlies a physical transport (WIFI or CELLULAR), so you can check both:

if (caps.hasTransport(NetworkCapabilities.TRANSPORT_VPN)) {
    // VPN routes through an underlying physical transport
    if (caps.hasTransport(NetworkCapabilities.TRANSPORT_WIFI)) {
        return Constants.NETWORK_WIFI;
    }
    return Constants.NETWORK_CLASS_MOBILE;
}

Or simply fall through to return Constants.NETWORK_CLASS_UNKNOWN at the end without the explicit VPN check (VPN-only without wifi/cellular already falls through correctly).


4. isNetWorkReady() still uses deprecated NetworkInfoAndroidNetwork.java:44

NetworkInfo info = connMgr.getActiveNetworkInfo();
return info != null && info.isConnected();

NetworkInfo and getActiveNetworkInfo() are deprecated since API 29. Since this PR is already migrating networkType() to NetworkCapabilities, isNetWorkReady() should follow the same pattern for consistency — otherwise the deprecation warning remains. This can be addressed in a follow-up PR.

View job run

@YangSen-qn YangSen-qn force-pushed the master branch 4 times, most recently from 160b0d9 to 46bc5be Compare April 13, 2026 07:09
@YangSen-qn
Copy link
Copy Markdown
Contributor Author

@xgopilot 重新 Review 下

@xgopilot
Copy link
Copy Markdown

xgopilot bot commented Apr 13, 2026

Code Review — PR #521: optimize permission dependencies(Re-review)

上次 Review 指出的问题均已修复:

  • .idea/ 文件已全部移出 PR ✅
  • catch (Throwable t) 已改回 catch (Exception e)
  • isNetWorkReady() 已在 API 23+ 使用 NetworkCapabilities
  • 存储权限(READ_EXTERNAL_STORAGE/WRITE_EXTERNAL_STORAGE)已移除 ✅
  • READ_PHONE_STATE 限制为 maxSdkVersion="28"

遗留问题:NETWORK_TYPE_NR 是 API 29+ 常量,出现在只被 API < 29 调用的方法中AndroidNetwork.java:178

case TelephonyManager.NETWORK_TYPE_NR:
    return Constants.NETWORK_CLASS_5_G;

getNetworkTypeByTelephony() 只在 getNetWorkClass() 的 API < 29 路径下被调用(API >= Q 直接走 getNetworkClassByConnectivity 提前返回)。NETWORK_TYPE_NR 是 API 29 引入的常量,在 API < 29 设备上 getNetworkType() 不会返回该值,这条 case 实际是死代码。编译不会出错(常量值 20 在低版本也可用),但语义上容易引起误解。可以考虑删除该 case,或在注释中说明原因。


整体来看代码质量明显提升,权限依赖合理收窄,回退逻辑也清晰。除上述一处可选优化外,无其他阻塞性问题,可以合并。

View job run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant