-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
feat: uri encode processing for attachment paths when querying attachments #1874
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
PS:该改动仅对新上传的内容有效,之前的错误数据并不会被修正 |
将生成链接中的文件名进行 urlencode 似乎更合理一些。 |
对文件名#% 进行 urlencode 之后,在文章引用的过程中,%会被二次urlencode,导致引用的错误 |
个人认为这样改可能会有点粗暴,如果说再有另外的字符也会导致此问题怎么办呢。 /cc @halo-dev/sig-halo |
根据本人测试以及之前的issue中提到,暂时只发现这两个字符的问题 |
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.
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.
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.
考虑到兼容性问题,应该在此行对文件名进行 url encoding 处理,即只对返回结果编码
halo/src/main/java/run/halo/app/service/impl/AttachmentServiceImpl.java
Lines 177 to 180 in 913002d
String fullPath = StringUtils | |
.join(enabledAbsolutePath ? blogBaseUrl : "", "/", attachmentDTO.getPath()); | |
String fullThumbPath = StringUtils | |
.join(enabledAbsolutePath ? blogBaseUrl : "", "/", attachmentDTO.getThumbPath()); |
可以如下操作
// 将 local 存储的链接中的文件名替换为编码后的文件名
String path = attachmentDTO.getPath()
.replace(attachmentDTO.getName(), encodeValue(attachmentDTO.getName()));
// ....
// 附件缩略图亦如是
String basename = FilenameUtils.getBasename(attachmentDTO.getName());
String extension = FilenameUtils.getExtension(attachmentDTO.getName());
// 得到 thumbail name
String thumbnailName = String.format("%s-thumbnail%s", basename, extension);
String thumbnailPath = attachmentDTO.getThumbPath()
.replace(thumbnailName, encodeValue(thumbnailName));
// ...
private String encodeValue(String value) {
return UriUtils.encode(value, StandardCharsets.UTF_8);
}
以上代码中关于缩略图文件名获取的方式需要考虑:后续如果缩略图规则改变不应该影响到此处功能,所以要确保本地上传策略生存缩略图和此处规则一致
如果后端对此做了修改,那么前端所有编码的位置都需要移除。 |
Hi @FanZeros 有兴趣尝试一下这种方法么:#1874 (review) |
好的,我研究研究 |
暂时改动是对fullPath进行url encoding 处理,但thumbnailPath 不处理 前端部分进行如下修改: https://github.com/halo-dev/halo-admin/blob/master/src/components/Attachment/AttachmentSelectModal.vue#L262 不过发现一个问题:encodeURI() 不会对#进行编码,但是java的UriUtils.encode() 会对#进行编码 |
只需要后端通过 UriUtils.encode() 对文件名部分进行编码即可 然后把前端编码的部分删除统一由后端处理应该就可以解决问题了 |
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.
Hi @FanZeros ,我的建议也是在构建完整连接的时候,仅对 path
和 thumbnailPath
进行 encode。所有在 halo-admin encodeURL 的地方都需要移除,否则会造成多次 encode,无法正常访问。
halo/src/main/java/run/halo/app/service/impl/AttachmentServiceImpl.java
Lines 177 to 180 in 913002d
String fullPath = StringUtils | |
.join(enabledAbsolutePath ? blogBaseUrl : "", "/", attachmentDTO.getPath()); | |
String fullThumbPath = StringUtils | |
.join(enabledAbsolutePath ? blogBaseUrl : "", "/", attachmentDTO.getThumbPath()); |
嗯,我昨晚在尝试你们说的方法,在得到的缩略图链接处,去除encodeURI之后会在年月的“/”出现奇怪的% 理论值 upload/2022\05\2%23%252甘雨-1652370784582-thumbnail.png 以及123.png 暂不清楚该情况是怎么发生的,还在进一步研究中 |
halo/src/main/java/run/halo/app/service/impl/AttachmentServiceImpl.java Lines 177 to 180 in 913002d
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JohnNiang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JohnNiang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: guqing, JohnNiang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
New changes are detected. LGTM label has been removed. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: guqing, JohnNiang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@FanZeros 好像有 checkstyle 的错误哦 https://github.com/halo-dev/halo/pull/1874/checks 设置方法可以看这个 https://docs.halo.run/developer-guide/core/code-style |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: guqing, JohnNiang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
src/test/java/run/halo/app/service/impl/AttachmentServiceImplTest.java
Outdated
Show resolved
Hide resolved
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: guqing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
当前该测试用例貌似并不能在全平台上成功运行,所以废弃
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: guqing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: guqing, JohnNiang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
/lgtm
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: guqing, JohnNiang, ruibaby The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @FanZeros ,非常感谢花时间修复此功能。在最后,请点击 签署成功后,PR 会自动合并。 |
@JohnNiang: new pull request created: #2094 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What this PR does?
在上传文件名带有特殊字符(#%)的文件时,在生成链接的过程中,会使用相近的符号代替它们
避免之后在使用或者查看的时候因为转码问题而找不到对应资源
Which issue(s) this PR fixes:
Fixes #1205
Does this PR introduce a user-facing change?