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 the NullPointerException when "start param to overwrite global param" #15677

Merged
merged 2 commits into from
Mar 15, 2024
Merged

Fix the NullPointerException when "start param to overwrite global param" #15677

merged 2 commits into from
Mar 15, 2024

Conversation

wustlz
Copy link
Contributor

@wustlz wustlz commented Mar 6, 2024 •
edited
Loading

Purpose of the pull request

This pull request to fix #15676 , fix the NullPointerException by checking whether the key exists when "start param to overwrite global param"

Brief change log

Add a "containsKey" judgement when "start param to overwrite global param"

Verify this pull request

This pull request is code cleanup without any test coverage.

@github-actions github-actions bot added UI ui and front end related backend test CI&CD e2e e2e test document labels Mar 6, 2024
@rickchengx rickchengx added the first time contributor First-time contributor label Mar 6, 2024
@rickchengx
Copy link
Contributor

@wustlz please rebase your PR and link this to an issue

@github-actions github-actions bot removed UI ui and front end related test CI&CD e2e e2e test document labels Mar 6, 2024
@wustlz
Copy link
Contributor Author

wustlz commented Mar 6, 2024

@wustlz please rebase your PR and link this to an issue

done

ruanwenjun
ruanwenjun reviewed Mar 7, 2024
View reviewed changes
...er-service/src/main/java/org/apache/dolphinscheduler/service/process/ProcessServiceImpl.java Outdated
Comment on lines 604 to 609
if (startParamMap.containsKey(param.getKey())) {
String val = startParamMap.get(param.getKey()).getValue();
if (val != null) {
param.setValue(val);
}
Copy link
Member

@ruanwenjun ruanwenjun Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your PR, please add UT for this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ruanwenjun ruanwenjun added this to the 3.2.2 milestone Mar 7, 2024
@SbloodyS SbloodyS added bug Something isn't working 3.2.2 labels Mar 8, 2024
rickchengx
rickchengx reviewed Mar 12, 2024
View reviewed changes
Copy link
Contributor

@rickchengx rickchengx left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@wustlz overall LGTM, please Run 'mvn spotless:apply' to fix the code style

@wustlz wustlz closed this Mar 12, 2024
@wustlz wustlz reopened this Mar 13, 2024
@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 80.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 39.07%. Comparing base (eab71f1) to head (fcaa484).
Report is 1 commits behind head on dev.

❗ Current head fcaa484 differs from pull request most recent head 87b614d. Consider uploading reports for the commit 87b614d to get more accurate results

Files Patch % Lines
...nscheduler/service/process/ProcessServiceImpl.java 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##                dev   #15677      +/-   ##
============================================
+ Coverage     39.05%   39.07%   +0.01%     
- Complexity     4845     4849       +4     
============================================
  Files          1316     1316              
  Lines         45012    45014       +2     
  Branches       4818     4819       +1     
============================================
+ Hits          17581    17589       +8     
+ Misses        25525    25517       -8     
- Partials       1906     1908       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rickchengx
rickchengx approved these changes Mar 15, 2024
View reviewed changes
Copy link
Contributor

@rickchengx rickchengx left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Mar 15, 2024

Quality Gate Passed Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@rickchengx rickchengx merged commit e97f6a4 into apache:dev Mar 15, 2024
56 checks passed
@ruanwenjun ruanwenjun mentioned this pull request Jun 23, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruanwenjun ruanwenjun ruanwenjun left review comments

@rickchengx rickchengx rickchengx approved these changes

@EricGao888 EricGao888 Awaiting requested review from EricGao888

@songjianet songjianet Awaiting requested review from songjianet

@Amy0104 Amy0104 Awaiting requested review from Amy0104

@caishunfeng caishunfeng Awaiting requested review from caishunfeng caishunfeng is a code owner

@SbloodyS SbloodyS Awaiting requested review from SbloodyS

@zhuangchong zhuangchong Awaiting requested review from zhuangchong

Assignees

@wustlz wustlz

Labels
3.2.2 backend bug Something isn't working first time contributor First-time contributor ready-to-merge
Projects
None yet
Milestone
3.2.2
Development

Successfully merging this pull request may close these issues.

[Bug] [Master] Create WorkflowExecuteRunnable failed
5 participants
@wustlz @rickchengx @codecov-commenter @ruanwenjun @SbloodyS