[Feature] Add params check in createUser Function by ly1ex · Pull Request #11255 · apache/dolphinscheduler

ly1ex · 2022-08-02T08:19:47Z

Purpose of the pull request

close #11277

Add params check in createUser Function

Brief change log

Add params check in createUser Function

Verify this pull request

This pull request is already covered by existing tests, such as (please describe tests).

ly1ex · 2022-08-02T08:21:50Z

PTAL @zhongjiajie

codecov-commenter · 2022-08-02T11:28:26Z

Codecov Report

Merging #11255 (d8b6a51) into dev (f8b9aad) will decrease coverage by 0.00%.
The diff coverage is 55.31%.

@@             Coverage Diff              @@
##                dev   #11255      +/-   ##
============================================
- Coverage     39.59%   39.58%   -0.01%     
- Complexity     4689     4696       +7     
============================================
  Files          1014     1014              
  Lines         37918    37910       -8     
  Branches       4238     4235       -3     
============================================
- Hits          15013    15007       -6     
- Misses        21297    21301       +4     
+ Partials       1608     1602       -6

Impacted Files	Coverage Δ
...a/org/apache/dolphinscheduler/dao/entity/User.java	`63.76% <16.66%> (-10.37%)`	⬇️
...inscheduler/api/service/impl/UsersServiceImpl.java	`74.04% <68.57%> (+1.75%)`	⬆️
...inscheduler/server/log/LoggerRequestProcessor.java	`17.30% <0.00%> (-0.17%)`	⬇️
...ver/master/runner/task/ConditionTaskProcessor.java	`4.76% <0.00%> (-0.06%)`	⬇️
.../server/master/runner/WorkflowExecuteRunnable.java	`7.97% <0.00%> (-0.03%)`	⬇️
...server/master/runner/task/SwitchTaskProcessor.java	`2.52% <0.00%> (-0.03%)`	⬇️
...e/dolphinscheduler/server/master/MasterServer.java	`0.00% <0.00%> (ø)`
...e/dolphinscheduler/server/worker/WorkerServer.java	`0.00% <0.00%> (ø)`
...r/api/service/impl/ProcessInstanceServiceImpl.java	`60.85% <0.00%> (ø)`
... and 8 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

sonarqubecloud · 2022-08-02T11:38:46Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

59.0% Coverage
0.0% Duplication

zhuxt2015 · 2022-08-02T12:28:43Z

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

