-
Notifications
You must be signed in to change notification settings - Fork 324
[fit] Use Set-Cookie header instead of Cookie in HTTP responses #335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 3.5.x
Are you sure you want to change the base?
Conversation
… attribute constants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
检视意见
我对这个PR进行了详细的代码检视。总体来说,这是一个高质量的Bug修复PR,核心逻辑正确,测试覆盖充分。
✅ 优点
- 问题定位准确: 正确识别了将Cookie写入
Cookie
头而非Set-Cookie
头的核心问题 - 符合RFC标准: 实现遵循RFC 6265规范,支持所有标准Cookie属性
- 测试覆盖充分: 新增14个测试用例覆盖Cookie解析、格式化、边界情况
- 向后兼容: 将
version()
和comment()
标记为@Deprecated
而非直接删除 - 代码质量高: 代码结构清晰,注释完整,符合项目规范
⚠️ 发现的问题
1. 空指针风险 (重要)
位置: DefaultCookieCollection.java:87
@Override
public void add(Cookie cookie) {
store.computeIfAbsent(cookie.name(), k -> new ArrayList<>()); // 如果cookie为null会抛NPE
// ...
}
建议: 添加空值检查
@Override
public void add(Cookie cookie) {
if (cookie == null || StringUtils.isBlank(cookie.name())) {
return;
}
// 现有逻辑...
}
2. Expires转换精度问题 (中等)
位置: HttpUtils.java:165
当Expires时间超过2038年时,long转int会溢出。
建议:
return (int) Math.min(Math.max(seconds, 0), Integer.MAX_VALUE);
3. Cookie解析鲁棒性 (轻微)
位置: HttpUtils.parseCookies()
建议增加测试用例覆盖边界情况,如引号不完整的value: a="incomplete
4. 文档完整性 (轻微)
PR checklist中"我已经更新了相应的文档"未勾选,建议补充用户文档说明Cookie处理的变更。
📋 总结
建议修复问题1、2(必要)后即可合并,问题3、4可在后续优化。
整体评价: LGTM with minor fixes 👍
已修复检视意见中的必要问题:
|
🔗 相关问题 / Related Issue
Issue 链接 / Issue Link: #307 👈👈
📋 变更类型 / Type of Change
📝 变更目的 / Purpose of the Change
当前
HttpClassicServerResponse
在设置 Cookie 时,会错误地将其写入响应头中的Cookie
字段,而不是标准的Set-Cookie
。这会导致客户端无法正确识别和存储服务端下发的 Cookie,且丢失了
Path
、Secure
、HttpOnly
等关键属性。本次变更修复了该问题,使响应中每个 Cookie 都以单独的
Set-Cookie
header 下发,并正确包含所有属性。📋 主要变更 / Brief Changelog
🧪 验证变更 / Verifying this Change
测试步骤 / Test Steps
测试覆盖 / Test Coverage
📸 截图 / Screenshots
修改前:


修改后:
✅ 贡献者检查清单 / Contributor Checklist
请确保你的 Pull Request 符合以下要求 / Please ensure your Pull Request meets the following requirements:
基本要求 / Basic Requirements:
代码质量 / Code Quality:
测试要求 / Testing Requirements:
mvn -B clean package -Dmaven.test.skip=true
,elsa README 中的编译检查 / Basic checks passmvn clean install
/ Unit tests pass文档和兼容性 / Documentation and Compatibility:
📋 附加信息 / Additional Notes
审查者注意事项 / Reviewer Notes: