From 355564b4d17329eac9758d80f7bd32ab369e5589 Mon Sep 17 00:00:00 2001 From: Gunnar Velle Date: Thu, 21 May 2026 12:33:27 +0200 Subject: [PATCH 1/6] Feat: Add editor table for images --- .../migration/V29__AddImageEditorsTable.sql | 29 ++++++++++++ .../imageapi/repository/ImageRepository.scala | 46 ++++++++++++++++--- 2 files changed, 68 insertions(+), 7 deletions(-) create mode 100644 image-api/src/main/resources/no/ndla/imageapi/db/migration/V29__AddImageEditorsTable.sql diff --git a/image-api/src/main/resources/no/ndla/imageapi/db/migration/V29__AddImageEditorsTable.sql b/image-api/src/main/resources/no/ndla/imageapi/db/migration/V29__AddImageEditorsTable.sql new file mode 100644 index 0000000000..ea7d089c6c --- /dev/null +++ b/image-api/src/main/resources/no/ndla/imageapi/db/migration/V29__AddImageEditorsTable.sql @@ -0,0 +1,29 @@ +create table image_editors +( + image_id bigint not null, + user_id text not null, + primary key (image_id, user_id) +); + +insert into image_editors (image_id, user_id) +select note_users.image_id, note_users.user_id +from (select id as image_id, + notes ->> 'updatedBy' as user_id + from imagemetadata + cross join lateral jsonb_array_elements(metadata -> 'editorNotes') as notes) as note_users +where user_id is not null; + +insert into image_editors (image_id, user_id) +select id, metadata ->> 'createdBy' +from imagemetadata +where metadata is not null + and metadata ->> 'createdBy' is not null +on conflict do nothing; + +insert into image_editors (image_id, user_id) +select id, metadata ->> 'updatedBy' +from imagemetadata +where metadata is not null + and metadata ->> 'updatedBy' is not null +on conflict do nothing; + diff --git a/image-api/src/main/scala/no/ndla/imageapi/repository/ImageRepository.scala b/image-api/src/main/scala/no/ndla/imageapi/repository/ImageRepository.scala index f64efd1988..f14454f6c3 100644 --- a/image-api/src/main/scala/no/ndla/imageapi/repository/ImageRepository.scala +++ b/image-api/src/main/scala/no/ndla/imageapi/repository/ImageRepository.scala @@ -47,9 +47,11 @@ class ImageRepository(using dbUtility: DBUtility, dbImageMetaInformation: DBImag dataObject.setType("jsonb") dataObject.setValue(CirceUtil.toJsonString(imageMeta)) - tsql"insert into imagemetadata(metadata) values ($dataObject)" - .updateAndReturnGeneratedKey() - .map(id => imageMeta.copy(id = Some(id))) + for { + id <- tsql"insert into imagemetadata(metadata) values ($dataObject)".updateAndReturnGeneratedKey() + inserted = imageMeta.copy(id = Some(id)) + tracked <- trackEditors(inserted) + } yield tracked def update(imageMetaInformation: ImageMetaInformation, id: Long)(implicit session: DBSession = dbUtility.autoSession @@ -58,9 +60,37 @@ class ImageRepository(using dbUtility: DBUtility, dbImageMetaInformation: DBImag val dataObject = new PGobject() dataObject.setType("jsonb") dataObject.setValue(json) - tsql"update imagemetadata set metadata = $dataObject where id = $id" - .update() - .map(_ => imageMetaInformation.copy(id = Some(id))) + for { + updated <- tsql"update imagemetadata set metadata = $dataObject where id = $id" + .update() + .map(_ => imageMetaInformation.copy(id = Some(id))) + tracked <- trackEditors(updated) + } yield tracked + } + + def getAllEditors(implicit session: DBSession = dbUtility.readOnlySession): Try[Seq[String]] = + tsql"select distinct user_id from image_editors".map(rs => rs.string("user_id")).runList() + + private def trackEditors(image: ImageMetaInformation)(implicit session: DBSession): Try[ImageMetaInformation] = { + image.id match { + case None => Success(image) + case Some(id) => + val userIds = ( + Seq(image.createdBy, image.updatedBy) ++ + image.editorNotes.map(_.updatedBy) + ).distinct + + userIds + .map { userId => + tsql""" + insert into image_editors (image_id, user_id) + values ($id, $userId) + on conflict do nothing + """.update() + } + .sequence + .map(_ => image) + } } def delete(imageId: Long)(implicit session: DBSession = dbUtility.autoSession): Try[Int] = Try { @@ -68,7 +98,9 @@ class ImageRepository(using dbUtility: DBUtility, dbImageMetaInformation: DBImag }.flatMap { case n if n < 1 => Failure(new ImageNotFoundException(s"Image with id $imageId was not found, and could not be deleted.")) - case n => Success(n) + case n => + val _ = tsql"delete from image_editors where image_id = $imageId".update().get + Success(n) } def minMaxId: Try[(Long, Long)] = dbUtility.readOnly { implicit session => From 46566015b244d6e8f24bd6987a8ff2aba0a1ab83 Mon Sep 17 00:00:00 2001 From: Gunnar Velle Date: Thu, 21 May 2026 12:43:40 +0200 Subject: [PATCH 2/6] Add test to verify updating editor-table --- .../repository/ImageRepositoryTest.scala | 25 +++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/image-api/src/test/scala/no/ndla/imageapi/repository/ImageRepositoryTest.scala b/image-api/src/test/scala/no/ndla/imageapi/repository/ImageRepositoryTest.scala index 141eb45364..ff65e8b5e1 100644 --- a/image-api/src/test/scala/no/ndla/imageapi/repository/ImageRepositoryTest.scala +++ b/image-api/src/test/scala/no/ndla/imageapi/repository/ImageRepositoryTest.scala @@ -9,10 +9,11 @@ package no.ndla.imageapi.repository import java.net.Socket -import no.ndla.imageapi.model.domain.ImageTitle +import no.ndla.imageapi.model.domain.{EditorNote, ImageTitle} import no.ndla.imageapi.{ImageApiProperties, TestEnvironment, UnitSuite} import no.ndla.scalatestsuite.DatabaseIntegrationSuite import no.ndla.database.{DataSource, DBMigrator, DBUtility} +import no.ndla.common.model.NDLADate import scala.util.{Success, Try} import scalikejdbc.* @@ -36,7 +37,7 @@ class ImageRepositoryTest extends DatabaseIntegrationSuite with UnitSuite with T } def emptyTestDatabase: Boolean = dbUtility.writeSession(implicit session => { - sql"delete from imagemetadata;".execute()(using session) + sql"delete from imagemetadata; delete from image_editors;".execute()(using session) }) override def beforeAll(): Unit = { @@ -144,4 +145,24 @@ class ImageRepositoryTest extends DatabaseIntegrationSuite with UnitSuite with T repository.getImageFromFilePath(path2).get should be(Some(expected2)) } + test("That inserting and updating images updates the image_editors table") { + repository.getAllEditors.get should be(Nil) + + val image = TestData.bjorn.copy(id = None, createdBy = "creator-1", updatedBy = "editor-1", editorNotes = Seq.empty) + val inserted = repository.insert(image).failIfFailure + val id = inserted.id.get + + repository.getAllEditors.get.toSet should be(Set("creator-1", "editor-1")) + + val note = EditorNote(NDLADate.now(), "note-editor-1", "Some note") + val updated = inserted.copy(updatedBy = "editor-2", editorNotes = Seq(note)) + repository.update(updated, id).failIfFailure + + repository.getAllEditors.get.toSet should be(Set("creator-1", "editor-1", "editor-2", "note-editor-1")) + + repository.delete(id).failIfFailure + + repository.getAllEditors.get should be(Nil) + } + } From 77736b4c7554a8f02a5316b834a141818133e015 Mon Sep 17 00:00:00 2001 From: Gunnar Velle Date: Thu, 21 May 2026 14:46:47 +0200 Subject: [PATCH 3/6] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../ndla/imageapi/db/migration/V29__AddImageEditorsTable.sql | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/image-api/src/main/resources/no/ndla/imageapi/db/migration/V29__AddImageEditorsTable.sql b/image-api/src/main/resources/no/ndla/imageapi/db/migration/V29__AddImageEditorsTable.sql index ea7d089c6c..31d3131a63 100644 --- a/image-api/src/main/resources/no/ndla/imageapi/db/migration/V29__AddImageEditorsTable.sql +++ b/image-api/src/main/resources/no/ndla/imageapi/db/migration/V29__AddImageEditorsTable.sql @@ -6,12 +6,13 @@ create table image_editors ); insert into image_editors (image_id, user_id) -select note_users.image_id, note_users.user_id +select distinct note_users.image_id, note_users.user_id from (select id as image_id, notes ->> 'updatedBy' as user_id from imagemetadata cross join lateral jsonb_array_elements(metadata -> 'editorNotes') as notes) as note_users -where user_id is not null; +where user_id is not null +on conflict do nothing; insert into image_editors (image_id, user_id) select id, metadata ->> 'createdBy' From 49b39acb29ea7245608c015090769ccc4749fd80 Mon Sep 17 00:00:00 2001 From: Gunnar Velle Date: Thu, 21 May 2026 14:46:59 +0200 Subject: [PATCH 4/6] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../no/ndla/imageapi/repository/ImageRepositoryTest.scala | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/image-api/src/test/scala/no/ndla/imageapi/repository/ImageRepositoryTest.scala b/image-api/src/test/scala/no/ndla/imageapi/repository/ImageRepositoryTest.scala index ff65e8b5e1..5a2d9ce6b2 100644 --- a/image-api/src/test/scala/no/ndla/imageapi/repository/ImageRepositoryTest.scala +++ b/image-api/src/test/scala/no/ndla/imageapi/repository/ImageRepositoryTest.scala @@ -37,7 +37,8 @@ class ImageRepositoryTest extends DatabaseIntegrationSuite with UnitSuite with T } def emptyTestDatabase: Boolean = dbUtility.writeSession(implicit session => { - sql"delete from imagemetadata; delete from image_editors;".execute()(using session) + sql"delete from image_editors".execute()(using session) + sql"delete from imagemetadata".execute()(using session) }) override def beforeAll(): Unit = { From af88d50c40ae7f203c546d7fcc5baa9621d57021 Mon Sep 17 00:00:00 2001 From: Gunnar Velle Date: Fri, 22 May 2026 14:26:25 +0200 Subject: [PATCH 5/6] No need to take notes into consideration on update --- .../imageapi/repository/ImageRepository.scala | 24 +++++++------------ .../repository/ImageRepositoryTest.scala | 6 ++--- 2 files changed, 11 insertions(+), 19 deletions(-) diff --git a/image-api/src/main/scala/no/ndla/imageapi/repository/ImageRepository.scala b/image-api/src/main/scala/no/ndla/imageapi/repository/ImageRepository.scala index f14454f6c3..d87bb44a5b 100644 --- a/image-api/src/main/scala/no/ndla/imageapi/repository/ImageRepository.scala +++ b/image-api/src/main/scala/no/ndla/imageapi/repository/ImageRepository.scala @@ -72,25 +72,17 @@ class ImageRepository(using dbUtility: DBUtility, dbImageMetaInformation: DBImag tsql"select distinct user_id from image_editors".map(rs => rs.string("user_id")).runList() private def trackEditors(image: ImageMetaInformation)(implicit session: DBSession): Try[ImageMetaInformation] = { - image.id match { - case None => Success(image) - case Some(id) => - val userIds = ( - Seq(image.createdBy, image.updatedBy) ++ - image.editorNotes.map(_.updatedBy) - ).distinct - - userIds - .map { userId => - tsql""" + image + .id + .map { id => + tsql""" insert into image_editors (image_id, user_id) - values ($id, $userId) + values ($id, ${image.updatedBy}) on conflict do nothing """.update() - } - .sequence - .map(_ => image) - } + } + .getOrElse(Success(())) + .map(_ => image) } def delete(imageId: Long)(implicit session: DBSession = dbUtility.autoSession): Try[Int] = Try { diff --git a/image-api/src/test/scala/no/ndla/imageapi/repository/ImageRepositoryTest.scala b/image-api/src/test/scala/no/ndla/imageapi/repository/ImageRepositoryTest.scala index 5a2d9ce6b2..23afc9917e 100644 --- a/image-api/src/test/scala/no/ndla/imageapi/repository/ImageRepositoryTest.scala +++ b/image-api/src/test/scala/no/ndla/imageapi/repository/ImageRepositoryTest.scala @@ -153,13 +153,13 @@ class ImageRepositoryTest extends DatabaseIntegrationSuite with UnitSuite with T val inserted = repository.insert(image).failIfFailure val id = inserted.id.get - repository.getAllEditors.get.toSet should be(Set("creator-1", "editor-1")) + repository.getAllEditors.get.toSet should be(Set("editor-1")) val note = EditorNote(NDLADate.now(), "note-editor-1", "Some note") - val updated = inserted.copy(updatedBy = "editor-2", editorNotes = Seq(note)) + val updated = inserted.copy(updatedBy = "note-editor-1", editorNotes = Seq(note)) repository.update(updated, id).failIfFailure - repository.getAllEditors.get.toSet should be(Set("creator-1", "editor-1", "editor-2", "note-editor-1")) + repository.getAllEditors.get.toSet should be(Set("editor-1", "note-editor-1")) repository.delete(id).failIfFailure From c1620a5c4ae3264bc9cf222fc2e1596b8774d498 Mon Sep 17 00:00:00 2001 From: Gunnar Velle Date: Wed, 27 May 2026 12:43:20 +0200 Subject: [PATCH 6/6] Rewrite delete code to enforce atomic commit --- .../imageapi/repository/ImageRepository.scala | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/image-api/src/main/scala/no/ndla/imageapi/repository/ImageRepository.scala b/image-api/src/main/scala/no/ndla/imageapi/repository/ImageRepository.scala index d87bb44a5b..ad9654e47b 100644 --- a/image-api/src/main/scala/no/ndla/imageapi/repository/ImageRepository.scala +++ b/image-api/src/main/scala/no/ndla/imageapi/repository/ImageRepository.scala @@ -50,7 +50,7 @@ class ImageRepository(using dbUtility: DBUtility, dbImageMetaInformation: DBImag for { id <- tsql"insert into imagemetadata(metadata) values ($dataObject)".updateAndReturnGeneratedKey() inserted = imageMeta.copy(id = Some(id)) - tracked <- trackEditors(inserted) + tracked <- trackEditor(inserted) } yield tracked def update(imageMetaInformation: ImageMetaInformation, id: Long)(implicit @@ -64,14 +64,14 @@ class ImageRepository(using dbUtility: DBUtility, dbImageMetaInformation: DBImag updated <- tsql"update imagemetadata set metadata = $dataObject where id = $id" .update() .map(_ => imageMetaInformation.copy(id = Some(id))) - tracked <- trackEditors(updated) + tracked <- trackEditor(updated) } yield tracked } def getAllEditors(implicit session: DBSession = dbUtility.readOnlySession): Try[Seq[String]] = tsql"select distinct user_id from image_editors".map(rs => rs.string("user_id")).runList() - private def trackEditors(image: ImageMetaInformation)(implicit session: DBSession): Try[ImageMetaInformation] = { + private def trackEditor(image: ImageMetaInformation)(implicit session: DBSession): Try[ImageMetaInformation] = { image .id .map { id => @@ -85,14 +85,17 @@ class ImageRepository(using dbUtility: DBUtility, dbImageMetaInformation: DBImag .map(_ => image) } - def delete(imageId: Long)(implicit session: DBSession = dbUtility.autoSession): Try[Int] = Try { - tsql"delete from imagemetadata where id = $imageId".update().get - }.flatMap { - case n if n < 1 => - Failure(new ImageNotFoundException(s"Image with id $imageId was not found, and could not be deleted.")) - case n => - val _ = tsql"delete from image_editors where image_id = $imageId".update().get - Success(n) + def delete(imageId: Long)(implicit session: DBSession = dbUtility.autoSession): Try[Int] = { + for { + result <- tsql"delete from imagemetadata where id = $imageId" + .update() + .flatMap { + case n if n < 1 => + Failure(new ImageNotFoundException(s"Image with id $imageId was not found, and could not be deleted.")) + case n => Success(n) + } + _ <- tsql"delete from image_editors where image_id = $imageId".update() + } yield result } def minMaxId: Try[(Long, Long)] = dbUtility.readOnly { implicit session =>