Multi-platform: support multiple files per version #201

Merged
Oleg-Komarov merged 43 commits from multi-os into main 2024-07-09 16:27:46 +02:00
Owner

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 and get_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.

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` and `get_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.
Oleg-Komarov added 17 commits 2024-07-05 11:23:09 +02:00
Oleg-Komarov added 1 commit 2024-07-05 11:28:54 +02:00
Anna Sirota reviewed 2024-07-05 14:32:48 +02:00
@ -684,0 +679,4 @@
@property
def can_upload_more_files(self):
return len(self.get_available_platforms()) > 0
Owner

s/get_available_platforms/get_missing_platforms/?

`s/get_available_platforms/get_missing_platforms/`?
Oleg-Komarov marked this conversation as resolved
@ -684,0 +706,4 @@
for p in to_delete:
self.platforms.remove(Platform.objects.get(slug=p))
def get_available_platforms(self):
Owner

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

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
Oleg-Komarov marked this conversation as resolved
@ -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('.', '-')
Owner

The filename below
filename = 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.

The `filename` below `filename = 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.
Oleg-Komarov marked this conversation as resolved
@ -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'
Owner

to avoid making platform into file ext, maybe -?

to avoid making platform into file ext, maybe `-`?
Oleg-Komarov marked this conversation as resolved
@ -82,0 +77,4 @@
continue
# TODO? return all matching files (when no self.platform is passed)?
matching_file = file
matching_version = v
Owner

needs another break here

needs another `break` here
Oleg-Komarov marked this conversation as resolved
files/models.py Outdated
@ -194,3 +199,3 @@
'licenses': data.get('license'),
'permissions': data.get('permissions'),
'platforms': build_generated_platforms or data.get('platforms'),
'platforms': self.platforms(),
Owner

if keeping it a method is preferable, then get_platforms() might be cleaner,
otherwise making it a platforms property would make sense.

if keeping it a method is preferable, then `get_platforms()` might be cleaner, otherwise making it a `platforms` property would make sense.
Oleg-Komarov marked this conversation as resolved
Anna Sirota reviewed 2024-07-05 14:42:10 +02:00
@ -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
Owner

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.

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.
Oleg-Komarov marked this conversation as resolved
Anna Sirota reviewed 2024-07-05 14:56:44 +02:00
@ -744,6 +799,54 @@ class Version(CreatedModifiedMixin, TrackChangesMixin, models.Model):
download_url += f'?{query_string}'
return download_url
Owner

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.

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.
Oleg-Komarov marked this conversation as resolved
Oleg-Komarov added 4 commits 2024-07-05 15:55:25 +02:00
Oleg-Komarov added 1 commit 2024-07-05 17:40:11 +02:00

Nice :) Some bugs I found during my initial testing

...

  1. The download link it is including all platforms on its parameters:

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


  1. There seems to be a bug when showing the file sizes (see the empty parenthesis):
image
  1. I couldn't find a way to upload a new version until the extension is already sent for review (before that there is no link to Version History). Is it possible (from the backend data point of view) to upload new builds for the platform for the initial version?
Nice :) Some bugs I found during my initial testing ... 1. The download link it is including all platforms on its parameters: `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` --- 2. There seems to be a bug when showing the file sizes (see the empty parenthesis): <img width="510" alt="image" src="attachments/1b70cfd1-05cc-4524-b1e9-7e08e67269f7"> --- 3. I couldn't find a way to upload a new version until the extension is already sent for review (before that there is no link to Version History). Is it possible (from the backend data point of view) to upload new builds for the platform for the initial version?

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:

  • Right now I have to go to a lot of places to add the new version platform. I wouldn't mind if (in addition to the current method) I could just go to Upload a new Version and if the package I picked was from an existing version but new platform, it simply adds it (and return the corresponding version edit/update page).
  • Technically the same could apply for the upload API to keep things simple.
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: * Right now I have to go to a lot of places to add the new version platform. I wouldn't mind if (in addition to the current method) I could just go to Upload a new Version and if the package I picked was from an existing version but new platform, it simply adds it (and return the corresponding version edit/update page). * Technically the same could apply for the upload API to keep things simple.

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.

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.
Author
Owner

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

Is it possible (from the backend data point of view) to upload new builds for the platform for the initial version?

Yes, as soon as a version exists, this construction should work (replacing form.instance with version where appropriate):

{% if form.instance.can_upload_more_files %}
<a href="{{ form.instance.get_version_upload_url }}" class="btn btn-primary">
<i class="i-upload"></i>
{% trans 'Upload files for other platforms' %}
</a>
{% endif %}

I didn't put much thought into a better entry point - just made it possible to test somehow, following the initial ticket description.

I wouldn't mind if (in addition to the current method) I could just go to Upload a new Version and if the package I picked was from an existing version but new platform, it simply adds it (and return the corresponding version edit/update page)

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.

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 > Is it possible (from the backend data point of view) to upload new builds for the platform for the initial version? Yes, as soon as a version exists, this construction should work (replacing form.instance with version where appropriate): https://projects.blender.org/infrastructure/extensions-website/src/commit/8af96c48c0febf677ec1ee0ff7af2c52516485f7/extensions/templates/extensions/new_version_finalise.html#L51-L56 I didn't put much thought into a better entry point - just made it possible to test somehow, following the initial ticket description. > I wouldn't mind if (in addition to the current method) I could just go to Upload a new Version and if the package I picked was from an existing version but new platform, it simply adds it (and return the corresponding version edit/update page) 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.
Oleg-Komarov force-pushed multi-os from e31900fc80 to b922118d93 2024-07-05 18:37:32 +02:00 Compare
Oleg-Komarov added 2 commits 2024-07-05 18:54:03 +02:00
Author
Owner

The download link it is including all platforms on its parameters

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 this

file sizes (see the empty parenthesis)

oops, that was missed in one of later commits, fixed

> The download link it is including all platforms on its parameters 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 this > file sizes (see the empty parenthesis) oops, that was missed in one of later commits, fixed
Author
Owner

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

extensions/urls.py Lines 94 to 98 in 050ba772d7
path(
'<slug:slug>/<version>/<platform>/download/<filename>',
public.extension_version_platform_download,
name='version-platform-download',
),

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=...

  • keeping a human-readable 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)
  • using a unique file_hash for the actual file lookup on the backend
  • starting with "/download" to easily separate download traffic in dashboards and in our http stack (CloudFlare->nginx->django)

As 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 haven't decided yet on the download url change suggested by Anna in https://projects.blender.org/infrastructure/extensions-website/pulls/201#issuecomment-1232667 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 https://projects.blender.org/infrastructure/extensions-website/src/commit/050ba772d736109057c2fa66759fd12161874890/extensions/urls.py#L94-L98 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=...` - keeping a human-readable `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) - using a unique `file_hash` for the actual file lookup on the backend - starting with "/download" to easily separate download traffic in dashboards and in our http stack (CloudFlare->nginx->django) As 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.
Oleg-Komarov added 1 commit 2024-07-08 12:23:28 +02:00

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 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.
Oleg-Komarov added 1 commit 2024-07-08 13:41:15 +02:00
Oleg-Komarov added 1 commit 2024-07-08 13:43:59 +02:00
remove a separate view for uploading a file for an existing version
Author
Owner

shouldn't we list each of the platform builds as individual entries

I think this would make sense, i have a note about this:

# TODO? return all matching files (when no self.platform is passed)?

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

> shouldn't we list each of the platform builds as individual entries I think this would make sense, i have a note about this: https://projects.blender.org/infrastructure/extensions-website/src/commit/ab6e7eedb72602f6152d0ce73b8b547e4fc40eb3/extensions/views/api.py#L80 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
Author
Owner

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'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.
Oleg-Komarov added 1 commit 2024-07-08 14:17:18 +02:00
Oleg-Komarov added 1 commit 2024-07-08 14:44:27 +02:00
Oleg-Komarov added 1 commit 2024-07-08 16:06:29 +02:00
Oleg-Komarov added 1 commit 2024-07-08 16:15:23 +02:00

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.

image

You can try it by dragging them to any Text Editor. One shows the correct full URL, while the other gives me nothing.

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. <img width="432" alt="image" src="attachments/276ca20c-850e-4d65-bca2-4163d84486af"> You can try it by dragging them to any Text Editor. One shows the correct full URL, while the other gives me nothing.

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 believe this is still wrong:

>> 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 believe this is still wrong: * While `archive_hash` and `archive_size` are different to each entry, they all share the same `archive_url`. * The platform suffix is not included on the download URL at the moment. * Example: "archive_url": "http://extensions.local:8111/add-ons/molecularnodes/4.2.3/download/add-on-molecularnodes-v4.2.3.zip",
Author
Owner

While archive_hash and archive_size are different to each entry, they all share the same archive_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

> While archive_hash and archive_size are different to each entry, they all share the same archive_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
Oleg-Komarov added 1 commit 2024-07-08 17:15:33 +02:00
Oleg-Komarov added 1 commit 2024-07-08 18:41:03 +02:00

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.

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.
Márton Lente added 1 commit 2024-07-08 19:28:51 +02:00
Márton Lente added 1 commit 2024-07-09 12:15:20 +02:00
Márton Lente added 1 commit 2024-07-09 12:37:04 +02:00
Improve how install action item buttons are display when extension variants
are available for multiple platforms.
Part of #194, #201
Márton Lente added 1 commit 2024-07-09 13:13:35 +02:00
Add drag and drop install buttons support for extensions with multiple
explicit platform versions.
Part of #194, #201
Oleg-Komarov changed title from WIP: Multi-platform: support multiple files per version to Multi-platform: support multiple files per version 2024-07-09 13:53:15 +02:00
Oleg-Komarov added 1 commit 2024-07-09 13:53:27 +02:00

Front-end support has been added for new multi OS features, as outlined above.

For the details, see: #194 (comment)

Front-end support has been added for new multi OS features, as outlined above. For the details, see: https://projects.blender.org/infrastructure/extensions-website/issues/194#issuecomment-1235612
Oleg-Komarov added 2 commits 2024-07-09 15:17:14 +02:00
Oleg-Komarov added 1 commit 2024-07-09 15:33:29 +02:00
Owner

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:

{
	"version": "v2",
	"data": [
        "id": "some_extension",
        "name": "Some Extension",
		"license": ["GPLv3"],
		"maintainer": "Josh Doe",
		"tags": [],
		"website": "https://example.com",
        "permissions": [],
        "schema_version": 1,
        "tagline": "some tag line",
        "type": "add-on",
        "blender_version_max": null,
        "blender_version_min": "4.2.3",
        "version": "1.0.0",
        "versions": [
        	{
            	"version": "1.0.0",
                "files": [
                	{
                    	"platforms": [...],
                        "archive_url": "https://extensions.blender.org/download/foobar.zip",
                        "archive_size": 1234,
                        "archive_hash": "hash"
                    },
                    {
                    	"platforms": [...],
                        "archive_url": "https://extensions.blender.org/download/barfoo.zip",
                        "archive_size": 4321,
                        "archive_hash": "anotherhash"
                    }
                ],
                "release_notes": "..."
            }
        ]
    ]
}

However, I understand that this is out of scope because this'd require a new version of the API schema.

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: ```json { "version": "v2", "data": [ "id": "some_extension", "name": "Some Extension", "license": ["GPLv3"], "maintainer": "Josh Doe", "tags": [], "website": "https://example.com", "permissions": [], "schema_version": 1, "tagline": "some tag line", "type": "add-on", "blender_version_max": null, "blender_version_min": "4.2.3", "version": "1.0.0", "versions": [ { "version": "1.0.0", "files": [ { "platforms": [...], "archive_url": "https://extensions.blender.org/download/foobar.zip", "archive_size": 1234, "archive_hash": "hash" }, { "platforms": [...], "archive_url": "https://extensions.blender.org/download/barfoo.zip", "archive_size": 4321, "archive_hash": "anotherhash" } ], "release_notes": "..." } ] ] } ``` However, I understand that this is out of scope because this'd require a new version of the API schema.
Anna Sirota approved these changes 2024-07-09 16:09:25 +02:00
Anna Sirota left a comment
Owner

LGTM

LGTM
Oleg-Komarov merged commit dd82ca1156 into main 2024-07-09 16:27:46 +02:00
Oleg-Komarov deleted branch multi-os 2024-07-09 16:27:47 +02:00

However, I understand that this is out of scope because this'd require a new version of the API schema.

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.

> However, I understand that this is out of scope because this'd require a new version of the API schema. 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.
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: infrastructure/extensions-website#201
No description provided.