Skip to content

[Improvement][api] optimize api controller with AuthUtil#5422

Closed
haydenzhourepo wants to merge 18 commits intoapache:devfrom
haydenzhourepo:optimize-api-auth-util
Closed

[Improvement][api] optimize api controller with AuthUtil#5422
haydenzhourepo wants to merge 18 commits intoapache:devfrom
haydenzhourepo:optimize-api-auth-util

Conversation

@haydenzhourepo
Copy link
Contributor

Purpose of the pull request

Remove loginUser parameter in Controller's Handler, as described in #5339

Brief change log

  1. Remove loginUser parameter in Controller's Handler
  2. Create an AuthUtil to get loginUser info if needed.

Verify this pull request

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

@haydenzhourepo haydenzhourepo changed the title optimize api controller with AuthUtil [Improvement][api] optimize api controller with AuthUtil May 5, 2021
@CalvinKirs
Copy link
Member

CI is not triggered, can you resubmit the changes to trigger CI, (just a little change is needed)

@CalvinKirs CalvinKirs added the improvement make more easy to user or prompt friendly label May 6, 2021
@codecov-commenter
Copy link

codecov-commenter commented May 6, 2021

Codecov Report

❌ Patch coverage is 26.15385% with 96 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.65%. Comparing base (d93a9dc) to head (266c6f3).
⚠️ Report is 4314 commits behind head on dev.

Files with missing lines Patch % Lines
...nscheduler/api/controller/ResourcesController.java 0.00% 16 Missing ⚠️
...lphinscheduler/api/controller/UsersController.java 0.00% 15 Missing ⚠️
...uler/api/controller/ProcessInstanceController.java 0.00% 10 Missing ⚠️
...nscheduler/api/controller/SchedulerController.java 0.00% 8 Missing ⚠️
...scheduler/api/controller/DataSourceController.java 0.00% 7 Missing ⚠️
...cheduler/api/controller/AccessTokenController.java 0.00% 5 Missing ⚠️
...heduler/api/controller/DataAnalysisController.java 0.00% 5 Missing ⚠️
...scheduler/api/controller/AlertGroupController.java 0.00% 4 Missing ⚠️
.../api/controller/AlertPluginInstanceController.java 0.00% 4 Missing ⚠️
...hinscheduler/api/controller/MonitorController.java 0.00% 4 Missing ⚠️
... and 8 more
Additional details and impacted files
@@             Coverage Diff              @@
##                dev    #5422      +/-   ##
============================================
+ Coverage     45.21%   48.65%   +3.44%     
- Complexity     3395     3889     +494     
============================================
  Files           538      599      +61     
  Lines         23053    24307    +1254     
  Branches       2687     2782      +95     
============================================
+ Hits          10423    11827    +1404     
+ Misses        11698    11395     -303     
- Partials        932     1085     +153     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 7, 2021

@CalvinKirs CalvinKirs added the discussion discussion label May 7, 2021
@haydenzhourepo haydenzhourepo requested a review from CalvinKirs May 8, 2021 05:11
@ruanwenjun
Copy link
Member

Sorry, I think that if this pr just reduce the declared loginUser parameter in the controller, it doesn't make much sense, because the loginUser parameter still needs to be passed between multiple methods after modification.

If you want to reduce parameter passing, I would accept to use this implementation to achieve. https://www.programmersought.com/article/93593446439/

It is my personal opinion and does not represent the community, just for reference.

@haydenzhourepo
Copy link
Contributor Author

yes, we will use a threadlocal to store the loginuser info ultimately, i think there are too much changes if do this step now, many unit test related to it already。 so i thought split the changes by steps will be better for give more stable for project。

@github-actions
Copy link

github-actions bot commented Mar 1, 2022

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 github-actions bot added the Stale label Mar 1, 2022
@github-actions
Copy link

github-actions bot commented Mar 9, 2022

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.

@github-actions github-actions bot closed this Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

discussion discussion improvement make more easy to user or prompt friendly Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants