[python] Fix submit and run error #10792

zhongjiajie · 2022-07-05T07:59:56Z

because we add permission, so it failed out
createQueue method. this patch fix it and
do some refactor of our tenant and queue
validator code

close: #10794

because we add permission, so it failed out createQueue method. this patch fix it and do some refactor of our tenant and queue validator code

...heduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/QueueServiceImpl.java

...cheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/BaseServiceImpl.java

dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/TenantService.java

zhongjiajie · 2022-07-05T08:37:19Z

I changed the function name according to your suggestion @SbloodyS

zhongjiajie · 2022-07-05T08:37:51Z

I find out I forget add tests on it, will add it right now

...eduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/TenantServiceImpl.java

caishunfeng · 2022-07-05T08:51:57Z

...eduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/TenantServiceImpl.java

+        tenant.setUpdateTime(now);
+
+        // save
+        tenantMapper.insert(tenant);


Please use the original method to insert tenant, then it can keep the unified logic, such as add permission check, see #10718 (comment)

I think we should ignore the permission check currently for pydolphinscheudler, if we have to consider the permission, we have to create a role and connect the role for the current user who creates the workflow from pydolphinscheudler to define code.
But as we know, pydolphinscheudler is a batch operator API instead of an interactive API， which mean that it will batch create or update all related object when is submit. If users change the definition in

dolphinscheduler/dolphinscheduler-python/pydolphinscheduler/src/pydolphinscheduler/examples/tutorial.py

Lines 44 to 49 in a1aff51

with ProcessDefinition(

name="tutorial",

schedule="0 0 0 * * ? *",

start_time="2021-01-01",

tenant="tenant_exists",

) as pd:

like change users or tenant, it have to reflash all the user and role relation to make the change work.
The most important thing is, that it should work finally, otherwise, users will think there is a bug here, IMO is another method to ignore the permission. If we instead to add RBAC to pydolphinscheudler, will make dolphinscheduler have mulitple dummy role and the relation between roles and users

But I can indeed possible to abstract the insert part like below to a new private function:

Tenant tenant = new Tenant(); Date now = new Date(); tenant.setTenantCode(tenantCode); tenant.setQueueId(newQueue.getId()); tenant.setDescription(desc); tenant.setCreateTime(now); tenant.setUpdateTime(now); // save tenantMapper.insert(tenant);

like

