Skip to content

Commit

Permalink
Fix homepage user wrong statistics result for new user
Browse files Browse the repository at this point in the history 
For now, when we create a new user, it would see homepage
statistic info as admin user like, cause `projectCodeArray`
would not filter when we render SQL. So I add prefix conditions
to handle this situation.

Also, I do some minimal refactor here. including change function
name no quit fit

fix: apache#7182
  • Loading branch information
@zhongjiajie
zhongjiajie committed Dec 7, 2021
1 parent b3efcdf commit 98c7502
Show file tree
Hide file tree
Showing 11 changed files with 111 additions and 95 deletions.
4 changes: 3 additions & 1 deletion ...eduler-api/src/main/java/org/apache/dolphinscheduler/api/service/DataAnalysisService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ public interface DataAnalysisService {
Map<String, Object> countProcessInstanceStateByProject(User loginUser, long projectCode, String startDate, String endDate);

/**
* statistics the process definition quantities of certain person
* statistics the process definition quantities of a certain person
*
* We only need projects which users have permission to see to determine whether the definition belongs to the user or not.
*
* @param loginUser login user
* @param projectCode project code
Expand Down
56 changes: 36 additions & 20 deletions ...i/src/main/java/org/apache/dolphinscheduler/api/service/impl/DataAnalysisServiceImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@

import org.apache.commons.lang.StringUtils;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Date;
import java.util.HashMap;
Expand Down Expand Up @@ -90,30 +91,30 @@ public class DataAnalysisServiceImpl extends BaseServiceImpl implements DataAnal
/**
* statistical task instance status data
*
* @param loginUser login user
* @param loginUser login user
* @param projectCode project code
* @param startDate start date
* @param endDate end date
* @param startDate start date
* @param endDate end date
* @return task state count data
*/
@Override
public Map<String, Object> countTaskStateByProject(User loginUser, long projectCode, String startDate, String endDate) {

return countStateByProject(
loginUser,
projectCode,
startDate,
endDate,
(start, end, projectCodes) -> this.taskInstanceMapper.countTaskInstanceStateByUser(start, end, projectCodes));
loginUser,
projectCode,
startDate,
endDate,
(start, end, projectCodes) -> this.taskInstanceMapper.countTaskInstanceStateByProjectCodes(start, end, projectCodes));
}

/**
* statistical process instance status data
*
* @param loginUser login user
* @param loginUser login user
* @param projectCode project code
* @param startDate start date
* @param endDate end date
* @param startDate start date
* @param endDate end date
* @return process instance state count data
*/
@Override
Expand All @@ -123,14 +124,22 @@ public Map<String, Object> countProcessInstanceStateByProject(User loginUser, lo
projectCode,
startDate,
endDate,
(start, end, projectCodes) -> this.processInstanceMapper.countInstanceStateByUser(start, end, projectCodes));
(start, end, projectCodes) -> this.processInstanceMapper.countInstanceStateByProjectCodes(start, end, projectCodes));
// process state count needs to remove state of forced success
if (result.containsKey(Constants.STATUS) && result.get(Constants.STATUS).equals(Status.SUCCESS)) {
((TaskCountDto)result.get(Constants.DATA_LIST)).removeStateFromCountList(ExecutionStatus.FORCED_SUCCESS);
}
return result;
}

/**
* Wrapper function of counting process instance state and task state
*
* @param loginUser login user
* @param projectCode project code
* @param startDate start date
* @param endDate end date
*/
private Map<String, Object> countStateByProject(User loginUser, long projectCode, String startDate, String endDate
, TriFunction<Date, Date, Long[], List<ExecuteStatusCount>> instanceStateCounter) {
Map<String, Object> result = new HashMap<>();
Expand All @@ -154,10 +163,13 @@ private Map<String, Object> countStateByProject(User loginUser, long projectCode
}
}

List<ExecuteStatusCount> processInstanceStateCounts = new ArrayList<>();
Long[] projectCodeArray = projectCode == 0 ? getProjectCodesArrays(loginUser)
: new Long[] { projectCode };
List<ExecuteStatusCount> processInstanceStateCounts =
instanceStateCounter.apply(start, end, projectCodeArray);
: new Long[] {projectCode};

