fix auth plugin Page #11300

huangkemingyyds · 2023-10-29T08:38:36Z

Please do not create a Pull Request without creating an issue first.

What is the purpose of the change

#11299
1.修复auth模块用户，角色，权限查询分页失效的问题。

Brief changelog

auth模块调整为根据数据源选择适配器，进行分页辅助查询。
默认提供辅助查询三个适配器，msyql、derby、default（不做任何处理)。
使用AuthPaginationHelper 替换 PaginationHelper，尽可能得保持解耦persistence service。

Verifying this change

XXXX

Follow this checklist to help us incorporate your contribution quickly and easily:

Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
Run mvn -B clean package apache-rat:check findbugs:findbugs -Dmaven.test.skip=true to make sure basic checks pass. Run mvn clean install -DskipITs to make sure unit-test pass. Run mvn clean test-compile failsafe:integration-test to make sure integration-test pass.

KomachiSion

CI not passed

huangkemingyyds · 2023-10-30T04:44:13Z

CI not passed

本次修改没有修改到core模块哦。 Ci异常是某个测试类没有通过。

codecov-commenter · 2023-10-30T07:45:09Z

Codecov Report

Merging #11300 (6f8167d) into develop (f42b296) will decrease coverage by 0.80%.
Report is 16 commits behind head on develop.
The diff coverage is 19.25%.

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #11300      +/-   ##
=============================================
- Coverage      58.42%   57.62%   -0.80%     
- Complexity      5752     6429     +677     
=============================================
  Files            885     1059     +174     
  Lines          27356    31354    +3998     
  Branches        2890     3243     +353     
=============================================
+ Hits           15982    18067    +2085     
- Misses         10156    11974    +1818     
- Partials        1218     1313      +95

Files	Coverage Δ
...cos/persistence/constants/PersistenceConstant.java	`0.00% <ø> (ø)`
...ntrol/connection/response/ConnectionCheckCode.java	`0.00% <ø> (ø)`
...ository/embedded/EmbeddedPaginationHelperImpl.java	`0.00% <0.00%> (ø)`
...y/extrnal/ExternalStoragePaginationHelperImpl.java	`0.00% <0.00%> (ø)`
...os/plugin/auth/impl/constant/AuthPageConstant.java	`0.00% <0.00%> (ø)`
...pl/persistence/EmbeddedRolePersistServiceImpl.java	`56.25% <83.33%> (ø)`
...pl/persistence/EmbeddedUserPersistServiceImpl.java	`57.40% <66.66%> (ø)`
...pl/persistence/ExternalRolePersistServiceImpl.java	`44.82% <90.00%> (ø)`
...pl/persistence/ExternalUserPersistServiceImpl.java	`41.55% <87.50%> (ø)`
...sistence/EmbeddedPermissionPersistServiceImpl.java	`38.29% <60.00%> (ø)`
... and 8 more

... and 259 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f42b296...6f8167d. Read the comment docs.

KomachiSion · 2023-10-30T08:08:08Z

其他的CI可以通过，只有你修改后的PR不通过的话，应该是修改的地方间接影响到了对应的UT，需要修复代码。而不是直接改UT

huangkemingyyds · 2023-10-30T08:27:12Z

其他的CI可以通过，只有你修改后的PR不通过的话，应该是修改的地方间接影响到了对应的UT，需要修复代码。而不是直接改UT

1.我需要确认，我未提交PR之前。这两个DefaultTlsContextBuilderTest，DefaultTlsProtocolNegotiatorBuilderTest是否能正常运行？
2.我未提交PR之前，我本地这两个DefaultTlsContextBuilderTest，DefaultTlsProtocolNegotiatorBuilderTest不能正常确认。所以我提了两个PR.
3. @KomachiSion 我需要确定第一点。

KomachiSion · 2023-11-01T03:24:09Z

DefaultTlsContextBuilderTest.java

这个文件在另外一个PR合并了，这个PR应该rebase一下develop分支。

This reverts commit 4e2c85d

This reverts commit 4225891.

huangkemingyyds · 2023-11-01T05:38:00Z

DefaultTlsContextBuilderTest.java

这个文件在另外一个PR合并了，这个PR应该rebase一下develop分支。

done

