Skip to content

Simple fix release attachment links to human readable #21798

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.

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

Closed
wants to merge 1 commit into from
Closed

Simple fix release attachment links to human readable #21798

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 13, 2022

close #10919, which is stalled for a long time.

Before

https://gitea.com/attachments/c15910d0-736c-4dc3-846b-6440ad2048fd

After

https://gitea.com/gitea/tea/releases/download/v0.9.0/tea-0.9.0-darwin-amd64

Next step:

@ghost ghost requested a review from wxiaoguang November 13, 2022 08:33
@ghost ghost added this to the 1.19.0 milestone Nov 13, 2022
@ghost ghost added type/enhancement An improvement of existing functionality topic/ui Change the appearance of the Gitea UI labels Nov 13, 2022
@ghost ghost requested review from 6543 and jolheiser November 13, 2022 08:36
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

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

Tested it on try.gitea using

document.querySelector('a[href="https://gitea.com/attachments/e670ea07-5998-443d-95c0-fd64c91cd06b"]').href="https://gitea.com/gitea/distea/releases/download/v0.0.5/distea_0.0.5_linux_x86_64.tar.gz"

It works.

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Nov 13, 2022
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.

@ghost
Copy link
Author

ghost commented Nov 13, 2022

There was one:

I see WIP.... from 2020

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 13, 2022

Yup, I do not understand the blocker of the problem at the moment and haven't looked into it carefully. So I just hold my reviews.


ps: according to the comments in #13750, if I understand correctly, the problem is that the same "tag" may point to different attachments, which cause conflicts.

@delvh
Copy link
Member

delvh commented Nov 13, 2022

Well, as far as I've seen the original problem was that only the first attachment with that name can be downloaded.
However, I think that is rather expected behavior as uploading multiple attachments with the same name will not only confuse the computer, but also the humans…
The WIP PR wanted to add logic to disallow uploading release attachments with the same name (by replacing it with the new content), so I assume the problem is currently that the logic to block double uploads does not exist.

@ghost
Copy link
Author

ghost commented Nov 13, 2022

What GitHub does is just show: "An attachment with that filename alreadly exists."

@delvh
Copy link
Member

delvh commented Nov 13, 2022

Yes, that's also my proposed behavior, at least for the first step.
Afterward, we can still examine if we want to auto-replace a duplicated attachment with a new one, which has both advantages and disadvantages.

@ghost

This comment was marked as resolved.

Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

sorry that does not solve the unerlying issue

it was done so intentionaly

@6543
Copy link
Member

6543 commented Nov 13, 2022

see #13750 - please fix the open TODOs first and I'll ack to this (didn't had time myself to do so...)

PS: you can append this into this pull

@ghost
Copy link
Author

ghost commented Nov 14, 2022

The current workflow is:

  1. Upload an attachment (no release-related context, just set release_id=0), and get a set of attachmentUUIDs
  2. Create Release: Query a set of uploaded attachments through GetAttachmentsByUUIDs, and edit the release_id for each attachment (if release_id>0, permission denied)

I'm afraid the current workflow is hard to change correctly. Maybe a simple policy should be in the upload stage to check if each attachment of a release has the same name.


When I upload a file on GitHub, the frontend sends a POST containing name, size, content_type, authentication_token, repository_id, release_id

The GitHub server can easily check whether the attachments under a release have the same name according to repository_id, release_id, name

@lunny
Copy link
Member

lunny commented Nov 14, 2022

The problem is when creating a new release, how should we check the same filename? I think maybe we have to check them in frontend whatever creating or updating a release and only do backend duplicated filenames check when updating a release.

@ghost ghost closed this Nov 19, 2022
@ghost ghost deleted the bugfix/10919 branch November 19, 2022 12:31
@lunny lunny removed this from the 1.19.0 milestone Feb 22, 2023
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Attachment links are not human readable again
5 participants