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.

Sign up for GitHub

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

Jump to bottom

[Fix-16682][ApiServer] like query with % support special characher(% \ _) #16683

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from
+14 −22
Open

[Fix-16682][ApiServer] like query with % support special characher(% \ _) #16683

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0873df0
bugfix:`Like` text query condition support (% \ _)
shouwangyw Oct 10, 2024
94635f8
fix: Remove some don't need % escape processing
shouwangyw Oct 11, 2024
7b9fc48
Merge branch 'dev' into bug/bugfix-16682
SbloodyS Oct 12, 2024
e929410
Merge branch 'dev' into bug/bugfix-16682
shouwangyw Oct 12, 2024
d8a1e94
Merge branch 'dev' into bug/bugfix-16682
Gallardot Oct 16, 2024
5f9cacd
Merge branch 'dev' into bug/bugfix-16682
shouwangyw Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions ...ler-api/src/main/java/org/apache/dolphinscheduler/api/controller/ResourcesController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@
import org.apache.dolphinscheduler.api.vo.resources.FetchFileContentResponse;
import org.apache.dolphinscheduler.common.constants.Constants;
import org.apache.dolphinscheduler.dao.entity.User;
import org.apache.dolphinscheduler.plugin.task.api.utils.ParameterUtils;
import org.apache.dolphinscheduler.spi.enums.ResourceType;