+            user.setUserName(userName);
+        }
+
+        if (StringUtils.isNotEmpty(userPassword)) {


Please merge if statement

Please merge if statement

Sorry, I don't get your point...

I mean please fix code smell

I think OK, and merging can lead to unclear exception semantics.
sometimes, code smell needs to be judged based on the actual situation.

Yeah, agree with Calvin

EricGao888 · 2022-08-03T03:00:42Z

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

-        user.setUserName(userName);
-        user.setUserPassword(EncryptionUtils.getMd5(userPassword));
-        user.setEmail(email);
+        if (StringUtils.isNotEmpty(userName)) {


Hello @lyleshaw , thx for this improvement. Since you have added several parameters checks, could you please update related unit tests to get those checks covered? Thx

EricGao888

We need to update corresponding unit tests to get those new checks covered.

ly1ex · 2022-08-03T03:04:17Z

We need to update corresponding unit tests to get those new checks covered.

Got it.

zhongjiajie · 2022-08-03T03:22:43Z

@lyleshaw Please add a issue to target this PR, BTW, can we add some separate function from create and update function? I have do the same thing in Tenant and Queue

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

Lines 70 to 110 in 78e9122

    
               /** 
        
                * Check the queue new object valid or not 
        
                * 
        
                * @param queue The queue object want to create 
        
                */ 
        
               private void createQueueValid(Queue queue) throws ServiceException { 
        
                   if (StringUtils.isEmpty(queue.getQueue())) { 
        
                       throw new ServiceException(Status.REQUEST_PARAMS_NOT_VALID_ERROR, Constants.QUEUE); 
        
                   } else if (StringUtils.isEmpty(queue.getQueueName())) { 
        
                       throw new ServiceException(Status.REQUEST_PARAMS_NOT_VALID_ERROR, Constants.QUEUE_NAME); 
        
                   } else if (checkQueueExist(queue.getQueue())) { 
        
                       throw new ServiceException(Status.QUEUE_VALUE_EXIST, queue.getQueue()); 
        
                   } else if (checkQueueNameExist(queue.getQueueName())) { 
        
                       throw new ServiceException(Status.QUEUE_NAME_EXIST, queue.getQueueName()); 
        
                   } 
        
               } 
        
               /** 
        
                * Check queue update object valid or not 
        
                * 
        
                * @param existsQueue The exists queue object 
        
                * @param updateQueue The queue object want to update 
        
                */ 
        
               private void updateQueueValid(Queue existsQueue, Queue updateQueue) throws ServiceException { 
        
                   // Check the exists queue and the necessary of update operation, in not exist checker have to use updateQueue to avoid NPE 
        
                   if (Objects.isNull(existsQueue)) { 
        
                       throw new ServiceException(Status.QUEUE_NOT_EXIST, updateQueue.getQueue()); 
        
                   } else if (Objects.equals(existsQueue, updateQueue)) { 
        
                       throw new ServiceException(Status.NEED_NOT_UPDATE_QUEUE); 
        
                   } 
        
                   // Check the update queue parameters 
        
                   else if (StringUtils.isEmpty(updateQueue.getQueue())) { 
        
                       throw new ServiceException(Status.REQUEST_PARAMS_NOT_VALID_ERROR, Constants.QUEUE); 
        
                   } else if (StringUtils.isEmpty(updateQueue.getQueueName())) { 
        
                       throw new ServiceException(Status.REQUEST_PARAMS_NOT_VALID_ERROR, Constants.QUEUE_NAME); 
        
                   } else if (!Objects.equals(updateQueue.getQueue(), existsQueue.getQueue()) && checkQueueExist(updateQueue.getQueue())) { 
        
                       throw new ServiceException(Status.QUEUE_VALUE_EXIST, updateQueue.getQueue()); 
        
                   } else if (!Objects.equals(updateQueue.getQueueName(), existsQueue.getQueueName()) && checkQueueNameExist(updateQueue.getQueueName())) { 
        
                       throw new ServiceException(Status.QUEUE_NAME_EXIST, updateQueue.getQueueName()); 
        
                   } 
        
               }

ly1ex · 2022-08-03T09:10:53Z

Hi all, I have solved the above problems (including adding tests, abstracting to checkParams functions, etc.)
Please feel free to re-review it.
@zhongjiajie @EricGao888

zhongjiajie · 2022-08-04T03:40:47Z

Hi all, I have solved the above problems (including adding tests, abstracting to checkParams functions, etc.)
Please feel free to re-review it.
@zhongjiajie @EricGao888

@lyleshaw But I find out we have some failed CI step, could you please make a check?

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

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

zhongjiajie · 2022-09-05T14:04:12Z

@lyleshaw It seems dwonload jdbc driver error, could you try to rebase on upstream/dev, and force push to restart CI?

ly1ex · 2022-09-05T14:27:56Z

@lyleshaw It seems dwonload jdbc driver error, could you try to rebase on upstream/dev, and force push to restart CI?

I usually use git merge instead of git rebase, hoping things won't go wrong...

sonarqubecloud · 2022-09-05T16:06:43Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

70.7% Coverage
0.0% Duplication

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


-        user.setPhone(phone);
+        user = new User(userName, userPassword, email, phone, tenantId, state);
+        if (state == 0 && user.getState() != state && loginUser.getId() == user.getId()) {


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

-        String phone = "123456789";
+        String phone = "17366666666";
        String tenantCode = "tenantCode";
+        Integer userId = 1;


- Add params check in createUser Function

…/api/service/impl/UsersServiceImpl.java Co-authored-by: Jiajie Zhong <zhongjiajie955@gmail.com>

github-actions · 2024-02-09T00:19:16Z

This pull request has been automatically marked as stale because it has not had recent activity for 120 days. It will be closed in 7 days if no further activity occurs.

github-actions · 2024-02-16T00:19:22Z

This pull request has been closed because it has not had recent activity. You could reopen it if you try to continue your work, and anyone who are interested in it are encouraged to continue work on this pull request.

ly1ex requested review from SbloodyS and caishunfeng as code owners August 2, 2022 08:19

github-actions bot added the backend label Aug 2, 2022

SbloodyS assigned ly1ex Aug 2, 2022

SbloodyS added the feature new feature label Aug 2, 2022

SbloodyS added this to the 3.1.0-alpha milestone Aug 2, 2022

zhuxt2015 reviewed Aug 2, 2022

View reviewed changes

EricGao888 reviewed Aug 3, 2022

View reviewed changes

EricGao888 requested changes Aug 3, 2022

View reviewed changes

ly1ex requested review from CalvinKirs, EricGao888 and zhongjiajie August 3, 2022 09:11

zhongjiajie reviewed Aug 4, 2022

View reviewed changes

ly1ex commented Aug 11, 2022

View reviewed changes

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

zhongjiajie reviewed Aug 12, 2022

View reviewed changes

ly1ex force-pushed the feat/add-params-check-in-create-user branch from d8b6a51 to d63c5c4 Compare September 5, 2022 14:26

github-advanced-security bot found potential problems Sep 21, 2022

View reviewed changes

xiaoliangyu added 3 commits September 22, 2022 12:11

[Feature] Add params check in createUser Function

ffbddd0

- Add params check in createUser Function

[Fix] fix bug

308913f

[Fix] fix problems in code review

926ab91

xiaoliangyu and others added 10 commits September 23, 2022 15:57

[Feature] add tests

767dda3

fit ci

d1db85d

Update dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler…

90f5fd4

…/api/service/impl/UsersServiceImpl.java Co-authored-by: Jiajie Zhong <zhongjiajie955@gmail.com>

Update dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler…

82ea42c

…/api/service/impl/UsersServiceImpl.java Co-authored-by: Jiajie Zhong <zhongjiajie955@gmail.com>

Update dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler…

a32e27f

…/api/service/impl/UsersServiceImpl.java Co-authored-by: Jiajie Zhong <zhongjiajie955@gmail.com>

Update dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler…

d230e96

…/api/service/impl/UsersServiceImpl.java Co-authored-by: Jiajie Zhong <zhongjiajie955@gmail.com>

[Feature] fit code review comment

fb548bd

you should change UT testRegisterUser too

2d1c5c1

[fix]

b68ace0

[fix] pass tests

fa22386

ly1ex force-pushed the feat/add-params-check-in-create-user branch from 7165802 to fa22386 Compare September 23, 2022 08:04

caishunfeng modified the milestones: 3.1.0, 3.2.0 Sep 27, 2022

zhongjiajie modified the milestones: 3.2.0, 3.3.0 Aug 30, 2023

github-actions bot added the Stale label Feb 9, 2024

github-actions bot closed this Feb 16, 2024

Conversation

ly1ex commented Aug 2, 2022 • edited by SbloodyS Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of the pull request

Brief change log

Verify this pull request

Uh oh!

ly1ex commented Aug 2, 2022

Uh oh!

codecov-commenter commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sonarqubecloud bot commented Aug 2, 2022

Uh oh!

zhuxt2015 Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

ly1ex Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

zhuxt2015 Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

CalvinKirs Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

zhongjiajie Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

EricGao888 Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

EricGao888 left a comment

Choose a reason for hiding this comment

Uh oh!

ly1ex commented Aug 3, 2022

Uh oh!

zhongjiajie commented Aug 3, 2022

Uh oh!

ly1ex commented Aug 3, 2022

Uh oh!

zhongjiajie commented Aug 4, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zhongjiajie commented Sep 5, 2022

Uh oh!

ly1ex commented Sep 5, 2022

Uh oh!

sonarqubecloud bot commented Sep 5, 2022

Uh oh!

Check failure

Check notice

github-actions bot commented Feb 9, 2024

Uh oh!

github-actions bot commented Feb 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ly1ex commented Aug 2, 2022 •

edited by SbloodyS

Loading

codecov-commenter commented Aug 2, 2022 •

edited

Loading