You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@dolphinscheduler.apache.org by "shouwangyw (via GitHub)" <gi...@apache.org> on 2024/10/10 03:09:44 UTC
[PR] [Fix-16680][ApiServer]Query project list paging error [dolphinscheduler]
shouwangyw opened a new pull request, #16681:
URL: https://github.com/apache/dolphinscheduler/pull/16681
## Purpose of the pull request
This pull request fix the project query error
Fix #16680
## Brief change log
Change WorkflowDefinitionMapper.xml#queryProjectWorkflowDefinitionCountByProjectCodes
## Verify this pull request
This pull request is code cleanup without any test coverage.
*(or)*
This pull request is already covered by existing tests, such as *(please describe tests)*.
(or)
This change added tests and can be verified as follows:
(or)
## Pull Request Notice
[Pull Request Notice](https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/contribute/join/pull-request.md)
If your pull request contain incompatible change, you should also add it to `docs/docs/en/guide/upgrede/incompatible.md`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16680][ApiServer] Query project list paging error [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #16681:
URL: https://github.com/apache/dolphinscheduler/pull/16681#issuecomment-2406624988
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=16681) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=16681&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=16681&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=16681&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=16681&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=16681&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=16681)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16680][ApiServer] Query project list paging error [dolphinscheduler]
Posted by "SbloodyS (via GitHub)" <gi...@apache.org>.
SbloodyS commented on code in PR #16681:
URL: https://github.com/apache/dolphinscheduler/pull/16681#discussion_r1796374186
##########
dolphinscheduler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/WorkflowDefinitionMapper.xml:
##########
@@ -200,10 +200,14 @@
project_code as projectCode
,count(*) as count
from t_ds_workflow_definition
- where project_code in
- <foreach collection="projectCodes" index="index" item="i" open="(" separator="," close=")">
- #{i}
- </foreach>
+ <where>
Review Comment:
We don't need this since if the the projects don't exist, the number of workflow definitions under the projects is meaningless.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16680][ApiServer] Query project list paging error [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #16681:
URL: https://github.com/apache/dolphinscheduler/pull/16681#discussion_r1796322814
##########
dolphinscheduler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/WorkflowDefinitionMapper.xml:
##########
@@ -200,10 +200,14 @@
project_code as projectCode
,count(*) as count
from t_ds_workflow_definition
- where project_code in
- <foreach collection="projectCodes" index="index" item="i" open="(" separator="," close=")">
- #{i}
- </foreach>
+ <where>
Review Comment:
Yes, we should filter this in upper layer. The project_code should avoid to be null.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16680][ApiServer]Query project list paging error [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #16681:
URL: https://github.com/apache/dolphinscheduler/pull/16681#issuecomment-2403883007
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=16681) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=16681&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=16681&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=16681&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=16681&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=16681&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=16681)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16680][ApiServer] Query project list paging error [dolphinscheduler]
Posted by "shouwangyw (via GitHub)" <gi...@apache.org>.
shouwangyw commented on code in PR #16681:
URL: https://github.com/apache/dolphinscheduler/pull/16681#discussion_r1796336239
##########
dolphinscheduler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/WorkflowDefinitionMapper.xml:
##########
@@ -200,10 +200,14 @@
project_code as projectCode
,count(*) as count
from t_ds_workflow_definition
- where project_code in
- <foreach collection="projectCodes" index="index" item="i" open="(" separator="," close=")">
- #{i}
- </foreach>
+ <where>
Review Comment:
> Yes, we should filter this in upper layer. The project_code should avoid to be null.
Yes, but this method must also ensure robustness.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16680][ApiServer] Query project list paging error [dolphinscheduler]
Posted by "shouwangyw (via GitHub)" <gi...@apache.org>.
shouwangyw commented on code in PR #16681:
URL: https://github.com/apache/dolphinscheduler/pull/16681#discussion_r1796701627
##########
dolphinscheduler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/WorkflowDefinitionMapper.xml:
##########
@@ -200,10 +200,14 @@
project_code as projectCode
,count(*) as count
from t_ds_workflow_definition
- where project_code in
- <foreach collection="projectCodes" index="index" item="i" open="(" separator="," close=")">
- #{i}
- </foreach>
+ <where>
Review Comment:
> I think we don't need to change this since we've already filtered it at the entrance.
However, if someone uses this method in the future, there will be error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16680][ApiServer] Query project list paging error [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #16681:
URL: https://github.com/apache/dolphinscheduler/pull/16681#issuecomment-2407473796
Please retry analysis of this Pull-Request directly on SonarCloud
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16680][ApiServer] Query project list paging error [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #16681:
URL: https://github.com/apache/dolphinscheduler/pull/16681#discussion_r1796843374
##########
dolphinscheduler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/WorkflowDefinitionMapper.xml:
##########
@@ -189,21 +189,39 @@
select
code
from t_ds_workflow_definition
- where project_code in
- <foreach collection="projectCodes" index="index" item="i" open="(" separator="," close=")">
- #{i}
- </foreach>
+ <where>
Review Comment:
Please revert this change, this kind of check should in `dao`, we need to avoid using complex logic in mapper.
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ProjectServiceImpl.java:
##########
@@ -394,12 +394,15 @@ public Result queryProjectListPaging(User loginUser, Integer pageSize, Integer p
List<User> userList = userMapper.selectByIds(projectList.stream()
.map(Project::getUserId).distinct().collect(Collectors.toList()));
Map<Integer, String> userMap = userList.stream().collect(Collectors.toMap(User::getId, User::getUserName));
- List<ProjectWorkflowDefinitionCount> projectWorkflowDefinitionCountList =
- workflowDefinitionMapper.queryProjectWorkflowDefinitionCountByProjectCodes(
- projectList.stream().map(Project::getCode).distinct().collect(Collectors.toList()));
- Map<Long, Integer> projectWorkflowDefinitionCountMap = projectWorkflowDefinitionCountList.stream()
- .collect(Collectors.toMap(ProjectWorkflowDefinitionCount::getProjectCode,
- ProjectWorkflowDefinitionCount::getCount));
+ List<Long> projectCodes = projectList.stream().map(Project::getCode).distinct().collect(Collectors.toList());
+ Map<Long, Integer> projectWorkflowDefinitionCountMap = Collections.emptyMap();
+ if (CollectionUtils.isNotEmpty(projectCodes)) {
+ projectWorkflowDefinitionCountMap = workflowDefinitionMapper
+ .queryProjectWorkflowDefinitionCountByProjectCodes(projectCodes)
+ .stream()
+ .collect(Collectors.toMap(ProjectWorkflowDefinitionCount::getProjectCode,
+ ProjectWorkflowDefinitionCount::getCount));
+ }
Review Comment:
If the `projectList` is empty, then we can return early at line 389, then the logic would be clearer?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16680][ApiServer] Query project list paging error [dolphinscheduler]
Posted by "shouwangyw (via GitHub)" <gi...@apache.org>.
shouwangyw commented on code in PR #16681:
URL: https://github.com/apache/dolphinscheduler/pull/16681#discussion_r1794919800
##########
dolphinscheduler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/WorkflowDefinitionMapper.xml:
##########
@@ -200,10 +200,14 @@
project_code as projectCode
,count(*) as count
from t_ds_workflow_definition
- where project_code in
- <foreach collection="projectCodes" index="index" item="i" open="(" separator="," close=")">
- #{i}
- </foreach>
+ <where>
Review Comment:
> We should not do this query while projectCodes is null
Is it not possible to query all project when projectCodes is null？？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16680][ApiServer] Query project list paging error [dolphinscheduler]
Posted by "SbloodyS (via GitHub)" <gi...@apache.org>.
SbloodyS commented on code in PR #16681:
URL: https://github.com/apache/dolphinscheduler/pull/16681#discussion_r1796708585
##########
dolphinscheduler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/WorkflowDefinitionMapper.xml:
##########
@@ -200,10 +200,14 @@
project_code as projectCode
,count(*) as count
from t_ds_workflow_definition
- where project_code in
- <foreach collection="projectCodes" index="index" item="i" open="(" separator="," close=")">
- #{i}
- </foreach>
+ <where>
Review Comment:
> However, if someone uses this method in the future, there will be error.
We shouldn't do double check at every entrance, which is unreasonable. Avoiding similar problems should be ensured by ut and e2e tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16680][ApiServer] Query project list paging error [dolphinscheduler]
Posted by "SbloodyS (via GitHub)" <gi...@apache.org>.
SbloodyS merged PR #16681:
URL: https://github.com/apache/dolphinscheduler/pull/16681
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16680][ApiServer] Query project list paging error [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #16681:
URL: https://github.com/apache/dolphinscheduler/pull/16681#issuecomment-2406625244
Please retry analysis of this Pull-Request directly on SonarCloud
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16680][ApiServer]Query project list paging error [dolphinscheduler]
Posted by "SbloodyS (via GitHub)" <gi...@apache.org>.
SbloodyS commented on code in PR #16681:
URL: https://github.com/apache/dolphinscheduler/pull/16681#discussion_r1794694842
##########
dolphinscheduler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/WorkflowDefinitionMapper.xml:
##########
@@ -200,10 +200,14 @@
project_code as projectCode
,count(*) as count
from t_ds_workflow_definition
- where project_code in
- <foreach collection="projectCodes" index="index" item="i" open="(" separator="," close=")">
- #{i}
- </foreach>
+ <where>
Review Comment:
We should not do this query while projectCodes is null
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16680][ApiServer] Query project list paging error [dolphinscheduler]
Posted by "SbloodyS (via GitHub)" <gi...@apache.org>.
SbloodyS commented on code in PR #16681:
URL: https://github.com/apache/dolphinscheduler/pull/16681#discussion_r1796695499
##########
dolphinscheduler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/WorkflowDefinitionMapper.xml:
##########
@@ -200,10 +200,14 @@
project_code as projectCode
,count(*) as count
from t_ds_workflow_definition
- where project_code in
- <foreach collection="projectCodes" index="index" item="i" open="(" separator="," close=")">
- #{i}
- </foreach>
+ <where>
Review Comment:
I think we don't need to change this since we've already filtered it at the entrance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16680][ApiServer] Query project list paging error [dolphinscheduler]
Posted by "SbloodyS (via GitHub)" <gi...@apache.org>.
SbloodyS commented on code in PR #16681:
URL: https://github.com/apache/dolphinscheduler/pull/16681#discussion_r1796286011
##########
dolphinscheduler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/WorkflowDefinitionMapper.xml:
##########
@@ -200,10 +200,14 @@
project_code as projectCode
,count(*) as count
from t_ds_workflow_definition
- where project_code in
- <foreach collection="projectCodes" index="index" item="i" open="(" separator="," close=")">
- #{i}
- </foreach>
+ <where>
Review Comment:
This is used to query the number of workflow definitions under each project. If the projects don't exist, the number of workflow definitions under the projects is meaningless.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16680][ApiServer]Query project list paging error [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #16681:
URL: https://github.com/apache/dolphinscheduler/pull/16681#issuecomment-2403883389
Please retry analysis of this Pull-Request directly on SonarCloud
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16680][ApiServer] Query project list paging error [dolphinscheduler]
Posted by "shouwangyw (via GitHub)" <gi...@apache.org>.
shouwangyw commented on code in PR #16681:
URL: https://github.com/apache/dolphinscheduler/pull/16681#discussion_r1796384110
##########
dolphinscheduler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/WorkflowDefinitionMapper.xml:
##########
@@ -200,10 +200,14 @@
project_code as projectCode
,count(*) as count
from t_ds_workflow_definition
- where project_code in
- <foreach collection="projectCodes" index="index" item="i" open="(" separator="," close=")">
- #{i}
- </foreach>
+ <where>
Review Comment:
> We don't need this since if the the projects don't exist, the number of workflow definitions under the projects is meaningless.
OK, i have fix it use choose-when-otherwise, maybe this is better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16680][ApiServer] Query project list paging error [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #16681:
URL: https://github.com/apache/dolphinscheduler/pull/16681#issuecomment-2407473517
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=16681) **Quality Gate failed**
Failed conditions
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=16681&metric=new_coverage&view=list) (required ≥ 60%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=16681)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org