if (projectCodeArray.length != 0 || loginUser.getUserType() == UserType.ADMIN_USER) {
processInstanceStateCounts = instanceStateCounter.apply(start, end, projectCodeArray);
}

if (processInstanceStateCounts != null) {
TaskCountDto taskCountResult = new TaskCountDto(processInstanceStateCounts);
Expand All @@ -169,9 +181,11 @@ private Map<String, Object> countStateByProject(User loginUser, long projectCode


/**
* statistics the process definition quantities of certain person
* statistics the process definition quantities of a certain person
* <p>
* We only need projects which users have permission to see to determine whether the definition belongs to the user or not.
*
* @param loginUser login user
* @param loginUser login user
* @param projectCode project code
* @return definition count data
*/
Expand All @@ -187,10 +201,12 @@ public Map<String, Object> countDefinitionByUser(User loginUser, long projectCod
}
}

List<DefinitionGroupByUser> defineGroupByUsers = new ArrayList<>();
Long[] projectCodeArray = projectCode == 0 ? getProjectCodesArrays(loginUser)
: new Long[] { projectCode };
List<DefinitionGroupByUser> defineGroupByUsers = processDefinitionMapper.countDefinitionGroupByUser(
loginUser.getId(), projectCodeArray, isAdmin(loginUser));
: new Long[] {projectCode};
if (projectCodeArray.length != 0 || loginUser.getUserType() == UserType.ADMIN_USER) {
defineGroupByUsers = processDefinitionMapper.countDefinitionByProjectCodes(projectCodeArray);
}

DefineUserDto dto = new DefineUserDto(defineGroupByUsers);
result.put(Constants.DATA_LIST, dto);
Expand Down
12 changes: 6 additions & 6 deletions ...er-api/src/test/java/org/apache/dolphinscheduler/api/service/DataAnalysisServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ public void testCountTaskStateByProject_success() {
Mockito.when(projectMapper.queryByCode(1L)).thenReturn(getProject("test"));

//SUCCESS
Mockito.when(taskInstanceMapper.countTaskInstanceStateByUser(DateUtils.getScheduleDate(startDate),
Mockito.when(taskInstanceMapper.countTaskInstanceStateByProjectCodes(DateUtils.getScheduleDate(startDate),
DateUtils.getScheduleDate(endDate), new Long[]{1L})).thenReturn(getTaskInstanceStateCounts());
Mockito.when(projectMapper.selectById(Mockito.any())).thenReturn(getProject("test"));
Mockito.when(projectService.hasProjectAndPerm(Mockito.any(), Mockito.any(), (Map<String, Object>)Mockito.any())).thenReturn(true);
Expand Down Expand Up @@ -187,7 +187,7 @@ public void testCountTaskStateByProject_allCountZero() {

// when general user doesn't have any task then return all count are 0
user.setUserType(UserType.GENERAL_USER);
Mockito.when(taskInstanceMapper.countTaskInstanceStateByUser(any(), any(), any())).thenReturn(
Mockito.when(taskInstanceMapper.countTaskInstanceStateByProjectCodes(any(), any(), any())).thenReturn(
Collections.emptyList());
result = dataAnalysisServiceImpl.countTaskStateByProject(user, 1, null, null);
assertThat(result.get(Constants.DATA_LIST)).extracting("totalCount").isEqualTo(0);
Expand All @@ -206,7 +206,7 @@ public void testCountTaskStateByProject_noData() {

// when instanceStateCounter return null, then return nothing
user.setUserType(UserType.GENERAL_USER);
PowerMockito.when(taskInstanceMapper.countTaskInstanceStateByUser(any(), any(), any())).thenReturn(null);
PowerMockito.when(taskInstanceMapper.countTaskInstanceStateByProjectCodes(any(), any(), any())).thenReturn(null);
result = dataAnalysisServiceImpl.countTaskStateByProject(user, 1, null, null);
Assert.assertNull(result.get(Constants.DATA_LIST));
}
Expand All @@ -230,7 +230,7 @@ public void testCountProcessInstanceStateByProject() {
Mockito.when(projectService.checkProjectAndAuth(any(), any(), anyLong())).thenReturn(result);

//SUCCESS
Mockito.when(processInstanceMapper.countInstanceStateByUser(DateUtils.getScheduleDate(startDate),
Mockito.when(processInstanceMapper.countInstanceStateByProjectCodes(DateUtils.getScheduleDate(startDate),
DateUtils.getScheduleDate(endDate), new Long[]{1L})).thenReturn(getTaskInstanceStateCounts());
Mockito.when(projectService.hasProjectAndPerm(Mockito.any(), Mockito.any(), (Map<String, Object>)Mockito.any())).thenReturn(true);

Expand All @@ -246,8 +246,8 @@ public void testCountDefinitionByUser() {
putMsg(result, Status.SUCCESS, null);
Mockito.when(projectService.checkProjectAndAuth(any(), any(), anyLong())).thenReturn(result);

Mockito.when(processDefinitionMapper.countDefinitionGroupByUser(Mockito.anyInt(), Mockito.any(Long[].class),
Mockito.anyBoolean())).thenReturn(new ArrayList<DefinitionGroupByUser>());
Mockito.when(processDefinitionMapper.countDefinitionByProjectCodes(
Mockito.any(Long[].class))).thenReturn(new ArrayList<DefinitionGroupByUser>());
result = dataAnalysisServiceImpl.countDefinitionByUser(user, 0);
Assert.assertEquals(Status.SUCCESS, result.get(Constants.STATUS));
}
Expand Down
27 changes: 11 additions & 16 deletions ...ler-dao/src/main/java/org/apache/dolphinscheduler/dao/mapper/ProcessDefinitionMapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

import org.apache.dolphinscheduler.dao.entity.DefinitionGroupByUser;
import org.apache.dolphinscheduler.dao.entity.ProcessDefinition;
import org.apache.dolphinscheduler.dao.entity.ProcessDefinitionLog;

import org.apache.ibatis.annotations.MapKey;
import org.apache.ibatis.annotations.Param;
Expand All @@ -30,7 +29,6 @@

import com.baomidou.mybatisplus.core.mapper.BaseMapper;
import com.baomidou.mybatisplus.core.metadata.IPage;
import com.baomidou.mybatisplus.extension.plugins.pagination.Page;

/**
* process definition mapper interface
Expand Down Expand Up @@ -65,7 +63,7 @@ public interface ProcessDefinitionMapper extends BaseMapper<ProcessDefinition> {
* verify process definition by name
*
* @param projectCode projectCode
* @param name name
* @param name name
* @return process definition
*/
ProcessDefinition verifyByDefineName(@Param("projectCode") long projectCode,
Expand All @@ -75,7 +73,7 @@ ProcessDefinition verifyByDefineName(@Param("projectCode") long projectCode,
* query process definition by name
*
* @param projectCode projectCode
* @param name name
* @param name name
* @return process definition
*/
ProcessDefinition queryByDefineName(@Param("projectCode") long projectCode,
Expand All @@ -92,11 +90,11 @@ ProcessDefinition queryByDefineName(@Param("projectCode") long projectCode,
/**
* process definition page
*
* @param page page
* @param searchVal searchVal
* @param userId userId
* @param page page
* @param searchVal searchVal
* @param userId userId
* @param projectCode projectCode
* @param isAdmin isAdmin
* @param isAdmin isAdmin
* @return process definition IPage
*/
IPage<ProcessDefinition> queryDefineListPaging(IPage<ProcessDefinition> page,
Expand Down Expand Up @@ -130,17 +128,14 @@ IPage<ProcessDefinition> queryDefineListPaging(IPage<ProcessDefinition> page,
List<ProcessDefinition> queryDefinitionListByTenant(@Param("tenantId") int tenantId);

/**
* count process definition group by user
* Statistics process definition group by project codes list
* <p>
* We only need project codes to determine whether the definition belongs to the user or not.
*
* @param userId userId
* @param projectCodes projectCodes
* @param isAdmin isAdmin
* @return process definition list
* @return definition group by user
*/
List<DefinitionGroupByUser> countDefinitionGroupByUser(
@Param("userId") Integer userId,
@Param("projectCodes") Long[] projectCodes,
@Param("isAdmin") boolean isAdmin);
List<DefinitionGroupByUser> countDefinitionByProjectCodes(@Param("projectCodes") Long[] projectCodes);

/**
* list all resource ids
Expand Down
Loading

0 comments on commit 98c7502

Please sign in to comment.