import org.apache.commons.lang3.StringUtils;
Expand Down Expand Up @@ -261,7 +260,7 @@ public Result<PageInfo<ResourceItemVO>> pagingResourceItemRequest(@Parameter(hid
.loginUser(loginUser)
.resourceAbsolutePath(resourceAbsolutePath)
.resourceType(resourceType)
.resourceNameKeyWord(StringUtils.trim(ParameterUtils.handleEscapes(resourceNameKeyWord)))
.resourceNameKeyWord(StringUtils.trim(resourceNameKeyWord))
.pageNo(pageNo)
.pageSize(pageSize)
.build();
Expand Down
10 changes: 1 addition & 9 deletions ...heduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/UsersController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@

import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

import lombok.extern.slf4j.Slf4j;

Expand Down Expand Up @@ -497,10 +496,6 @@ public Result<Object> registerUser(@RequestParam(value = "userName") String user
@RequestParam(value = "userPassword") String userPassword,
@RequestParam(value = "repeatPassword") String repeatPassword,
@RequestParam(value = "email") String email) throws Exception {
userName = ParameterUtils.handleEscapes(userName);
userPassword = ParameterUtils.handleEscapes(userPassword);
repeatPassword = ParameterUtils.handleEscapes(repeatPassword);
email = ParameterUtils.handleEscapes(email);
Result<Object> verifyRet = usersService.verifyUserName(userName);
if (verifyRet.getCode() != Status.SUCCESS.getCode()) {
return verifyRet;
Expand All @@ -523,7 +518,6 @@ public Result<Object> registerUser(@RequestParam(value = "userName") String user
@ApiException(UPDATE_USER_ERROR)
public Result<Object> activateUser(@Parameter(hidden = true) @RequestAttribute(value = Constants.SESSION_USER) User loginUser,
@RequestParam(value = "userName") String userName) {
userName = ParameterUtils.handleEscapes(userName);
Map<String, Object> result = usersService.activateUser(loginUser, userName);
return returnDataList(result);
}
Expand All @@ -542,9 +536,7 @@ public Result<Object> activateUser(@Parameter(hidden = true) @RequestAttribute(v
@ApiException(UPDATE_USER_ERROR)
public Result<Object> batchActivateUser(@Parameter(hidden = true) @RequestAttribute(value = Constants.SESSION_USER) User loginUser,
@RequestBody List<String> userNames) {
List<String> formatUserNames =
userNames.stream().map(ParameterUtils::handleEscapes).collect(Collectors.toList());
Map<String, Object> result = usersService.batchActivateUser(loginUser, formatUserNames);
Map<String, Object> result = usersService.batchActivateUser(loginUser, userNames);
return returnDataList(result);
}
}
2 changes: 0 additions & 2 deletions ...r-api/src/main/java/org/apache/dolphinscheduler/api/controller/WorkerGroupController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import org.apache.dolphinscheduler.api.utils.Result;
import org.apache.dolphinscheduler.common.constants.Constants;
import org.apache.dolphinscheduler.dao.entity.User;
import org.apache.dolphinscheduler.plugin.task.api.utils.ParameterUtils;

import java.util.Map;

Expand Down Expand Up @@ -117,7 +116,6 @@ public Result queryAllWorkerGroupsPaging(@Parameter(hidden = true) @RequestAttri
@RequestParam("pageSize") Integer pageSize,
@RequestParam(value = "searchVal", required = false) String searchVal) {
checkPageParams(pageNo, pageSize);
searchVal = ParameterUtils.handleEscapes(searchVal);
return workerGroupService.queryAllGroupPaging(loginUser, pageNo, pageSize, searchVal);
}

Expand Down
2 changes: 0 additions & 2 deletions ...i/src/main/java/org/apache/dolphinscheduler/api/controller/WorkflowLineageController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import org.apache.dolphinscheduler.dao.entity.User;
import org.apache.dolphinscheduler.dao.entity.WorkFlowLineage;
import org.apache.dolphinscheduler.dao.entity.WorkFlowRelationDetail;
import org.apache.dolphinscheduler.plugin.task.api.utils.ParameterUtils;

import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -76,7 +75,6 @@ public class WorkflowLineageController extends BaseController {
public Result<List<WorkFlowRelationDetail>> queryWorkFlowLineageByName(@Parameter(hidden = true) @RequestAttribute(value = SESSION_USER) User loginUser,
@Parameter(name = "projectCode", description = "PROJECT_CODE", required = true) @PathVariable long projectCode,
@RequestParam(value = "workflowDefinitionName", required = false) String workflowDefinitionName) {
workflowDefinitionName = ParameterUtils.handleEscapes(workflowDefinitionName);
List<WorkFlowRelationDetail> workFlowLineages =
workflowLineageService.queryWorkFlowLineageByName(projectCode, workflowDefinitionName);
return Result.success(workFlowLineages);
Expand Down
15 changes: 9 additions & 6 deletions ...k-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/utils/ParameterUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,15 +314,18 @@ private static String dateTemplateParse(String templateStr, Date date) {
/**
* handle escapes
*
* @param inputString input string
* @param str input string
* @return string filter escapes
*/
public static String handleEscapes(String inputString) {

if (!StringUtils.isEmpty(inputString)) {
return inputString.replace("%", "////%").replaceAll("[\n|\r\t]", "_");
public static String handleEscapes(String str) {
str = StringUtils.trim(str);
if (StringUtils.isNotBlank(str)) {
str = str.replace("\\", "\\\\");
str = str.replace("_", "\\_");
str = str.replace("%", "\\%");
str = str.replaceAll("[\n|\r\t]", "");
Comment on lines +320 to +326
Copy link
Member

Choose a reason for hiding this comment

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

This is a general processing method, and we should not modify it. Workflow name and task name should not contain special characters such as %. We should add verification and documentation when creating it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a general processing method, and we should not modify it. Workflow name and task name should not contain special characters such as %. We should add verification and documentation when creating it.

I understand that this method is intended for special characters, and should be able to support like fuzzy queries.
And for general processing the character _ and \ should also be supported.This is not just a processing for workflow name and task name,validation should be implemented elsewhere.

Here are some self-tests：
image
image

Copy link
Member

Choose a reason for hiding this comment

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

Different fields should use different verification methods. For example, the workflow name should consist of uppercase and lowercase English letters or Chinese and underscores, and other symbols are illegal characters. But the workflow description can contain more special characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Different fields should use different verification methods. For example, the workflow name should consist of uppercase and lowercase English letters or Chinese and underscores, and other symbols are illegal characters. But the workflow description can contain more special characters.

I understand this, but this general method is not for verification, but for supporting special characters in like fuzzy queries.

Copy link
Member

Choose a reason for hiding this comment

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

This is a general processing method, and we should not modify it. Workflow name and task name should not contain special characters such as %. We should add verification and documentation when creating it.

This issue only solves the problem of workflow list query. We should follow this, and the rest do not belong to this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This issue is not just about solving the workflow list query problem, the issue only takes the workflow list as an example. This is indeed a general process.

If so, we need to analyze specific problems, because different query scenarios need to use different rules. The current implementation needs optimization.

This process involves many calls, and every time you write a new query list interface, you may need to call this method.
This may not be a good implementation, but it is the smallest change,for the time being, this is also implemented in our actual project

Copy link
Member

Choose a reason for hiding this comment

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

Since the caller of this method is too extensive, it is impossible to comprehensively evaluate the influence surface. In order to reduce the impact and achieve the optimization effect, it is better to deal with it separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the caller of this method is too extensive, it is impossible to comprehensively evaluate the influence surface. In order to reduce the impact and achieve the optimization effect, it is better to deal with it separately.

sure, and some places have not been searched by like, i will check it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have checked all the calling places, and most of them use like % query. In addition, remove some don't need % escapr processing. @SbloodyS

Copy link
Member

Choose a reason for hiding this comment

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

I have checked all the calling places, and most of them use like % query. In addition, remove some don't need % escapr processing. @SbloodyS

Since the caller of this method is too extensive, it is impossible to comprehensively evaluate the influence surface. In order to reduce the impact and achieve the optimization effect, it is better to deal with it separately.

The scope of this revision is relatively large and it does not belong to the description scope of this issue #16682 . In this Fix PR, we only need to extract the query of workflow list separately and fix it. The other is to create a DSIP for scheme design and discussion.

}
return inputString;
return str;
}

/**
Expand Down
4 changes: 3 additions & 1 deletion ...i/src/test/java/org/apache/dolphinscheduler/plugin/task/api/utils/ParameterUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,9 @@ public void testHandleEscapes() throws Exception {
Assertions.assertNull(ParameterUtils.handleEscapes(null));
Assertions.assertEquals("", ParameterUtils.handleEscapes(""));
Assertions.assertEquals("test Parameter", ParameterUtils.handleEscapes("test Parameter"));
Assertions.assertEquals("////%test////%Parameter", ParameterUtils.handleEscapes("%test%Parameter"));
Assertions.assertEquals("\\%test\\%Parameter", ParameterUtils.handleEscapes("%test%Parameter"));
Assertions.assertEquals("\\_test\\_Parameter", ParameterUtils.handleEscapes("_test_Parameter"));
Assertions.assertEquals("\\\\test\\\\Parameter", ParameterUtils.handleEscapes("\\test\\Parameter"));
}

}
Loading