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

Never use /api/v1 from Gitea UI Pages #19318

Merged
merged 7 commits into from
Apr 7, 2022
Merged

Never use /api/v1 from Gitea UI Pages #19318

merged 7 commits into from
Apr 7, 2022

Conversation

lunny
Copy link
Member

@lunny lunny commented Apr 4, 2022

Reusing /api/v1 from Gitea UI Pages have pros and cons.
Pros:

  1. Less code copy

Cons:

  1. API/v1 have to support shared session with page requests.
  2. You need to consider for each other when you want to change something about api/v1 or page.

This PR moves all dependencies to API/v1 from UI Pages.

Partially replace #16052

@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Apr 4, 2022
@lunny lunny added this to the 1.17.0 milestone Apr 4, 2022
This was referenced Apr 4, 2022
Merged
Closed
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 4, 2022 •
edited
Loading

If the handlers for /api/ui/ are the same as /api/v1, can these ui handlers just proxy/pass the request to the v1 handlers? IIRC the newly added code are copied from the v1 handlers.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 4, 2022
@lunny
Copy link
Member Author

lunny commented Apr 4, 2022

If the handlers for /api/ui/ are the same as /api/v1, can these ui handlers just proxy/pass the request to the v1 handlers? IIRC the newly added code are copied from the v1 handlers.

They cannot be reused simply. We need many work to do like what I tried in #16052. Currently they are just copied but I think they need to be changed in coming PRs.

wxiaoguang
wxiaoguang approved these changes Apr 4, 2022
View reviewed changes
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

If copying code here is the only choice, then it's fine.

Another question is can these code be covered by tests?

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 4, 2022
@6543
Copy link
Member

6543 commented Apr 5, 2022

@lunny you missed one:

repo/issue/view_content/sidebar.tmpl at line 432

@lunny
Copy link
Member Author

lunny commented Apr 6, 2022

Thanks, will fix that later.

6543
6543 reviewed Apr 7, 2022
View reviewed changes
integrations/org_test.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 7, 2022
@6543 6543 merged commit 783a021 into go-gitea:main Apr 7, 2022
@6543 6543 mentioned this pull request Apr 7, 2022
Merged
@lunny lunny deleted the lunny/seperate_apis branch April 7, 2022 23:54
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 11, 2022
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
6764005 
* giteaofficial/main: (22 commits)
  Add logic to switch between source/rendered on Markdown (go-gitea#19356)
  Fixed registry host value. (go-gitea#19363)
  [skip ci] Updated translations via Crowdin
  Allow package linking to private repository (go-gitea#19348)
  Use "main" as default branch name (go-gitea#19354)
  Move milestone to models/issues/ (go-gitea#19278)
  Refactor CSRF protection modules, make sure CSRF tokens can be up-to-date. (go-gitea#19337)
  Remove dependent on session auth for api/v1 routers (go-gitea#19321)
  API: Search Issues, dont show 500 if filter result in empty list (go-gitea#19244)
  [skip ci] Updated translations via Crowdin
  Never use /api/v1 from Gitea UI Pages (go-gitea#19318)
  [skip ci] Updated translations via Crowdin
  Show ssh command directly in template instead of i18n translation (go-gitea#19335)
  Package registry changes (go-gitea#19305)
  [skip ci] Updated translations via Crowdin
  Add `ENABLE_SSH_LOG` to debugging problems (go-gitea#19316)
  Warn on SSH connection for incorrect configuration (go-gitea#19317)
  escape fake link
  Allow custom redirect for landing page (go-gitea#19324)
  [skip ci] Updated translations via Crowdin
  ...
@wxiaoguang wxiaoguang mentioned this pull request Apr 12, 2022
Closed
@Gusted Gusted mentioned this pull request May 3, 2022
Merged
Gusted pushed a commit to Gusted/gitea that referenced this pull request Jun 4, 2022
Move /info outside authorization
6bffaf5 
- To use the web's API to get information about a issue/pull on a
repository, doesn't require authorization(nor that the repository isn't
archived).
- Regressed by: go-gitea#19318
@Gusted Gusted mentioned this pull request Jun 4, 2022
Merged
zeripath added a commit that referenced this pull request Jun 4, 2022
@zeripath
Move /info outside authorization (#19888)
744e452 
- To use the web's API to get information about a issue/pull on a
repository, doesn't require authorization(nor that the repository isn't
archived).
- Regressed by: #19318


Signed-off-by: Andrew Thornton <[email protected]>
Co-authored-by: Andrew Thornton <[email protected]>
This was referenced Jul 28, 2022
Closed
Open
noerw added a commit to noerw/gitea that referenced this pull request Jul 31, 2022
@noerw
fix remaining test cases
0930750 
the refactor in go-gitea#19318 skipped changing TestSearchIssuesWithLabels,
so this is changed to, so we don't have duplicate tests 🙃
@noerw noerw mentioned this pull request Jul 31, 2022
Merged
@wxiaoguang wxiaoguang mentioned this pull request Aug 2, 2022
Closed
@ghost ghost mentioned this pull request Aug 3, 2022
Closed
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
@lunny @AbdulrhmnGhanem
Never use /api/v1 from Gitea UI Pages (go-gitea#19318)
a9ba713 
Reusing `/api/v1` from Gitea UI Pages have pros and cons.
Pros:
1) Less code copy

Cons:
1) API/v1 have to support shared session with page requests.
2) You need to consider for each other when you want to change something about api/v1 or page.

This PR moves all dependencies to API/v1 from UI Pages.

Partially replace go-gitea#16052
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
@zeripath @AbdulrhmnGhanem
Move /info outside authorization (go-gitea#19888)
99703aa 
- To use the web's API to get information about a issue/pull on a
repository, doesn't require authorization(nor that the repository isn't
archived).
- Regressed by: go-gitea#19318


Signed-off-by: Andrew Thornton <[email protected]>
Co-authored-by: Andrew Thornton <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@6543 6543 6543 approved these changes

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@zeripath zeripath Awaiting requested review from zeripath

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.
4 participants
@lunny @wxiaoguang @6543 @GiteaBot