Add organization deletion route #4298

dantb · 2023-09-26T07:39:02Z

Key implementation points:

Added prune parameter to DELETE /orgs/{label} defaulting to false and requiring a new organizations/delete permission. Previously only deprecation was allowed for a specific rev. Not sure how great it is to ignore rev even if specified, maybe slightly confusing for the client? If the org is empty, return 409 Conflict.
OrganizationDeleter has all the logic. If the org is non-empty, we do these steps
1. Drop the org partitions for scoped tables
2. Delete the org records from the global tables
3. Delete the ACL records from the global tables

I used this ticket to try to understand some of the patterns in the codebase so I have a few questions / comments, some specific to this and some more general:

For the ACLs I manually wrote the deletion queries rather than reusing Acls, is this ok? The main reason was so it could all be in one transaction, since this feels like a bad thing to get stuck in a half-way state. It would also add a bigger dependency.
What places do I need to update with the new permission? Is it in infra somewhere?
Spent far too long trying to make the integration tests sane 😅 I wasn't sure the best approach and ended up using the old school arrange / act / assert. Preferably the tests should be independent so the tables get truncated before each. I don't really like the dodgy Monix type signatures though... Guess it comes down to trying to wrangle errors into a single type - I just didn't bother and opted for Any grimness. Is there a better way?
The unit tests do a lot of global state mutating and depend on each other a lot (as we've discussed before). It's a bit like a mishmash of pure FP and side effects which makes it quite difficult to reason about. I just threw more crap on the pile for this change. It would be nice to try to improve them at some point though - are there any plans for that? I could imagine tests like these being quite nice using CE, http4s, munit and scalacheck. Maybe that's just a personal preference though 🙂

dantb · 2023-09-27T19:42:13Z

Failing test is just to do with how we respond when rev is missing, will fix tomorrow

...sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/organizations/OrganizationDeleter.scala

imsdu · 2023-09-28T07:18:02Z

...sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/organizations/OrganizationDeleter.scala

+      } yield ()
+
+    private def deleteGlobalQuery(id: IriOrBNode.Iri, tpe: EntityType, table: String): ConnectionIO[Unit] =
+      Update[(EntityType, IriOrBNode.Iri)](s"DELETE FROM $table WHERE" ++ " type = ? AND id = ?").run((tpe, id)).void


You could use the sql interpolator here

It's not possible when parameterising over the table name since substitutions only work for values. This is maybe a bit overkill but I was curious to see if it was possible 😅

Fragment.const could also help here ? (not sure)

...src/test/scala/ch/epfl/bluebrain/nexus/delta/sdk/organizations/OrganizationDeleterSpec.scala

delta/sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/acls/Acls.scala

imsdu · 2023-09-28T07:43:11Z

For the ACLs I manually wrote the deletion queries rather than reusing Acls, is this ok? The main reason was so it could all be in one transaction, since this feels like a bad thing to get stuck in a half-way state. It would also add a bigger dependency.

I am ok with that

What places do I need to update with the new permission? Is it in infra somewhere?

Nothing to do with the permission itself but we will need to assign it to a user to test it

Spent far too long trying to make the integration tests sane 😅 I wasn't sure the best approach and ended up using the old school arrange / act / assert. Preferably the tests should be independent so the tables get truncated before each. I don't really like the dodgy Monix type signatures though... Guess it comes down to trying to wrangle errors into a single type - I just didn't bother and opted for Any grimness. Is there a better way?

Not sure we can do much better with this kind of PR but as this part is mostly independent from the rest, you may have opted for Cats-Effect in the end :) .

