Multi-platform: support multiple files per version #201
No reviewers
Labels
No Label
Priority
Critical
Priority
High
Priority
Low
Priority
Normal
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Type
Breaking
Type
Documentation
Type
Enhancement
Type
Feature
Type
Report
Type
Security
Type
Suggestion
Type
Testing
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: infrastructure/extensions-website#201
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "multi-os"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
See #194
This PR adds support for uploading and downloading multiple
platform-specific files per Version, updates the API endpoint and adds
Version methods
get_download_list
andget_build_list
for templates.Version.file
property is removed:all code should expect multiple files and access data using
Version.files
Download urls are still managed by Version, but all platform-aware contexts
are expected to pass a platform value, which should be enough to select
the correct file, since
Version.files
are not allowed to overlap by platforms.@ -684,0 +679,4 @@
@property
def can_upload_more_files(self):
return len(self.get_available_platforms()) > 0
s/get_available_platforms/get_missing_platforms/
?@ -684,0 +706,4 @@
for p in to_delete:
self.platforms.remove(Platform.objects.get(slug=p))
def get_available_platforms(self):
or
s/get_available_platforms/get_remaining_platforms/
in the sense that these are the platforms which don't yet have files uploaded for them, if any platform-specific file were uploaded
@ -713,3 +758,3 @@
def download_name(self) -> str:
def _get_download_name(self, platform=None) -> str:
"""Return a file name for downloads."""
replace_char = f'{self}'.replace('.', '-')
The
filename
belowfilename = f'{self.extension.type_slug_singular}-{self.extension.slug}-v{self.version}.zip'
looks like a better name: doesn't rely on
self.__str__
.It also looks like these file names should be the same in both places.
@ -715,2 +760,3 @@
replace_char = f'{self}'.replace('.', '-')
return f'{utils.slugify(replace_char)}{self.file.suffix}'
if platform:
return f'{utils.slugify(replace_char)}.{platform}.zip'
to avoid making platform into file ext, maybe
-
?@ -82,0 +77,4 @@
continue
# TODO? return all matching files (when no self.platform is passed)?
matching_file = file
matching_version = v
needs another
break
here@ -194,3 +199,3 @@
'licenses': data.get('license'),
'permissions': data.get('permissions'),
'platforms': build_generated_platforms or data.get('platforms'),
'platforms': self.platforms(),
if keeping it a method is preferable, then
get_platforms()
might be cleaner,otherwise making it a
platforms
property would make sense.@ -82,0 +76,4 @@
if self.platform and (platforms and self.platform not in platforms):
continue
# TODO? return all matching files (when no self.platform is passed)?
matching_file = file
this looks shared with the logic in
public
view that selects which file to pick from storage, and should probable become a utility or method somewhere.@ -744,6 +799,54 @@ class Version(CreatedModifiedMixin, TrackChangesMixin, models.Model):
download_url += f'?{query_string}'
return download_url
Would it make sense to use file hash in the URL?
This would allow skipping the file-selecting logic in the
public
view altogether by using the hash to retrieve the file directly.Nice :) Some bugs I found during my initial testing
...
http://extensions.local:8111/add-ons/bioxelnodes/0.2.2/macos-arm64/download/add-on-bioxelnodes-v0.2.2-macos-arm64.zip?repository=%2Fapi%2Fv1%2Fextensions%2F&blender_version_min=4.2.0&platforms=windows-x64%2Cmacos-arm64
As a follow up PR we could find a way to support this for the version upload API as well.
I have a few remarks regarding UX, but honestly although this is an important feature, it is not something that will be used by a lot of authors. So I'm not proposing we change that. But to share my 2 cents:
I just saw that you did the:
Rename /api/v1/extensions/<extension_id>/versions/new/ → /api/v1/extensions/<extension_id>/versions/upload/
(as per the design).So maybe the API is working already, sorry about the noise.
UPD: I've removed the can_upload_more_files property and added support for subsequent uploads for the same version via NewVersionView, the paragraph below is outdated
Yes, as soon as a version exists, this construction should work (replacing form.instance with version where appropriate):
I didn't put much thought into a better entry point - just made it possible to test somehow, following the initial ticket description.
This is how API already works in this branch, and we can make the website behave in a similar way, but we should double-check if there are any scenarios when this might become confusing. Perhaps, adding a special message to the existing "New Version" upload form when
can_upload_more_files
is true would be sufficient to resolve this?I'll check if it may lead to any weird corner-cases from backend perspective, but overall sounds reasonable.
e31900fc80
tob922118d93
fixed, but I use the same
get_download_url
code for the build list - showing one platform for a platform-specific file with multiple platforms there might be weird, so this might be not the last issue around that parameter, will see how to improve thisoops, that was missed in one of later commits, fixed
I haven't decided yet on the download url change suggested by Anna in #201 (comment)
To elaborate on this:
We will keep the current urls for backward compatibility for some time, and can remove them once we see little to no traffic to them (e.g. somebody cached an API response and doesn't re-fetch the listing for whatever reason)
But with platform-specific downloads we have to introduce a new url pattern, just because the current url pattern doesn't allow to encode the choice between multiple files of the same Version.
This branch already adds a new url pattern in
I don't like that we have to deal with platform-specific and platform-agnostic files in different ways - by the time that we serve the download the platform details shouldn't matter, it is the task of the UI offering the download link to clarify those, the download endpoint itself ideally should not repeat this logic of picking the right file for a platform.
That's why moving to an url pattern that explicitly encodes a File object itself is appealing.
It could look like this:
/download/<file_hash>/<filename>?repository=...&blender_version_min=...&platforms=...
filename
ending with '.zip' as it exists today, to support drag&drop and to respond with a proper Content-Disposition header to use the same filename when saving to disk (devserver doesn't do this, but nginx in prod does)file_hash
for the actual file lookup on the backendAs long as we don't have a need to replace a file for a given version (or a given version+platform combination), this approach should work fine: a hash-based url will return 404 when the file gets replaced, but the current, more dynamic implementation will look up a new file instead.
We likely don't have a use case for this, and even if we had, we could demand that a listing containing an old hash gets re-fetched.
I wonder, when calling
/api/v1/extensions/
without specifying the platform, shouldn't we list each of the platform builds as individual entries? Either that or to make the platform item a requirement.Because right now we get a download link which is probably arbritary (the first uploaded platform?) while listing all the platforms.
I think this would make sense, i have a note about this:
I'll check - it's probably easy to add, although current behavior shouldn't be an issue for the official use case, since blender always passes a platform, and I don't know if we should make platform a required parameter at this point tbh
UPD: done
I've updated one of the comments above, the separate button to upload more files is removed, now "Upload New Version" should accept more files for the same version.
I've also removed the
can_upload_more_files
property, since we don't have a place to use it anymore. It's easy to bring it back it finds its place in UI.I know the UI is not supposed to be super polished right now, but be as it may. Right now only the "top" URL is draggable for me. The other one has no URL on it or something.
You can try it by dragging them to any Text Editor. One shows the correct full URL, while the other gives me nothing.
I believe this is still wrong:
archive_hash
andarchive_size
are different to each entry, they all share the samearchive_url
.oh, right - this is a bit fragile due to how the download urls are constructed, I think this is one more argument in favor of changing the implementation for download urls, outlined above
and it also makes sense to add a better test for this
UPD: this is now done
Alright, everything seems working.
If it wasn't for the regression on the Drag & Drop behaviour I would say this could merge as it is, while we fix the front-end it in main.
I even think we could merge as it is since the wonkness of the UI only happens when there are multi-platform packages, which won't be widely known until we promote it more.
WIP: Multi-platform: support multiple files per versionto Multi-platform: support multiple files per versionFront-end support has been added for new multi OS features, as outlined above.
For the details, see: #194 (comment)
A note about the API: having each extension entry duplicated by a number of version files in it instead of nesting version and files using the existing hierarchy of
Extension
->Version
->Files
seems very far from any standard REST API design.If I were just looking at the API for the first time, I'd expect the response to look more like this:
However, I understand that this is out of scope because this'd require a new version of the API schema.
LGTM
Technically speaking the V1 of the API schema is up for changing until Blender 4.2 is officially released. So it depends on how strong you are about this change.
Changing this would mean changing Blender as well, nothing impossible, but not something that should wait if we decide to change it.