Change `mount_series` endpoint to accept and move existing series #1225

owi92 · 2024-08-12T09:13:34Z

This is necessary for an upcoming admin UI feature which will allow admins to change the path of series pages with no other blocks (part of opencast/opencast-admin-interface#311).

This shouldn't break any existing behaviour so merging this now should be ok, but it doesn't need to be in the upcoming 2.11 release.
Related Opencast and admin UI PRs: opencast/opencast#6091, opencast/opencast-admin-interface#878.

backend/src/api/mutation.rs

LukasKalbertodt

We just discussed this in chat, I'm just writing it down here for protocol.

I was still not too happy with very overloaded move API. So the idea now to wait for #1179 and then add three more API endpoints to end up with these:

createRealmLineage(realms: { name: string, pathSegment: string })
prefigureSeries(series: NewSeries) (not necessarily final name)
addSeriesMountPoint(series_oc_id: string, realm_path: string)
removeSeriesMountPoint(realm_path: string) (maybe also pass series id to check stuff?)

The admin UI is then changed to send a GraphQL request with multiple mutations in them. GraphQL guarantees us, that these are executed in order. And Tobira uses one DB transaction per GraphQL request, so the whole operation is atomic. Nice!

We do need to keep the old mountSeries endpoint around for some while, until the last admin UI using it is not supported anymore, but that's not a big problem.

Note though that the individual endpoints (especially the MountPoint ones) are still... no atomic perfectly defined operation. It still represents a "high level task" (like the current mountSeries) instead of manipulating Tobira's DB objects directly.

backend/src/api/mutation.rs

Well, the auto updater already did update it, breaking it. So this makes Grafana work again. Well, it already does, as I ran these scripts locally already.

This fixes an issue where the popup menu in paella wasn't sticking to its trigger button correctly.

Well, the auto updater already did update it, breaking it. So this makes Grafana work again. Well, it already does, as I ran these scripts locally already.

This fixes an issue where the popup menu in paella wasn't sticking to its trigger button correctly. Closes elan-ev#1231

Bumps [body-parser](https://github.com/expressjs/body-parser) and [express](https://github.com/expressjs/express). These dependencies needed to be updated together. Updates `body-parser` from 1.20.2 to 1.20.3 - [Release notes](https://github.com/expressjs/body-parser/releases) - [Changelog](https://github.com/expressjs/body-parser/blob/master/HISTORY.md) - [Commits](expressjs/body-parser@1.20.2...1.20.3) Updates `express` from 4.19.2 to 4.21.0 - [Release notes](https://github.com/expressjs/express/releases) - [Changelog](https://github.com/expressjs/express/blob/4.21.0/History.md) - [Commits](expressjs/express@4.19.2...4.21.0) --- updated-dependencies: - dependency-name: body-parser dependency-type: indirect - dependency-name: express dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>

This is necessary for an upcoming admin UI feature which will allow admins to change the path of series pages with no other blocks.

LukasKalbertodt

I sadly found a few things that need fixing. But I think they should all be straight forward? The general direction is good.

LukasKalbertodt · 2024-10-01T10:29:12Z

backend/src/api/mutation.rs

@@ -275,6 +276,94 @@ impl Mutation {
        Ok(CreateRealmLineageOutcome { num_created })
    }

+    async fn prepare_series(series: NewSeries, context: &Context) -> ApiResult<PreparedSeries> {


Not sure prepare is the best term to use here. I "suggested" prefigure, which I was also not happy with (mainly because I never heard it before). But prepare to me does not sound like "informing Tobira of its existance", but rather like Tobira already knows about the series. So what's happening here is that Opencast informs Tobira in advance about a series that exists, and that Tobira will learn all details of via the harvest API. I already mentioned the German word "vorankündigen" or just "ankündigen" which i think fits nicely. Translation for this are: announce, notify, herald, pronounce, advertise, bode, presage, prefigure, harbinger. Maybe "herald"? Or just announce_series? I had my reservations regarding "announce" but maybe it's actually fine?

LukasKalbertodt · 2024-10-01T10:31:58Z

backend/src/api/mutation.rs

+
+        let series = Series::create(series, context).await?;
+
+        Ok(PreparedSeries { id: series.id() })


Why not simply return Series from this?

LukasKalbertodt · 2024-10-01T10:35:10Z

backend/src/api/mutation.rs

+        if context.auth != AuthContext::TrustedExternal {
+            return Err(not_authorized!("only trusted external applications can use this mutation"));
+        }


These three lines are repeated a bit. How about adding a method to auth named required_trusted_external() -> Result<()>? Then you can just call context.auth.require_trusted_external()?; everywhere

LukasKalbertodt · 2024-10-01T10:36:53Z

backend/src/api/mutation.rs

@@ -275,6 +276,94 @@ impl Mutation {
        Ok(CreateRealmLineageOutcome { num_created })
    }

+    async fn prepare_series(series: NewSeries, context: &Context) -> ApiResult<PreparedSeries> {


All new API endpoints should have some docs explaining what they do.

LukasKalbertodt · 2024-10-01T10:40:49Z

backend/src/api/mutation.rs

+
+        let target_realm = Realm::load_by_path(target_path, context)
+            .await?
+            .ok_or_else(|| invalid_input!("`targetRealmPath` does not refer to a valid realm"))?;


Suggested change

.ok_or_else(|| invalid_input!("`targetRealmPath` does not refer to a valid realm"))?;

.ok_or_else(|| invalid_input!("`targetPath` does not refer to a valid realm"))?;

LukasKalbertodt · 2024-10-01T10:51:59Z

backend/src/api/mutation.rs

    /// Atomically mount a series into an (empty) realm.
    /// Creates all the necessary realms on the path to the target
    /// and adds a block with the given series at the leaf.


mount_series should probably be changed to just call the three new functions (createRealmLineage, announceSeries, addSeriesMountPoint) to avoid duplicated code. Yes, this means some things are needlessly loaded from the DB, but I really don't think that's a problem. And it means that the actual impl needs to live in an actual method somewhere in another file (model::Realm::create_lineage, model::Series::announce, ...) but that's not a bad idea anyway to reduce the size of this mutation.rs file, which cannot be split up.

LukasKalbertodt · 2024-10-01T10:59:16Z

backend/src/api/mutation.rs

+        let blocks = BlockValue::load_for_realm(old_realm.key, context).await?;
+
+        if blocks.len() > 1 {
+            return Err(invalid_input!("series can only be moved if its current realm has no other blocks"));


Error message needs adjusting (its not a "move" API anymore)

LukasKalbertodt · 2024-10-01T10:59:28Z

backend/src/api/mutation.rs

+
+        let old_realm = Realm::load_by_path(path, context)
+            .await?
+            .ok_or_else(|| invalid_input!("`currentRealmPath` does not refer to a valid realm"))?;


Suggested change

.ok_or_else(|| invalid_input!("`currentRealmPath` does not refer to a valid realm"))?;

.ok_or_else(|| invalid_input!("`path` does not refer to a valid realm"))?;

LukasKalbertodt · 2024-10-01T11:04:12Z

backend/src/api/mutation.rs

+            // The realm has no children, so it can be removed.
+            removed_realm = Some(Realm::remove(old_realm.id(), context).await?);
+        } else {
+            // At this point we can be certain that there is only one block, which is the series block.


Actually no, these checks are not happening in this method, at this point we only know that there is 0 or 1 blocks in this realm and that the realm has some children. If the block has 0 blocks, the blocks[0] expression below panics. And the check whether the block is even mentioning the series is probably also useful? Also, the realm doesn't need to be renamed if it doesn't derive the name from the block.

LukasKalbertodt · 2024-10-01T11:05:10Z

backend/src/api/model/realm/mutations.rs

+#[derive(juniper::GraphQLObject)]
+#[graphql(Context = Context)]
+pub(crate) struct RemoveMountedSeriesOutcome {
+    pub removed_realm: Option<RemovedRealm>,
+    pub removed_block: Option<RemovedBlock>,
+}


It seems like there is always exactly one of these fields set to Some. Rather sounds like an enum (Rust) / union (graphql) to me?

github-actions bot temporarily deployed to test-deployment-pr1225 August 12, 2024 09:19 Destroyed

This was referenced Aug 12, 2024

Add additional features for Tobira integration opencast/opencast-admin-interface#878

Draft

Add more admin-UI Tobira endpoints opencast/opencast#6091

Draft

owi92 force-pushed the mount-and-move-series branch from a123e17 to 6a2d43e Compare August 12, 2024 16:57

github-actions bot temporarily deployed to test-deployment-pr1225 August 12, 2024 16:59 Destroyed

owi92 force-pushed the mount-and-move-series branch from fe39123 to a9556b3 Compare August 13, 2024 09:20

github-actions bot temporarily deployed to test-deployment-pr1225 August 13, 2024 09:26 Destroyed

owi92 force-pushed the mount-and-move-series branch from a9556b3 to 156f1ab Compare August 13, 2024 09:27

github-actions bot temporarily deployed to test-deployment-pr1225 August 13, 2024 09:30 Destroyed

owi92 force-pushed the mount-and-move-series branch from 156f1ab to dfc0d4d Compare August 13, 2024 11:28

github-actions bot temporarily deployed to test-deployment-pr1225 August 13, 2024 11:30 Destroyed

LukasKalbertodt requested changes Aug 13, 2024

View reviewed changes

owi92 force-pushed the mount-and-move-series branch 2 times, most recently from 6a2d43e to fb675ac Compare August 13, 2024 12:45

github-actions bot temporarily deployed to test-deployment-pr1225 August 13, 2024 12:48 Destroyed

github-actions bot temporarily deployed to test-deployment-pr1225 August 13, 2024 12:52 Destroyed

owi92 force-pushed the mount-and-move-series branch from 0f37033 to fb675ac Compare August 13, 2024 12:53

LukasKalbertodt force-pushed the master branch from 6fa85fe to bc1902d Compare August 13, 2024 12:56

owi92 force-pushed the mount-and-move-series branch from fb675ac to 3a5d83a Compare August 13, 2024 12:57

This comment was marked as resolved.

Sign in to view

github-actions bot added the status:conflicts This PR has conflicts that need to be resolved label Aug 13, 2024

owi92 force-pushed the mount-and-move-series branch from 3a5d83a to a06ba4e Compare August 13, 2024 13:12

github-actions bot removed the status:conflicts This PR has conflicts that need to be resolved label Aug 13, 2024

github-actions bot temporarily deployed to test-deployment-pr1225 August 13, 2024 13:15 Destroyed

owi92 force-pushed the mount-and-move-series branch from a06ba4e to 5be04f0 Compare August 13, 2024 13:26

This comment has been minimized.

Sign in to view

github-actions bot temporarily deployed to test-deployment-pr1225 August 13, 2024 13:30 Destroyed

owi92 force-pushed the mount-and-move-series branch from 5be04f0 to adc795b Compare August 13, 2024 15:02

github-actions bot temporarily deployed to test-deployment-pr1225 August 13, 2024 15:04 Destroyed

LukasKalbertodt requested changes Sep 2, 2024

View reviewed changes

backend/src/api/mutation.rs Outdated Show resolved Hide resolved

owi92 added the changelog:dev Changes primarily for developers label Sep 9, 2024

Update Grafana to 11.2 on test deployment

2db6210

Well, the auto updater already did update it, breaking it. So this makes Grafana work again. Well, it already does, as I ran these scripts locally already.

github-actions bot added the status:conflicts This PR has conflicts that need to be resolved label Sep 12, 2024

This comment was marked as resolved.

Sign in to view

owi92 and others added 5 commits September 13, 2024 11:58

Update paella-core to 1.49.6

a53d45f

This fixes an issue where the popup menu in paella wasn't sticking to its trigger button correctly.

Update Grafana to 11.2 on test deployment (elan-ev#1241)

5889f57

Well, the auto updater already did update it, breaking it. So this makes Grafana work again. Well, it already does, as I ran these scripts locally already.

Update paella-core to 1.49.6 (elan-ev#1245)

1d14142

This fixes an issue where the popup menu in paella wasn't sticking to its trigger button correctly. Closes elan-ev#1231

owi92 force-pushed the mount-and-move-series branch from adc795b to 3f63c30 Compare September 13, 2024 13:06

github-actions bot removed the status:conflicts This PR has conflicts that need to be resolved label Sep 13, 2024

github-actions bot temporarily deployed to test-deployment-pr1225 September 13, 2024 13:12 Destroyed

owi92 marked this pull request as draft September 16, 2024 08:33

Add endpoints to move existing series

4f86349

This is necessary for an upcoming admin UI feature which will allow admins to change the path of series pages with no other blocks.

owi92 force-pushed the mount-and-move-series branch from 3f63c30 to 4f86349 Compare September 30, 2024 10:33

github-actions bot deployed to test-deployment-pr1225 September 30, 2024 10:41 View deployment

owi92 marked this pull request as ready for review September 30, 2024 10:52

LukasKalbertodt requested changes Oct 1, 2024

View reviewed changes

LukasKalbertodt force-pushed the master branch from df91940 to ac46ed4 Compare October 2, 2024 10:27

owi92 changed the base branch from main to next October 2, 2024 14:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change `mount_series` endpoint to accept and move existing series #1225

Change `mount_series` endpoint to accept and move existing series #1225

owi92 commented Aug 12, 2024 •

edited

Loading

This comment was marked as resolved.

This comment has been minimized.

LukasKalbertodt left a comment

This comment was marked as resolved.

LukasKalbertodt left a comment

LukasKalbertodt Oct 1, 2024

LukasKalbertodt Oct 1, 2024

LukasKalbertodt Oct 1, 2024

LukasKalbertodt Oct 1, 2024

LukasKalbertodt Oct 1, 2024

LukasKalbertodt Oct 1, 2024

LukasKalbertodt Oct 1, 2024

LukasKalbertodt Oct 1, 2024

LukasKalbertodt Oct 1, 2024

LukasKalbertodt Oct 1, 2024


		let series = Series::create(series, context).await?;

		Ok(PreparedSeries { id: series.id() })

	.ok_or_else(\|\| invalid_input!("`targetRealmPath` does not refer to a valid realm"))?;
	.ok_or_else(\|\| invalid_input!("`targetPath` does not refer to a valid realm"))?;

	.ok_or_else(\|\| invalid_input!("`currentRealmPath` does not refer to a valid realm"))?;
	.ok_or_else(\|\| invalid_input!("`path` does not refer to a valid realm"))?;

Change mount_series endpoint to accept and move existing series #1225

Are you sure you want to change the base?

Change mount_series endpoint to accept and move existing series #1225

Conversation

owi92 commented Aug 12, 2024 • edited Loading

This comment was marked as resolved.

This comment has been minimized.

LukasKalbertodt left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

LukasKalbertodt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Change `mount_series` endpoint to accept and move existing series #1225

Change `mount_series` endpoint to accept and move existing series #1225

owi92 commented Aug 12, 2024 •

edited

Loading