Shared modpacks routing by thesuzerain · Pull Request #815 · modrinth/labrinth

thesuzerain · 2023-12-29T12:26:08Z

NOTE: DO NOT MERGE until changes noted in the migrations file are reverted.

Fixes MOD-576

Notable deviations from spec:

Link generated is not immediately returned to the user, but attached to another route (so can be called multiple times for multiple share links to be generated as they run out of uses/expire)
Link generated is an API link to the join-profile route (with the appropriate url identifier). This will be changed to be a modrinth:// link as we move from testing this with cloudflare to having the theseus implementation.

… shared-modpacks

Geometrically

make the routes internal also

Geometrically · 2024-01-12T21:52:28Z

+    updated timestamptz NOT NULL DEFAULT CURRENT_TIMESTAMP,
+    created timestamptz NOT NULL DEFAULT CURRENT_TIMESTAMP,
+
+    maximum_users integer NOT NULL,


what's this field for?

Geometrically · 2024-01-12T21:53:37Z

+
+CREATE TABLE shared_profiles_links (
+    id bigint PRIMARY KEY, -- id of the shared profile link (ignored in labrinth, for db use only)
+    link varchar(48) NOT NULL UNIQUE, -- extension of the url that identifies this (ie profiles/afgxxczsewq)


Why not just make it based on the ID? Like discord's invites, it could be like: https://modrinth/invite/AAdwaDWD

Geometrically · 2024-01-12T21:58:15Z

+    maximum_users integer NOT NULL,
+
+    loader_id int NOT NULL REFERENCES loaders(id), 
+    loader_version varchar(255) NOT NULL,


I think it would be better for these to not be columns and rather their own column with strings as the id. So it would be like a jsonb metadata column like `{ "game_version": "1.19.2", "loader_version": "47.1.2" } (same with game versions) as games and loader versions can be specific to Minecraft

Geometrically · 2024-01-12T22:05:12Z

+#[derive(Clone, Debug, Serialize, Deserialize)]
+pub enum ClientProfileGame {
+    Minecraft {
+        game_id: GameId,
+        game_name: String,
+        game_version_id: LoaderFieldEnumValueId,
+        game_version: String,
+    },
+    Unknown {
+        game_id: GameId,
+        game_name: String,
+    },
+}


Do we need an enum here? Since we already have games dynamically in the database. Couldn't we just add a jsonb metadata field with game metadata (and then remove game_id and game_name fields

Alternatively we could build this in with the loader fields system... but I don't think it's the best idea bc stuff like loader versions aren't on labrinth

Geometrically · 2024-01-19T17:33:25Z

+    metadata jsonb NOT NULL DEFAULT '{}'::jsonb,
+
+    game_id int NOT NULL REFERENCES games(id),
+    game_version_id int NULL REFERENCES loader_field_enum_values(id) -- Minecraft java


why does this exist if the game version is also in the metadata?

Geometrically · 2024-01-19T17:35:24Z

+#[derive(Serialize, Deserialize, Clone)]
+pub struct ClientProfileShareLink {
+    pub id: ClientProfileLinkId, // The url identifier, encoded as base62
+    pub url: String,             // Includes the url identifier, intentionally redundant
+    pub profile_id: ClientProfileId,
+    pub created: DateTime<Utc>,
+    pub expires: DateTime<Utc>,
+}
+
+impl From<database::models::client_profile_item::ClientProfileLink> for ClientProfileShareLink {
+    fn from(link: database::models::client_profile_item::ClientProfileLink) -> Self {
+        // Generate URL for easy access
+        let profile_id: ClientProfileId = link.shared_profile_id.into();
+        let link_id: ClientProfileLinkId = link.id.into();
+        let url = format!(
+            "{}/v3/client/profile/{}/accept/{}",
+            dotenvy::var("SELF_ADDR").unwrap(),
+            profile_id,
+            link_id
+        );
+
+        Self {
+            id: link_id,
+            url,
+            profile_id,
+            created: link.created,
+            expires: link.expires,
+        }
+    }
+}


The URL shouldn't be returned / parsed here. The frontend will generate it based on the link ID

Geometrically · 2024-01-19T17:36:44Z

+    /// Modrinth-associated versions
+    pub versions: Vec<VersionId>,
+    /// Overrides for this profile- only install paths are given,
+    /// hashes are looked up in the CDN by the client
+    pub override_install_paths: Vec<PathBuf>,


What does this mean? IMO we shouldn't have the versions or any install info be returned here. It should be a separate route that is only called when the pack is being installed. And it can only be accessed once a user has accepted the invite.

Im not sure I agree- I think it could be useful to be able to see the stuff youre going to install on your computer before accepting, or what's 'in/on' the shared modpack, like in the frontend as a snippet of what you're getting in the pack.

All this displays is what the versions are and the 'names' of the overrides. do you think its not worth returning this?

Geometrically · 2024-01-19T17:38:55Z

+// Delete a client profile
+pub async fn profile_delete(
+    req: HttpRequest,
+    info: web::Path<(String,)>,
+    pool: web::Data<PgPool>,
+    redis: web::Data<RedisPool>,
+    session_queue: web::Data<AuthQueue>,
+) -> Result<HttpResponse, ApiError> {


This should delete override files from the CDN as long as they are unused by other shared packs

Geometrically · 2024-01-19T17:44:36Z

+    version_id bigint NULL REFERENCES versions(id), -- for versions
+
+    -- for cdn links to files we host directly
+    file_hash varchar(255) NULL,


We should restructure this. Override files should be a separate table which this table refers too IMO. And then it should refer to this here (we could just re-use the files table TBH).

Like other API objects, this would allow us to have SHA1 and SHA512 hashes. Shared modpacks will also share files a ton (ex: if someone has the same exact config file between packs, we shouldn't haave to host it twice or have duplicate database entries)

I think just using the files table would probably work well enough. It would also allow it so if someone did ever upload their mod to Modrinth, we could "re-link" it to the project dynamically and change it from an override to a Modrinth version ID. If that makes sense

Geometrically · 2024-01-19T17:47:26Z

+                        "{id}/override",
+                        web::delete().to(client_profile_remove_overrides),
+                    )
+                    .route("{id}/share", web::get().to(profile_share))


IMO this route is kind of pointless. We should just return share links in the main profile body. When would you get a profile and not get its share links?

Geometrically · 2024-01-19T17:48:25Z

+                    .route(
+                        "{id}/share/{url_identifier}",
+                        web::get().to(profile_link_get),
+                    )
+                    .route(
+                        "{id}/accept/{url_identifier}",
+                        web::post().to(accept_share_link),
+                    )


These routes are structured wrong. A share link would be like https://modrinth.com/share/aaJJJwwa. You won't know the profile ID. There should be a route to just get a profile from share link and accept a share link

thesuzerain added 9 commits December 27, 2023 15:38

initial commit

3774e59

working tests

af77b1d

fmt clippy prepare

72bb871

removed printlns

6eb7801

fixes

79ffdc1

switched to make sharing a one time invite and repeated downloads

df9e87c

profile deletion, tests

97a9afb

sha and upload limit

abce9c1

todo

65d8a0a

thesuzerain mentioned this pull request Jan 3, 2024

Adds shared profile CDN check modrinth/sisyphus#8

Open

simplifies- only uses user token

2739d62

thesuzerain marked this pull request as ready for review January 3, 2024 02:01

thesuzerain added 5 commits January 2, 2024 18:02

Merge branch 'master' into shared-modpacks

3a70025

Merge branch 'master' of http://www.github.com/modrinth/labrinth into…

29ae087

… shared-modpacks

uses enums

51ad5dd

removed println

961af8a

Merge branch 'master' of http://www.github.com/modrinth/labrinth into…

56736f0

… shared-modpacks

Geometrically suggested changes Jan 12, 2024

View reviewed changes

thesuzerain added 4 commits January 12, 2024 14:14

merge fixes

eadae95

temporary commit

5caf683

revs

34517de

revs

584a20a

Geometrically suggested changes Jan 19, 2024

View reviewed changes

thesuzerain and others added 7 commits January 19, 2024 12:42

moving computers

0477209

reorganized routes

3d9e118

revised routes

e07b4e1

partial conversion to file system

fd25991

Fixed test

5f76c91

profiles system now uses 'files'

64f977e

adds user route

388e81a

thesuzerain marked this pull request as draft January 30, 2024 01:21

thesuzerain added 3 commits January 29, 2024 17:29

temporary testing reversion

4a8bcbf

adds hashes to return

7e3199d

Merge branch 'master' into shared-modpacks

c0eb1fc

Uh oh!

Conversation

thesuzerain commented Dec 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Geometrically left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thesuzerain Jan 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

thesuzerain commented Dec 29, 2023 •

edited

Loading

thesuzerain Jan 19, 2024 •

edited

Loading