KomachiSion · 2023-11-02T02:02:38Z

persistence/src/main/java/com/alibaba/nacos/persistence/dialects/DialectFactory.java

+ *
+ * @author hkm
+ */
+public class DialectFactory {


这个设计不太好，实际上数据源已经插件化，不应该强制感知数据库类型，如果需要类似Dialect的东西，最好在插件测实现。

可以看下 datasource-plugin模块

这个设计不太好，实际上数据源已经插件化，不应该强制感知数据库类型，如果需要类似Dialect的东西，最好在插件测实现。

我也有思考过这个问题。

现在persistence 模块是单独存在的，不依赖于 datasource-plugin模块。

但是PaginationHelper类这个分页是写在persistence模块下的。如果不在persistence 模块下做数据分页查询条件的适配的话，更换数据源，会导致那么用户、角色等分页查询失效。

我认为现在最小的改动，就是在persistence 下将nacos所有支持的数据源类型，对应分页查询条件补齐Dialect类型。

或者说 PaginationHelper 目前只支持实现类：

ExternalStoragePaginationHelperImpl-mysql 、

EmbeddedPaginationHelperImpl-derby。

后续继续增加实现类。xxxxHelperImpl-xxx。

已这种方式来弥补分页失效的问题。

如果选择 DialectFactory 的方式。我可以补充好其他数据源的数据类型，继续PR。

或者说有更好的方式，我将关闭PR。

KomachiSion · 2023-11-06T07:36:09Z

数据库类型的感知应该有datasource插件来做，如果你需要加这个Dialect来进行分页的辅助，需要在不同的插件实现中进行实现，而不是把所有数据库的实现都放在nacos中来做。

huangkemingyyds · 2023-11-06T07:54:13Z

像 auth这个模块，不依赖于 datasource插件，要使用分页查询。又该如何实现？比如下面这个接口：
http://localhost:8848/nacos/v1/auth/users?pageNo=1&pageSize=2&search=accurate

KomachiSion · 2023-11-09T07:49:07Z

在auth模块自己实现，auth模块甚至可以使用jpa直接替换persistence service

KomachiSion · 2023-11-09T07:49:31Z

在auth模块自己实现，auth模块甚至可以使用jpa直接替换persistence service

插件之间不应该互相依赖和影响。

huangkemingyyds · 2023-11-09T15:35:13Z

在auth模块自己实现，auth模块甚至可以使用jpa直接替换persistence service

插件之间不应该互相依赖和影响。

我个人认为PaginationHelper 在项目中定位为一种基础组件，Dialect支持感知数据源，辅助分页查询。并且随着数据源增多，只需迭代Dialect。好处是在拓展其他数据源的时候，不用在查询sql中手动的拼写查询条件，交给Dialect来处理。
像分页辅助Dialect这种具有共性的组件，却没有统一管理的地方。其他模块有使用到的地方，得重新编写，应该是重复开发了。

huangkemingyyds · 2023-11-14T06:14:20Z

在auth模块自己实现，auth模块甚至可以使用jpa直接替换persistence service

插件之间不应该互相依赖和影响。

我个人认为PaginationHelper 在项目中定位为一种基础组件，Dialect支持感知数据源，辅助分页查询。并且随着数据源增多，只需迭代Dialect。好处是在拓展其他数据源的时候，不用在查询sql中手动的拼写查询条件，交给Dialect来处理。

像分页辅助Dialect这种具有共性的组件，却没有统一管理的地方。其他模块有使用到的地方，得重新编写，应该是重复开发了。
补充：3. 如果auth自己实现分页查询，也是要根据数据源拼接参数的。

@KomachiSion

KomachiSion · 2023-11-15T08:16:23Z

PaginationHelper 可以作为基础接口，但是不管实现。

Dialect既然是适配的实现，就应该在实现的插件里面去实现，插件管理器不应该有。if是a插件，if是b插件的逻辑，就应该是getDialect，然后Dialect.xxx().

huangkemingyyds · 2023-11-15T08:25:33Z

PaginationHelper 可以作为基础接口，但是不管实现。

Dialect既然是适配的实现，就应该在实现的插件里面去实现，插件管理器不应该有。if是a插件，if是b插件的逻辑，就应该是getDialect，然后Dialect.xxx().

那既然是这样的话，那auth模块，角色、用户、权限的查询分页，就不能依赖persistence service中的PaginationHelper。
bug不存在于 PaginationHelper分页。而是auth模块没有分页查询的适配了。
是否要在auth写数据源分页适配查询的逻辑？

KomachiSion · 2023-11-17T06:51:59Z

PaginationHelper 可以作为基础接口，但是不管实现。
Dialect既然是适配的实现，就应该在实现的插件里面去实现，插件管理器不应该有。if是a插件，if是b插件的逻辑，就应该是getDialect，然后Dialect.xxx().

那既然是这样的话，那auth模块，角色、用户、权限的查询分页，就不能依赖persistence service中的PaginationHelper。

bug不存在于 PaginationHelper分页。而是auth模块没有分页查询的适配了。

是否要在auth写数据源分页适配查询的逻辑？

正确，auth模块是独立模块，其实不应该依赖任何底层模块，当前的默认实现在逐渐排除对nacos server模块的依赖。
那就只需要在auth模块中修复即可。
auth插件是auth插件， auth插件只支持MySQL数据库，不涉及多数据源适配，需要适配的话就自行开发对应数据源的auth插件; 或者将默认鉴权插件和persistence也解耦，直接使用spring jpa来处理数据库操作。

KomachiSion

看完你的实现，感觉和插件base中的这部分实现有一定重复了。

你看下是不是。

https://github.com/nacos-group/nacos-plugin/blob/develop/nacos-datasource-plugin-ext/nacos-datasource-plugin-ext-base/src/main/java/com/alibaba/nacos/plugin/datasource/dialect/AbstractDatabaseDialect.java

huangkemingyyds · 2023-11-20T07:17:08Z

看完你的实现，感觉和插件base中的这部分实现有一定重复了。

你看下是不是。

https://github.com/nacos-group/nacos-plugin/blob/develop/nacos-datasource-plugin-ext/nacos-datasource-plugin-ext-base/src/main/java/com/alibaba/nacos/plugin/datasource/dialect/AbstractDatabaseDialect.java
是有重复了我这边去掉吧

* Optimize paginated queries based on data sources * add startup conditions * user movkEnv * update Exception Class name * Optimize paginated queries based on data sources * update Exception Class name * Revert "add startup conditions" This reverts commit 4e2c85d * Optimize paginated queries based on data sources * add startup conditions * user movkEnv * update Exception Class name * Revert "add startup conditions" This reverts commit 4e2c85d * Revert "Revert "add startup conditions"" This reverts commit 4225891. * DialectFactory add database supported * fix auth plugin Page, add dataSource plugin Dialect * description * remove dataSource plugin Dialect * detail adjustment * DefaultPageHandlerAdapter

Optimize paginated queries based on data sources

ac964eb

KomachiSion approved these changes Oct 30, 2023

View reviewed changes

KomachiSion requested changes Oct 30, 2023

View reviewed changes

huangkemingyyds added 2 commits October 30, 2023 15:23

add startup conditions

4e2c85d

Merge branch 'issues_11304' into issue_page

04fa5fe

huangkemingyyds added 3 commits October 30, 2023 19:26

user movkEnv

1fb4b4c

Merge branch 'issues_11304' into issue_page

db88258

update Exception Class name

82d4bad

huangkemingyyds added 8 commits November 1, 2023 12:59

Optimize paginated queries based on data sources

5e0eba0

update Exception Class name

f380abb

Revert "add startup conditions"

63056e5

This reverts commit 4e2c85d

Optimize paginated queries based on data sources

dad513d

add startup conditions

3e4bf20

user movkEnv

73c34d2

update Exception Class name

fb70370

Revert "add startup conditions"

4225891

This reverts commit 4e2c85d

huangkemingyyds force-pushed the issue_page branch from 6debecf to 4225891 Compare November 1, 2023 05:01

Revert "Revert "add startup conditions""

8f3dc72

This reverts commit 4225891.

KomachiSion requested changes Nov 2, 2023

View reviewed changes

huangkemingyyds added 2 commits November 2, 2023 23:43

Merge remote-tracking branch 'origin/issue_page' into issue_page

e47b7a8

DialectFactory add database supported

526d5ad

KomachiSion added the pending On hold due to dependency or release label Nov 6, 2023

huangkemingyyds requested a review from KomachiSion November 15, 2023 07:16

huangkemingyyds added 2 commits November 18, 2023 07:42

fix auth plugin Page, add dataSource plugin Dialect

163eed6

description

777bbaf

huangkemingyyds changed the title ~~Optimize paginated queries based on data sources~~ fix auth plugin Page, add dataSource plugin Dialect Nov 18, 2023

KomachiSion requested changes Nov 20, 2023

View reviewed changes

remove dataSource plugin Dialect

457379b

huangkemingyyds changed the title ~~fix auth plugin Page, add dataSource plugin Dialect~~ fix auth plugin Page Nov 20, 2023

huangkemingyyds added 2 commits November 21, 2023 23:00

detail adjustment

4ae7db2

DefaultPageHandlerAdapter

6f8167d

KomachiSion approved these changes Nov 24, 2023

View reviewed changes

KomachiSion added plugin kind/bug Category issues or prs related to bug. and removed pending On hold due to dependency or release labels Nov 24, 2023

KomachiSion added this to the 2.3.0 milestone Nov 24, 2023

KomachiSion merged commit 736948f into alibaba:develop Nov 24, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix auth plugin Page #11300

fix auth plugin Page #11300

huangkemingyyds commented Oct 29, 2023 •

edited

Loading

KomachiSion left a comment

huangkemingyyds commented Oct 30, 2023

codecov-commenter commented Oct 30, 2023 •

edited

Loading

KomachiSion commented Oct 30, 2023

huangkemingyyds commented Oct 30, 2023 •

edited

Loading

KomachiSion commented Nov 1, 2023

huangkemingyyds commented Nov 1, 2023

KomachiSion Nov 2, 2023

KomachiSion Nov 2, 2023

huangkemingyyds Nov 2, 2023

huangkemingyyds Nov 2, 2023

huangkemingyyds Nov 2, 2023

KomachiSion commented Nov 6, 2023

huangkemingyyds commented Nov 6, 2023

KomachiSion commented Nov 9, 2023

KomachiSion commented Nov 9, 2023

huangkemingyyds commented Nov 9, 2023

huangkemingyyds commented Nov 14, 2023

KomachiSion commented Nov 15, 2023

huangkemingyyds commented Nov 15, 2023

KomachiSion commented Nov 17, 2023

KomachiSion left a comment

huangkemingyyds commented Nov 20, 2023

fix auth plugin Page #11300

fix auth plugin Page #11300

Conversation

huangkemingyyds commented Oct 29, 2023 • edited Loading

What is the purpose of the change

Brief changelog

Verifying this change

KomachiSion left a comment

Choose a reason for hiding this comment

huangkemingyyds commented Oct 30, 2023

codecov-commenter commented Oct 30, 2023 • edited Loading

Codecov Report

KomachiSion commented Oct 30, 2023

huangkemingyyds commented Oct 30, 2023 • edited Loading

KomachiSion commented Nov 1, 2023

huangkemingyyds commented Nov 1, 2023

KomachiSion Nov 2, 2023

Choose a reason for hiding this comment

KomachiSion Nov 2, 2023

Choose a reason for hiding this comment

huangkemingyyds Nov 2, 2023

Choose a reason for hiding this comment

huangkemingyyds Nov 2, 2023

Choose a reason for hiding this comment

huangkemingyyds Nov 2, 2023

Choose a reason for hiding this comment

KomachiSion commented Nov 6, 2023

huangkemingyyds commented Nov 6, 2023

KomachiSion commented Nov 9, 2023

KomachiSion commented Nov 9, 2023

huangkemingyyds commented Nov 9, 2023

huangkemingyyds commented Nov 14, 2023

KomachiSion commented Nov 15, 2023

huangkemingyyds commented Nov 15, 2023

KomachiSion commented Nov 17, 2023

KomachiSion left a comment

Choose a reason for hiding this comment

huangkemingyyds commented Nov 20, 2023

huangkemingyyds commented Oct 29, 2023 •

edited

Loading

codecov-commenter commented Oct 30, 2023 •

edited

Loading

huangkemingyyds commented Oct 30, 2023 •

edited

Loading