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.

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

feat(get): file name is computed from the v2/resources/(resource_id) URL value #322

Open
atyrode opened this issue Oct 8, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@atyrode
Copy link

atyrode commented Oct 8, 2023

Feature request

Hello!

I suggest that the downloaded file name for a given resource is not ID.jar but instead computed from the ['file'] -> ['url'] key of the SPIGET API at the v2/resources/ + ID endpoint.

By extracting the filename between resource/ and /download in the value of the URL key, and sanitizing it (replace every non [a-zA-Z0-9] character by _), this would transform ID.jar as 'example_name_ID.jar'.

For resource ID=81
81.jar becomes libs_disguise_free_81.jar


This change would have two benefits:

  1. Being able to identify what the file is representing right away in the filesystem
  2. The file remains maintainable programatically because the resource ID will always be the last part of the file name

I tried to see if I could do this msyelf but Java is not my expertise at all. However, if you can pinpoint where this change needs to be made, I'll gladly do a PR once I work this out; if you agree with the idea.

Thanks! Keep up the great work.

@itzg
Copy link
Owner

itzg commented Oct 8, 2023

Actually, spiget support isn't yet converted over to mc-image-helper

https://github.com/itzg/docker-minecraft-server/blob/master/scripts/start-spiget

That's why the file naming algorithm is overly simplistic so far.

When it is converted over the proper file naming can be used like the other retrieval logic here.

@itzg itzg added the enhancement New feature or request label Oct 8, 2023
@atyrode
Copy link
Author

atyrode commented Oct 8, 2023

I see, my bad for the overlook. I was watching the debug log and assumed the call to mc get meant it was doing the parsing, now I think I understand that it's being used to get the json path and start-spiget does the rest.

Would it make sense to submit a PR that does the suggested enhancement on the dockerfile side or would this be futile since a proper implementation is planned here?

@itzg
Copy link
Owner

itzg commented Oct 8, 2023

Good question. Yeah, I'd prefer to put the effort into porting it fully to mc-image-helper.

I'll try to take a quick go at that today.

@itzg
Copy link
Owner

itzg commented Oct 8, 2023

...ah, I remember now: I don't like Spiget's API. It is annoying that

https://spiget.org/documentation/#!/resources/get_resources_resource_download

redirects and downloads from Spiget's CDN whereas

https://spiget.org/documentation/#!/resources/get_resources_resource_versions_version_download

acts completely different and just redirects to spigotmc, which in turn responds with a 403 forbidden since they don't want automated downloads supported.

I would prefer a cleaner/robust API usage of

  1. check lastest resource metadata
  2. compare version in metadata with the one we already downloaded
  3. if not the same, download that specific version

Instead it's simply a blind download from the latest of that resource ID. To make matter worse:

  1. the directed URL does not contain the filename identifier, such as https://cdn.spiget.org/file/spiget-resources/28103.jar
  2. the response headers of the CDN redirect do not include a Content-Disposition which is where CDNs normally convey the resolved filename

Your suggestion of deriving from the file->url of the resource metadata does seem to be about the only solution, but that feels like it might be brittle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: To do
Development

No branches or pull requests

2 participants