private Tenant createRecordToDB(String tenantCode, int queueId) { Tenant tenant = new Tenant(); Date now = new Date(); tenant.setTenantCode(tenantCode); tenant.setQueueId(queueID); tenant.setDescription(desc); tenant.setCreateTime(now); tenant.setUpdateTime(now); // save tenantMapper.insert(tenant); return tenant; };

But I can indeed possible to abstract the insert part like below to a new private function:

Tenant tenant = new Tenant(); Date now = new Date(); tenant.setTenantCode(tenantCode); tenant.setQueueId(newQueue.getId()); tenant.setDescription(desc); tenant.setCreateTime(now); tenant.setUpdateTime(now); // save tenantMapper.insert(tenant);

like

private Tenant createRecordToDB(String tenantCode, int queueId) { Tenant tenant = new Tenant(); Date now = new Date(); tenant.setTenantCode(tenantCode); tenant.setQueueId(queueID); tenant.setDescription(desc); tenant.setCreateTime(now); tenant.setUpdateTime(now); // save tenantMapper.insert(tenant); return tenant; };

Yes, permission check is just a case, and maybe we will add other logic when create tenant or queue. If we use a new method, when someone modify the insert logic, he should modify two places, otherwise there may be problems.

But currently, python api will create or update objects with admin permission

zhongjiajie · 2022-07-05T11:14:54Z

I find out I forget add tests on it, will add it right now

Add in edc1ff8

codecov-commenter · 2022-07-05T11:29:18Z

Codecov Report

Merging #10792 (05e65e0) into dev (d2fe16d) will increase coverage by 0.06%.
The diff coverage is 33.67%.

❗ Current head 05e65e0 differs from pull request most recent head 09cb86b. Consider uploading reports for the commit 09cb86b to get more accurate results

@@             Coverage Diff              @@
##                dev   #10792      +/-   ##
============================================
+ Coverage     40.61%   40.67%   +0.06%     
- Complexity     4817     4826       +9     
============================================
  Files           902      913      +11     
  Lines         36221    36306      +85     
  Branches       3999     3989      -10     
============================================
+ Hits          14711    14768      +57     
- Misses        20045    20065      +20     
- Partials       1465     1473       +8

Impacted Files	Coverage Δ
...che/dolphinscheduler/api/k8s/K8sClientService.java	`4.25% <0.00%> (-2.13%)`	⬇️
...che/dolphinscheduler/api/python/PythonGateway.java	`21.24% <0.00%> (+1.81%)`	⬆️
...duler/api/service/impl/AccessTokenServiceImpl.java	`83.72% <ø> (ø)`
...eduler/api/service/impl/AlertGroupServiceImpl.java	`82.52% <ø> (+1.57%)`	⬆️
...eduler/api/service/impl/DataSourceServiceImpl.java	`29.49% <ø> (+0.19%)`	⬆️
...duler/api/service/impl/EnvironmentServiceImpl.java	`80.51% <ø> (ø)`
...api/service/impl/ProcessDefinitionServiceImpl.java	`31.37% <ø> (ø)`
...i/service/impl/ProcessTaskRelationServiceImpl.java	`21.03% <ø> (ø)`
...scheduler/api/service/impl/ProjectServiceImpl.java	`57.92% <ø> (ø)`
...heduler/api/service/impl/ResourcesServiceImpl.java	`48.56% <ø> (ø)`
... and 71 more

Continue to review full report at Codecov.

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

zhongjiajie · 2022-07-05T15:29:21Z

I add new commit f4fc406 to fix this #10792 (comment)

zhongjiajie · 2022-07-05T15:30:46Z

Maybe we should re-design the permission in pydolphinscheduler after we add role in our codebase

zhongjiajie · 2022-07-05T15:32:11Z

dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/enums/Status.java

@@ -194,7 +194,7 @@ public enum Status {
    QUERY_WORKFLOW_LINEAGE_ERROR(10161, "query workflow lineage error", "查询血缘失败"),
    QUERY_AUTHORIZED_AND_USER_CREATED_PROJECT_ERROR(10162, "query authorized and user created project error error", "查询授权的和用户创建的项目错误"),
    DELETE_PROCESS_DEFINITION_BY_CODE_FAIL(10163, "delete process definition by code fail, for there are {0} process instances in executing using it", "删除工作流定义失败，有[{0}]个运行中的工作流实例正在使用"),
-    CHECK_OS_TENANT_CODE_ERROR(10164, "Please enter the English os tenant code", "请输入英文操作系统租户"),
+    CHECK_OS_TENANT_CODE_ERROR(10164, "Tenant code invalid, should follow linux's users naming conventions", "非法的租户名，需要遵守 Linux 用户命名规范"),


for the reviewers, I change this message due to I think the pervious message is wrong

zhongjiajie · 2022-07-05T15:34:28Z

...hinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/QueueServiceTest.java

        // not exist
        Map<String, Object> result = queueService.updateQueue(getLoginUser(), 0, "queue", queueName);
-        logger.info(result.toString());


And I delete all the logger is our tests, because I think is redundance, correct me if I am wrong

zhongjiajie · 2022-07-07T04:06:02Z

I suggest python API is always created or updated in Admin role currently, and all our validate expect permission should including in valid function, which will call in both createTenant(use in web ui and open api) and createTenantIfNotExists(use in python api) to keep the same logic

zhongjiajie · 2022-07-07T09:19:51Z

...eduler-api/src/main/java/org/apache/dolphinscheduler/api/exceptions/ApiExceptionHandler.java

+    @ExceptionHandler(ServiceException.class)
+    public Result exceptionHandler(ServiceException e, HandlerMethod hm) {
+        logger.error("ServiceException: ", e);
+        return new Result(e.getCode(), e.getMessage());
+    }


I add a new handler to handle Service Exception throw from service side, according to @caishunfeng suggetion. So we can throw exception in service side, and keep our code clear

sonarcloud · 2022-07-07T11:45:55Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
18 Code Smells

79.6% Coverage
0.0% Duplication

zhongjiajie · 2022-07-07T12:08:09Z

Finally, the CI pass 🤣

SbloodyS

+1

zhongjiajie · 2022-07-08T02:54:54Z

Thanks all

because we add permission, so it failed out createQueue method. this patch fix it and do some refactor of our tenant and queue validator code

because we add permission, so it failed out createQueue method. this patch fix it and do some refactor of our tenant and queue validator code (cherry picked from commit ae6aa53)

zhongjiajie added 2 commits July 5, 2022 16:01

[python] Fix submit and run error

ecb0e71

because we add permission, so it failed out createQueue method. this patch fix it and do some refactor of our tenant and queue validator code

Add function comment

77dbef4

zhongjiajie force-pushed the f-py-api-submit branch from ccb6d01 to 77dbef4 Compare July 5, 2022 08:02

change newline char

ea84992

zhongjiajie commented Jul 5, 2022

View reviewed changes

...heduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/QueueServiceImpl.java Outdated Show resolved Hide resolved

...cheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/BaseServiceImpl.java Outdated Show resolved Hide resolved

zhongjiajie self-assigned this Jul 5, 2022

zhongjiajie added bug Something isn't working Python labels Jul 5, 2022

zhongjiajie marked this pull request as ready for review July 5, 2022 08:24

zhongjiajie requested review from caishunfeng and SbloodyS as code owners July 5, 2022 08:24

zhongjiajie added this to the 3.1.0-alpha milestone Jul 5, 2022

SbloodyS reviewed Jul 5, 2022

View reviewed changes

dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/TenantService.java Outdated Show resolved Hide resolved

change the interface name

9f3c412

caishunfeng reviewed Jul 5, 2022

View reviewed changes

Add some test about it

edc1ff8

Add new private method keep create function

f4fc406

zhongjiajie commented Jul 5, 2022

View reviewed changes

valid throw exception instead of return map

e389459

zhongjiajie mentioned this pull request Jul 7, 2022

[Python] Supports creating or editing resources. #10823

Merged

zhongjiajie mentioned this pull request Jul 7, 2022

[python] Integrate test run strategy change #10825

Merged

zhongjiajie added 2 commits July 7, 2022 15:31

correct status type and remove unused

1714952

Fix queue controller test error

08f49cc

zhongjiajie added 2 commits July 7, 2022 17:18

Fix tenant service test

09bfe2d

Remove unused putmsg method

a3b9cd6

zhongjiajie commented Jul 7, 2022

View reviewed changes

zhongjiajie added 4 commits July 7, 2022 17:42

throw execption directly in tenant service

b40e62f

Fix error

550f7d8

code format

55eeea7

fix ut

09cb86b

zhongjiajie requested a review from caishunfeng July 7, 2022 13:14

caishunfeng approved these changes Jul 8, 2022

View reviewed changes

SbloodyS approved these changes Jul 8, 2022

View reviewed changes

zhongjiajie merged commit ae6aa53 into apache:dev Jul 8, 2022

zhongjiajie deleted the f-py-api-submit branch July 8, 2022 02:54

This was referenced Jul 11, 2022

[fix] update queue and tenant with same code error #10877

Merged

[Feature][Python] Migrate all exists method call in PythonGateway to corresponding service #10989

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[python] Fix submit and run error #10792

[python] Fix submit and run error #10792

zhongjiajie commented Jul 5, 2022 •

edited

Loading

zhongjiajie commented Jul 5, 2022

zhongjiajie commented Jul 5, 2022

caishunfeng Jul 5, 2022

zhongjiajie Jul 5, 2022

zhongjiajie Jul 5, 2022 •

edited

Loading

caishunfeng Jul 6, 2022

zhongjiajie Jul 7, 2022

zhongjiajie commented Jul 5, 2022 •

edited

Loading

codecov-commenter commented Jul 5, 2022 •

edited

Loading

zhongjiajie commented Jul 5, 2022

zhongjiajie commented Jul 5, 2022

zhongjiajie Jul 5, 2022

zhongjiajie Jul 5, 2022

zhongjiajie commented Jul 7, 2022

zhongjiajie Jul 7, 2022

sonarcloud bot commented Jul 7, 2022

zhongjiajie commented Jul 7, 2022 •

edited

Loading

SbloodyS left a comment

zhongjiajie commented Jul 8, 2022

	with ProcessDefinition(
	name="tutorial",
	schedule="0 0 0 * * ? *",
	start_time="2021-01-01",
	tenant="tenant_exists",
	) as pd:

[python] Fix submit and run error #10792

[python] Fix submit and run error #10792

Conversation

zhongjiajie commented Jul 5, 2022 • edited Loading

zhongjiajie commented Jul 5, 2022

zhongjiajie commented Jul 5, 2022

caishunfeng Jul 5, 2022

Choose a reason for hiding this comment

zhongjiajie Jul 5, 2022

Choose a reason for hiding this comment

zhongjiajie Jul 5, 2022 • edited Loading

Choose a reason for hiding this comment

caishunfeng Jul 6, 2022

Choose a reason for hiding this comment

zhongjiajie Jul 7, 2022

Choose a reason for hiding this comment

zhongjiajie commented Jul 5, 2022 • edited Loading

codecov-commenter commented Jul 5, 2022 • edited Loading

Codecov Report

zhongjiajie commented Jul 5, 2022

zhongjiajie commented Jul 5, 2022

zhongjiajie Jul 5, 2022

Choose a reason for hiding this comment

zhongjiajie Jul 5, 2022

Choose a reason for hiding this comment

zhongjiajie commented Jul 7, 2022

zhongjiajie Jul 7, 2022

Choose a reason for hiding this comment

sonarcloud bot commented Jul 7, 2022

zhongjiajie commented Jul 7, 2022 • edited Loading

SbloodyS left a comment

Choose a reason for hiding this comment

zhongjiajie commented Jul 8, 2022

zhongjiajie commented Jul 5, 2022 •

edited

Loading

zhongjiajie Jul 5, 2022 •

edited

Loading

zhongjiajie commented Jul 5, 2022 •

edited

Loading

codecov-commenter commented Jul 5, 2022 •

edited

Loading

zhongjiajie commented Jul 7, 2022 •

edited

Loading