更新分类(Category)的实现有问题

[1379]

What is version of Halo has the issue?

latest

What database are you using?

MySQL 8.x

What is your deployment method?

Fat Jar

Your site address.

无

What happened?

halo/src/main/java/run/halo/app/service/impl/CategoryServiceImpl.java

Line 556 in 631b214

collectAllChildByCategoryId(collectorList,

这里的实现有问题，如果当前分类不是顶级分类的话，collectAllChildByCategoryId这个方法里面的for循环不会处理到当前分类

Relevant log output

No response

Additional information

No response

[JohnNiang]

有兴趣修复么😁

[1379]

如果确定是个bug，明天可以我提个pr

…

---原始邮件--- 发件人: "John ***@***.***> 发送时间: 2022年2月22日(周二) 晚上11:44 收件人: ***@***.***>; 抄送: ***@***.******@***.***>; 主题: Re: [halo-dev/halo] 更新分类(Category)的实现有问题 (Issue #1673) 有兴趣修复么😁 — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you authored the thread.Message ID: ***@***.***>

[guqing]

hi @1379,感谢你对此提出issue，该方法的确存在问题，如果你有兴趣修复的话请继续阅读：
在这之前我修复一个问题的过程中也发现了该代码的问题，因此我在代码中写了一个

halo/src/main/java/run/halo/app/service/impl/CategoryServiceImpl.java

Line 333 in 631b214

private Optional<CategoryVO> findCategoryTreeNodeById(List<CategoryVO> categoryVos,

方法来替代这件事，但由于该块代码可读性不是很好且没有良好的测试所以之前并没有替换它，你可以尝试使用 findCategoryTreeNodeById 方法替换它，可能存在需要过滤加密分类的问题，我的建议是分为两个步骤做：

1. 先根据分类id获取子分类
2. 判断是否需要过滤加密分类（加密分类的过滤其实不应该在service或不应该是现在这样处理做但由于历史原因，遵循该逻辑即可，我们会在后续版本重构它）

对于 步骤 2 如果你需要可以将

halo/src/main/java/run/halo/app/service/impl/CategoryServiceImpl.java

Line 307 in 631b214

private List<Category> walkCategoryTree(CategoryVO root) {

改为模版方法即下面的形式

private List<Category> walkCategoryTree(CategoryVO root, Consumer<CategoryVO> consumer) {
        Assert.notNull(root, "The category 'root' must not be null");
        List<Category> categories = new LinkedList<>();
        Queue<CategoryVO> queue = new ArrayDeque<>();
        queue.add(root);
        while (!queue.isEmpty()) {
            CategoryVO categoryNode = queue.poll();

            consumer.accept(category);

            if (HaloUtils.isNotEmpty(categoryNode.getChildren())) {
                queue.addAll(categoryNode.getChildren());
            }
        }
        return categories;
    }

当然这一切重构的前提是建议你先写测试类，使其在原先代码上能顺利通过，然后在小步重构代码伴随反复运行测试类，与此往复。还请小心为之。

[1379]

感谢guqing大佬，既然已经修复了，那我没问题啦

[guqing]

感谢 guqing 大佬，既然已经修复了，那我没问题啦

@1379 你理解错了 我想表达的意思是 你提的这个方法的确是存在问题的 不过目前代码中存在一个和这个有问题的方法相同功能的方法 如果你有兴趣修复 可以使用 findCategoryTreeNodeById这个方法 替换 collectAllChildByCategoryId这个有问题的方法的引用 然后删除这个collectAllChildByCategoryId方法，简言之就是修复的时候不要去修复这个方法而且使用findCategoryTreeNodeById替换它😶‍🌫️

[1379]

我在阅读这块源码的过程中产生了另一个疑惑，collectAllChild这个方法中调用了categoryAuthentication 方法，我不太清楚这里的作用是什么，执行当前操作的肯定是管理员，既然如此，为什么还要进行权限验证呢

[guqing]

我在阅读这块源码的过程中产生了另一个疑惑，collectAllChild 这个方法中调用了 categoryAuthentication 方法，我不太清楚这里的作用是什么，执行当前操作的肯定是管理员，既然如此，为什么还要进行权限验证呢

这个service的方法会同时被前台和管理后台使用，在后台不需要判断加密分类的鉴权但前台使用时需要，这是之前设计的问题属于历史遗留 也是因为我们review pr时的疏忽

[1379]

hi @1379,感谢你对此提出issue，该方法的确存在问题，如果你有兴趣修复的话请继续阅读： 在这之前我修复一个问题的过程中也发现了该代码的问题，因此我在代码中写了一个

halo/src/main/java/run/halo/app/service/impl/CategoryServiceImpl.java

Line 333 in 631b214

private Optional<CategoryVO> findCategoryTreeNodeById(List<CategoryVO> categoryVos,

方法来替代这件事，但由于该块代码可读性不是很好且没有良好的测试所以之前并没有替换它，你可以尝试使用 findCategoryTreeNodeById 方法替换它，可能存在需要过滤加密分类的问题，我的建议是分为两个步骤做：

1. 先根据分类id获取子分类
2. 判断是否需要过滤加密分类（加密分类的过滤其实不应该在service或不应该是现在这样处理做但由于历史原因，遵循该逻辑即可，我们会在后续版本重构它）

对于 步骤 2 如果你需要可以将

halo/src/main/java/run/halo/app/service/impl/CategoryServiceImpl.java

Line 307 in 631b214

private List<Category> walkCategoryTree(CategoryVO root) {

改为模版方法即下面的形式

private List<Category> walkCategoryTree(CategoryVO root, Consumer<CategoryVO> consumer) {
        Assert.notNull(root, "The category 'root' must not be null");
        List<Category> categories = new LinkedList<>();
        Queue<CategoryVO> queue = new ArrayDeque<>();
        queue.add(root);
        while (!queue.isEmpty()) {
            CategoryVO categoryNode = queue.poll();

            consumer.accept(category);

            if (HaloUtils.isNotEmpty(categoryNode.getChildren())) {
                queue.addAll(categoryNode.getChildren());
            }
        }
        return categories;
    }

当然这一切重构的前提是建议你先写测试类，使其在原先代码上能顺利通过，然后在小步重构代码伴随反复运行测试类，与此往复。还请小心为之。

“加密分类的过滤不应放在Service” ，那这里比较好的实践是要放到Controller去做吗

[guqing]

hi @1379,感谢你对此提出issue，该方法的确存在问题，如果你有兴趣修复的话请继续阅读： 在这之前我修复一个问题的过程中也发现了该代码的问题，因此我在代码中写了一个

halo/src/main/java/run/halo/app/service/impl/CategoryServiceImpl.java

Line 333 in 631b214

private Optional<CategoryVO> findCategoryTreeNodeById(List<CategoryVO> categoryVos,

方法来替代这件事，但由于该块代码可读性不是很好且没有良好的测试所以之前并没有替换它，你可以尝试使用 findCategoryTreeNodeById 方法替换它，可能存在需要过滤加密分类的问题，我的建议是分为两个步骤做：

1. 先根据分类id获取子分类
2. 判断是否需要过滤加密分类（加密分类的过滤其实不应该在service或不应该是现在这样处理做但由于历史原因，遵循该逻辑即可，我们会在后续版本重构它）

对于 步骤 2 如果你需要可以将

halo/src/main/java/run/halo/app/service/impl/CategoryServiceImpl.java

Line 307 in 631b214

private List<Category> walkCategoryTree(CategoryVO root) {

改为模版方法即下面的形式

private List<Category> walkCategoryTree(CategoryVO root, Consumer<CategoryVO> consumer) {
        Assert.notNull(root, "The category 'root' must not be null");
        List<Category> categories = new LinkedList<>();
        Queue<CategoryVO> queue = new ArrayDeque<>();
        queue.add(root);
        while (!queue.isEmpty()) {
            CategoryVO categoryNode = queue.poll();

            consumer.accept(category);

            if (HaloUtils.isNotEmpty(categoryNode.getChildren())) {
                queue.addAll(categoryNode.getChildren());
            }
        }
        return categories;
    }

当然这一切重构的前提是建议你先写测试类，使其在原先代码上能顺利通过，然后在小步重构代码伴随反复运行测试类，与此往复。还请小心为之。

“加密分类的过滤不应放在Service” ，那这里比较好的实践是要放到Controller去做吗

应该放在具体的场景 比如前台需要过滤就放到前台调用的地方加一个防腐层 或者使用不同的方法 或者加参数来决定是否需要过滤等方案 不过这个你先不用考虑 这个issue有关的pr只要替换 collectAllChildByCategoryId 的使用然后删除这个有问题的方法即可

[1379]

加密这块太复杂了，我觉得我提pr可能考虑不太全面，还是大佬来吧。
另外，我总觉得Halo在加密文章和分类这块设计的太不合理了。

halo/src/main/java/run/halo/app/service/impl/AuthorizationServiceImpl.java

Line 60 in 631b214

private void doDeleteAuthorization(String value) {

比如当更新一个分类的时候，可能需要删除所有人在这个分类上之前的授权，doDeleteAuthorization的方法实现竟然是取出所有缓存，然后遍历，然后逐个删除这个分类在这些缓存里的存在，这样效率太低了。

总之，我感觉现在halo的加密文章和分类的设计挺糟糕的，而且关于这块的漏洞也很多(上次我就提过一个issue)，加密这个功能直接让业务逻辑复杂了很多。

(不是单纯的提意见、白嫖党，我自己也是halo的忠实用户，所以一直给halo挖掘bug，希望能一起做的更好)

[guqing]

加密这块太复杂了，我觉得我提pr可能考虑不太全面，还是大佬来吧。
另外，我总觉得Halo在加密文章和分类这块设计的太不合理了。

halo/src/main/java/run/halo/app/service/impl/AuthorizationServiceImpl.java

Line 60 in 631b214

private void doDeleteAuthorization(String value) {

比如当更新一个分类的时候，可能需要删除所有人在这个分类上之前的授权，doDeleteAuthorization的方法实现竟然是取出所有缓存，然后遍历，然后逐个删除这个分类在这些缓存里的存在，这样效率太低了。

总之，我感觉现在halo的加密文章和分类的设计挺糟糕的，而且关于这块的漏洞也很多(上次我就提过一个issue)，加密这个功能直接让业务逻辑复杂了很多。

(不是单纯的提意见、白嫖党，我自己也是halo的忠实用户，所以一直给halo挖掘bug，希望能一起做的更好)

是的 这块设计确实有问题

[ruibaby]

另外，我总觉得Halo在加密文章和分类这块设计的太不合理了。

经过讨论，我们将尝试在 1.5 重构这个部分，可以关注后续 1.5 alpha 的版本发布。

[1379]

另外，我总觉得Halo在加密文章和分类这块设计的太不合理了。

经过讨论，我们将尝试在 1.5 重构这个部分，可以关注后续 1.5 alpha 的版本发布。

那1.5 预计重构的目标是什么呢，这个有吗

[ruibaby]

那1.5 预计重构的目标是什么呢，这个有吗

已经添加到 1.5 的 milestone。这次重构主要为了简化部分逻辑，尽量保证兼容。

[yuanzhixiang]

那1.5 预计重构的目标是什么呢，这个有吗

已经添加到 1.5 的 milestone。这次重构主要为了简化部分逻辑，尽量保证兼容。

这块之前是我写的，有问题可以我可以改这块

[yuanzhixiang]

二月下旬和三月上旬没时间参与开源，这个问题不急的话可以留到三月下旬我来改

[ruibaby]

这块之前是我写的，有问题可以我可以改这块

@guqing 似乎已经在进行了。

[yuanzhixiang]

这块之前是我写的，有问题可以我可以改这块

@guqing 似乎已经在进行了。

好的，需要帮忙可以 @yuanzhixiang 我，之前 github 邮箱配的有问题收不到邮件，现在 @ 我可以随时联系到我

[guqing]

二月下旬和三月上旬没时间参与开源，这个问题不急的话可以留到三月下旬我来改

我在进行中了 如果有时间可以看看其他的 issue 😃

[guqing]

这块之前是我写的，有问题可以我可以改这块

@guqing 似乎已经在进行了。

好的，需要帮忙可以 @yuanzhixiang 我，之前 github 邮箱配的有问题收不到邮件，现在 @ 我可以随时联系到我

好的

[ruibaby]

好的，需要帮忙可以 @yuanzhixiang 我，之前 github 邮箱配的有问题收不到邮件，现在 @ 我可以随时联系到我

非常感谢你的参与。

[yuanzhixiang]

@ruibaby @guqing 看到 halo 发了 1.5 的 alpha 真的很开心，很感谢老哥们的贡献

[guqing]

@ruibaby @guqing 看到 halo 发了 1.5 的 alpha 真的很开心，很感谢老哥们的贡献

欢迎对 1.5.0.alpha-1进行测试 有问题随时提issue 🤪

[1379]

halo/src/main/java/run/halo/app/service/impl/CategoryServiceImpl.java

Line 533 in c306586

if (StringUtils.isNotBlank(category.getPassword())) {

这里也有问题，如果把当前要更新的category放到一个加密分类下面，那么即使他的密码是空，也需要进行encrypt操作

[guqing]

halo/src/main/java/run/halo/app/service/impl/CategoryServiceImpl.java

Line 533 in c306586

if (StringUtils.isNotBlank(category.getPassword())) {

这里也有问题，如果把当前要更新的 category 放到一个加密分类下面，那么即使他的密码是空，也需要进行 encrypt 操作

已经在该PR重构了 See #1678

[1379]

我看到把Category更新的动作用事件来处理了，不过这样似乎几个操作不是在一个事务里了？这样会有问题吗？
@guqing

[guqing]

我看到把Category更新的动作用事件来处理了，不过这样似乎几个操作不是在一个事务里了？这样会有问题吗？
@guqing

这不是更新数据库 而且清除该分类对应的认证记录缓存 所以不需要事物

[1379]

不只有清除缓存啊，还有更新post呢
@guqing

[1379]

是不是可以用TransactionalEventListener 来处理

[guqing]

是不是可以用 TransactionalEventListener 来处理

@1379 非常感谢你对我们的代码提出疑问，针对你的提问以下提供说明：

首先我们这里的逻辑是在文章或分类更新后如果是从加密状态->非加密则去刷新文章为草稿，但是在分类更新时刷新文章状态的动作，会导致文章分类(Category)依赖文章，这样的耦合非常不好，因此这里使用事件来做是为了将分类更新时需要刷新文章状态 的动作解耦出去，并不是需要前者事物提交后获取其结果，因此这里使用@EventListener且没有加@Async，简言之就是说这里发送的都是同步事件，针对同步事件spring的事物处理等同于private方法（@EventListener 的方法体会内联到 分类/文章更新 方法中）也就是都在同一个事物上下文，所以即使当更新分类的方法在事件发布后执行失败，那么事件中的操作数据库逻辑也会被事物回滚，详情你可以参考spring 事物原理或者Spring events and transactions，综上所述，并不需要使用你所说的 @TransactionalEventListener来做，这里的实现没有问题。