The unit tests do a lot of global state mutating and depend on each other a lot (as we've discussed before). It's a bit like a mishmash of pure FP and side effects which makes it quite difficult to reason about. I just threw more crap on the pile for this change. It would be nice to try to improve them at some point though - are there any plans for that? I could imagine tests like these being quite nice using CE, http4s, munit and scalacheck. Maybe that's just a personal preference though

There is definitely room to improve things by making the code more modular, by improving the way we use MUnit (like setting up CE Resources and transform to MUnit fixtures the latest possible as those don't compose), by designing how to test the lifecycle of a resource without too much repetition and slowing them down too much...

delta/app/src/main/scala/ch/epfl/bluebrain/nexus/delta/routes/OrganizationsRoutes.scala

...src/test/scala/ch/epfl/bluebrain/nexus/delta/sdk/organizations/OrganizationDeleterSpec.scala

… data

...main/scala/ch/epfl/bluebrain/nexus/delta/sdk/organizations/model/OrganizationRejection.scala

delta/sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/permissions/Permissions.scala

imsdu · 2023-10-03T09:31:15Z

delta/app/src/test/scala/ch/epfl/bluebrain/nexus/delta/routes/OrganizationsRoutesSpec.scala

+
+  private lazy val orgs                            = OrganizationsImpl(Set(aopd), config, xas)
+  private lazy val orgDeleter: OrganizationDeleter = id =>
+    if (id == org1.label) IO.raiseError(OrganizationNonEmpty(id))


IO.raiseWhen would make it shorter

imsdu · 2023-10-03T09:36:50Z

delta/sourcing-psql/src/main/scala/ch/epfl/bluebrain/nexus/delta/sourcing/Transactors.scala

-) {
+)(implicit s: Scheduler) {
+
+  def readCE: Transactor[IO]  = read.mapK(BIO.liftTo)


Nice that something like this is possible

imsdu · 2023-10-03T09:39:57Z

docs/src/main/paradox/docs/releases/v1.9-release-notes.md

@@ -123,6 +123,12 @@ Creating an archive now requires only the `resources/read` permission instead of

 Tarball archives are no longer supported due to unnecessary restrictions. ZIP is now the only allowed format and clients should send `application/zip` in the `Accept` header when creating archives.

+### Organizations
+
+#### Support deletion of empty organizations


Link to the operation in the API ?

dantb requested review from shinyhappydan, imsdu and olivergrabinski September 26, 2023 07:39

dantb force-pushed the delete-org-support branch 3 times, most recently from dcb4f83 to b90801e Compare September 27, 2023 19:41

dantb marked this pull request as ready for review September 27, 2023 19:41

imsdu requested changes Sep 28, 2023

View reviewed changes

imsdu reviewed Sep 28, 2023

View reviewed changes

delta/sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/acls/Acls.scala Outdated Show resolved Hide resolved

shinyhappydan reviewed Sep 28, 2023

View reviewed changes

delta/app/src/main/scala/ch/epfl/bluebrain/nexus/delta/routes/OrganizationsRoutes.scala Outdated Show resolved Hide resolved

...src/test/scala/ch/epfl/bluebrain/nexus/delta/sdk/organizations/OrganizationDeleterSpec.scala Outdated Show resolved Hide resolved

Add organization deletion route - delete scoped partitions and global…

e3564a1

… data

dantb force-pushed the delete-org-support branch from 8a3b232 to e3564a1 Compare September 30, 2023 13:26

Docs for org deletion

131e4d9

shinyhappydan previously approved these changes Oct 2, 2023

View reviewed changes

...main/scala/ch/epfl/bluebrain/nexus/delta/sdk/organizations/model/OrganizationRejection.scala Outdated Show resolved Hide resolved

delta/sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/permissions/Permissions.scala Outdated Show resolved Hide resolved

Tidy

ce9c81d

dantb dismissed shinyhappydan’s stale review via ce9c81d October 3, 2023 07:34

imsdu reviewed Oct 3, 2023

View reviewed changes

imsdu previously approved these changes Oct 3, 2023

View reviewed changes

Tweaks

a6b2e15

dantb dismissed imsdu’s stale review via a6b2e15 October 4, 2023 07:56

Merge branch 'master' into delete-org-support

d56363a

imsdu approved these changes Oct 4, 2023

View reviewed changes

dantb merged commit d8ab831 into BlueBrain:master Oct 4, 2023
6 checks passed

dantb deleted the delete-org-support branch October 4, 2023 09:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add organization deletion route #4298

Add organization deletion route #4298

dantb commented Sep 26, 2023 •

edited

Loading

dantb commented Sep 27, 2023

imsdu Sep 28, 2023

dantb Sep 28, 2023

imsdu Oct 3, 2023 •

edited

Loading

imsdu commented Sep 28, 2023

imsdu Oct 3, 2023 •

edited

Loading

imsdu Oct 3, 2023

imsdu Oct 3, 2023

Add organization deletion route #4298

Add organization deletion route #4298

Conversation

dantb commented Sep 26, 2023 • edited Loading

dantb commented Sep 27, 2023

imsdu Sep 28, 2023

Choose a reason for hiding this comment

dantb Sep 28, 2023

Choose a reason for hiding this comment

imsdu Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

imsdu commented Sep 28, 2023

imsdu Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

imsdu Oct 3, 2023

Choose a reason for hiding this comment

imsdu Oct 3, 2023

Choose a reason for hiding this comment

dantb commented Sep 26, 2023 •

edited

Loading

imsdu Oct 3, 2023 •

edited

Loading

imsdu Oct 3, 2023 •

edited

Loading