Skip to content
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

[ISSUE#11910] resolve mapper class paging parameter problem #11915

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

publicize-y
Copy link
Contributor

link #11910

@publicize-y
Copy link
Contributor Author

Some paginated queries seem to require the helper. fetchPageLimit method instead of the original helper. fetchPage method ,Is it possible to modify it this way? @KomachiSion

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.63%. Comparing base (c09ee2d) to head (2d07948).
Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             develop   #11915      +/-   ##
=============================================
- Coverage      68.63%   68.63%   -0.01%     
- Complexity      9034     9036       +2     
=============================================
  Files           1239     1239              
  Lines          40610    40606       -4     
  Branches        4317     4317              
=============================================
- Hits           27872    27869       -3     
+ Misses         10746    10745       -1     
  Partials        1992     1992              
Files Coverage Δ
...d/EmbeddedHistoryConfigInfoPersistServiceImpl.java 95.71% <100.00%> (ø)
...l/ExternalHistoryConfigInfoPersistServiceImpl.java 98.94% <100.00%> (+0.01%) ⬆️
...source/impl/derby/ConfigInfoAggrMapperByDerby.java 100.00% <100.00%> (ø)
...source/impl/derby/ConfigInfoBetaMapperByDerby.java 100.00% <ø> (ø)
...datasource/impl/derby/ConfigInfoMapperByDerby.java 68.18% <100.00%> (-1.17%) ⬇️
...asource/impl/derby/ConfigInfoTagMapperByDerby.java 100.00% <100.00%> (ø)
...mpl/derby/ConfigInfoTagsRelationMapperByDerby.java 65.71% <100.00%> (+2.07%) ⬆️
...rce/impl/derby/HistoryConfigInfoMapperByDerby.java 100.00% <100.00%> (+30.76%) ⬆️
...source/impl/mysql/ConfigInfoAggrMapperByMySql.java 100.00% <100.00%> (ø)
...source/impl/mysql/ConfigInfoBetaMapperByMySql.java 100.00% <ø> (ø)
... and 4 more

... and 3 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 c09ee2d...2d07948. Read the comment docs.

Copy link
Collaborator

@KomachiSion KomachiSion left a comment

Choose a reason for hiding this comment

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

I think you should do more understand for nacos database plugin.

paramList.add(tenantId);
final String sqlCount = "SELECT count(*) FROM config_info_aggr WHERE data_id=? AND group_id=? AND tenant_id=?";
return new MapperResult(sqlCount, paramList);
}
Copy link
Contributor Author

@publicize-y publicize-y Apr 25, 2024

Choose a reason for hiding this comment

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

这么改似乎目前不会影响用户自定义的插件逻辑,相当于还是一种固定参数的形式。针对分页提供了count的默认实现方法,如果用户想实现自己的count逻辑也可以重写默认实现方法。相当于用户自己实现的数据源插件,count查询提供了入口进行修改。看这么改是否合适;可以review一下我最新一次的提交 @KomachiSion

@KomachiSion
Copy link
Collaborator

I see the new changes, But I think no much different with the old one, or the change files is so many that I can't get the key change points.

@publicize-y
Copy link
Contributor Author

I see the new changes, But I think no much different with the old one, or the change files is so many that I can't get the key change points.

image
目前我只更改了一个地方,对sqlcount提供了默认实现,这种方式可行的话我将把其他地方一并更改掉

@publicize-y
Copy link
Contributor Author

I see the new changes, But I think no much different with the old one, or the change files is so many that I can't get the key change points.

image 目前我只更改了一个地方,对sqlcount提供了默认实现,这种方式可行的话我将把其他地方一并更改掉

@KomachiSion

@publicize-y
Copy link
Contributor Author

I see the new changes, But I think no much different with the old one, or the change files is so many that I can't get the key change points.

image 目前我只更改了一个地方,对sqlcount提供了默认实现,这种方式可行的话我将把其他地方一并更改掉

@KomachiSion

@publicize-y publicize-y reopened this Jun 12, 2024
@publicize-y
Copy link
Contributor Author

I see the new changes, But I think no much different with the old one, or the change files is so many that I can't get the key change points.

image 目前我只更改了一个地方,对sqlcount提供了默认实现,这种方式可行的话我将把其他地方一并更改掉

@KomachiSion

Can you have time to reply to me @KomachiSion

@KomachiSion
Copy link
Collaborator

I see the new changes, But I think no much different with the old one, or the change files is so many that I can't get the key change points.

image 目前我只更改了一个地方,对sqlcount提供了默认实现,这种方式可行的话我将把其他地方一并更改掉

@KomachiSion

Can you have time to reply to me @KomachiSion

抱歉, 这个issue 暂时先block吧, 感觉改动有一点大。

@publicize-y
Copy link
Contributor Author

I see the new changes, But I think no much different with the old one, or the change files is so many that I can't get the key change points.

image 目前我只更改了一个地方,对sqlcount提供了默认实现,这种方式可行的话我将把其他地方一并更改掉

@KomachiSion

Can you have time to reply to me @KomachiSion

抱歉, 这个issue 暂时先block吧, 感觉改动有一点大。

我想问一下这个issue计划在哪个版本优化掉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants