From ce526d6ad28a529a9f018c45dff6b4d67eeed7f7 Mon Sep 17 00:00:00 2001 From: Jakub Chrobasik Date: Tue, 26 Mar 2024 11:31:31 +0100 Subject: [PATCH] fix: document updates to work in cases user role is changed (#73) --- .../search/provision/MessageHandlers.scala | 6 +- .../provision/handler/DocumentConverter.scala | 2 +- .../provision/handler/DocumentMerger.scala | 2 +- .../provision/handler/DocumentUpdates.scala | 2 +- .../provision/handler/TypeTransformers.scala | 2 +- .../provision/project/ProjectSyntax.scala | 2 +- .../solr/documents/EntityDocument.scala | 20 ++- .../documents/PartialEntityDocument.scala | 8 +- .../solr/documents/PartialProjectSpec.scala | 114 ++++++++++++++++++ .../search/solr/documents/ProjectSpec.scala | 13 ++ 10 files changed, 154 insertions(+), 17 deletions(-) create mode 100644 modules/search-solr-client/src/test/scala/io/renku/search/solr/documents/PartialProjectSpec.scala diff --git a/modules/search-provision/src/main/scala/io/renku/search/provision/MessageHandlers.scala b/modules/search-provision/src/main/scala/io/renku/search/provision/MessageHandlers.scala index e2128bc9..38b01901 100644 --- a/modules/search-provision/src/main/scala/io/renku/search/provision/MessageHandlers.scala +++ b/modules/search-provision/src/main/scala/io/renku/search/provision/MessageHandlers.scala @@ -46,7 +46,7 @@ final class MessageHandlers[F[_]: Async]( val projectCreated: Stream[F, Unit] = add(cfg.projectCreated, makeUpsert[ProjectCreated](cfg.projectCreated)) - val projectUpdated = + val projectUpdated: Stream[F, Unit] = add( cfg.projectUpdated, makeUpdated[ProjectUpdated](cfg.projectUpdated, DocumentUpdates.project) @@ -75,10 +75,10 @@ final class MessageHandlers[F[_]: Async]( val userAdded: Stream[F, Unit] = add(cfg.userAdded, makeCreated[UserAdded](cfg.userAdded)) - val userUpdated = + val userUpdated: Stream[F, Unit] = add(cfg.userUpdated, makeUpdated[UserUpdated](cfg.userUpdated, DocumentUpdates.user)) - val userRemoved = + val userRemoved: Stream[F, Unit] = val ps = steps(cfg.userRemoved) add( cfg.userRemoved, diff --git a/modules/search-provision/src/main/scala/io/renku/search/provision/handler/DocumentConverter.scala b/modules/search-provision/src/main/scala/io/renku/search/provision/handler/DocumentConverter.scala index 7584adeb..a61d82fc 100644 --- a/modules/search-provision/src/main/scala/io/renku/search/provision/handler/DocumentConverter.scala +++ b/modules/search-provision/src/main/scala/io/renku/search/provision/handler/DocumentConverter.scala @@ -21,7 +21,7 @@ package io.renku.search.provision.handler import io.github.arainko.ducktape.* import io.renku.events.v1.* import io.renku.search.model.Id -import io.renku.search.provision.TypeTransformers.given +import io.renku.search.provision.handler.TypeTransformers.given import io.renku.search.solr.documents.EntityDocument import io.renku.search.solr.documents.Project as ProjectDocument import io.renku.search.solr.documents.User as UserDocument diff --git a/modules/search-provision/src/main/scala/io/renku/search/provision/handler/DocumentMerger.scala b/modules/search-provision/src/main/scala/io/renku/search/provision/handler/DocumentMerger.scala index 2c951402..a77cd3d6 100644 --- a/modules/search-provision/src/main/scala/io/renku/search/provision/handler/DocumentMerger.scala +++ b/modules/search-provision/src/main/scala/io/renku/search/provision/handler/DocumentMerger.scala @@ -23,7 +23,7 @@ import cats.syntax.all.* import io.github.arainko.ducktape.* import io.renku.events.v1.* import io.renku.search.model.Id -import io.renku.search.provision.TypeTransformers.given +import io.renku.search.provision.handler.TypeTransformers.given import io.renku.search.solr.documents.EntityDocument import io.renku.search.solr.documents.PartialEntityDocument import io.renku.search.solr.documents.Project as ProjectDocument diff --git a/modules/search-provision/src/main/scala/io/renku/search/provision/handler/DocumentUpdates.scala b/modules/search-provision/src/main/scala/io/renku/search/provision/handler/DocumentUpdates.scala index 5c8529b1..a1c78b61 100644 --- a/modules/search-provision/src/main/scala/io/renku/search/provision/handler/DocumentUpdates.scala +++ b/modules/search-provision/src/main/scala/io/renku/search/provision/handler/DocumentUpdates.scala @@ -23,7 +23,7 @@ import cats.syntax.all.* import io.github.arainko.ducktape.* import io.renku.events.v1.* import io.renku.search.model.Id -import io.renku.search.provision.TypeTransformers.given +import io.renku.search.provision.handler.TypeTransformers.given import io.renku.search.solr.documents.EntityDocument import io.renku.search.solr.documents.Project as ProjectDocument import io.renku.search.solr.documents.User as UserDocument diff --git a/modules/search-provision/src/main/scala/io/renku/search/provision/handler/TypeTransformers.scala b/modules/search-provision/src/main/scala/io/renku/search/provision/handler/TypeTransformers.scala index 8c151493..64ebff2c 100644 --- a/modules/search-provision/src/main/scala/io/renku/search/provision/handler/TypeTransformers.scala +++ b/modules/search-provision/src/main/scala/io/renku/search/provision/handler/TypeTransformers.scala @@ -16,7 +16,7 @@ * limitations under the License. */ -package io.renku.search.provision +package io.renku.search.provision.handler import io.github.arainko.ducktape.* import io.renku.events.v1 diff --git a/modules/search-provision/src/test/scala/io/renku/search/provision/project/ProjectSyntax.scala b/modules/search-provision/src/test/scala/io/renku/search/provision/project/ProjectSyntax.scala index dcb47c2e..bb350d33 100644 --- a/modules/search-provision/src/test/scala/io/renku/search/provision/project/ProjectSyntax.scala +++ b/modules/search-provision/src/test/scala/io/renku/search/provision/project/ProjectSyntax.scala @@ -22,7 +22,7 @@ import io.github.arainko.ducktape.* import io.renku.events.v1.ProjectCreated import io.renku.events.v1.ProjectUpdated import io.renku.search.model.Id -import io.renku.search.provision.TypeTransformers.given +import io.renku.search.provision.handler.TypeTransformers.given import io.renku.search.solr.documents.Project trait ProjectSyntax: diff --git a/modules/search-solr-client/src/main/scala/io/renku/search/solr/documents/EntityDocument.scala b/modules/search-solr-client/src/main/scala/io/renku/search/solr/documents/EntityDocument.scala index 779216dc..ffe53075 100644 --- a/modules/search-solr-client/src/main/scala/io/renku/search/solr/documents/EntityDocument.scala +++ b/modules/search-solr-client/src/main/scala/io/renku/search/solr/documents/EntityDocument.scala @@ -18,15 +18,15 @@ package io.renku.search.solr.documents -import io.bullet.borer.NullOptions.given import io.bullet.borer.* +import io.bullet.borer.NullOptions.given import io.bullet.borer.derivation.MapBasedCodecs import io.renku.search.model.* -import io.renku.search.model.projects.MemberRole import io.renku.search.model.projects.MemberRole.{Member, Owner} -import io.renku.search.model.projects.Visibility +import io.renku.search.model.projects.{MemberRole, Visibility} import io.renku.search.solr.schema.EntityDocumentSchema.Fields import io.renku.solr.client.EncoderSupport +import io.renku.solr.client.EncoderSupport.* sealed trait EntityDocument extends SolrDocument: val score: Option[Double] @@ -55,8 +55,18 @@ final case class Project( ) extends EntityDocument: def addMember(userId: Id, role: MemberRole): Project = role match { - case Owner => copy(owners = (userId :: owners).distinct, score = None) - case Member => copy(members = (userId :: members).distinct, score = None) + case Owner => + copy( + owners = (userId :: owners).distinct, + members = members.filterNot(_ == userId).distinct, + score = None + ) + case Member => + copy( + owners = owners.filterNot(_ == userId).distinct, + members = (userId :: members).distinct, + score = None + ) } def addMembers(role: MemberRole, ids: List[Id]): Project = role match diff --git a/modules/search-solr-client/src/main/scala/io/renku/search/solr/documents/PartialEntityDocument.scala b/modules/search-solr-client/src/main/scala/io/renku/search/solr/documents/PartialEntityDocument.scala index b6ca2533..eb45f49e 100644 --- a/modules/search-solr-client/src/main/scala/io/renku/search/solr/documents/PartialEntityDocument.scala +++ b/modules/search-solr-client/src/main/scala/io/renku/search/solr/documents/PartialEntityDocument.scala @@ -48,8 +48,8 @@ object PartialEntityDocument: def remove(id: Id): Project = copy(owners = owners - id, members = members - id) def add(id: Id, role: MemberRole): Project = role match - case MemberRole.Owner => copy(owners = owners + id) - case MemberRole.Member => copy(members = members + id) + case MemberRole.Owner => copy(owners = owners + id, members = members - id) + case MemberRole.Member => copy(members = members + id, owners = owners - id) def applyTo(e: EntityDocument): EntityDocument = e match @@ -61,8 +61,8 @@ object PartialEntityDocument: def combine(p: Project): Project = if (p.id == id) p.copy( - members = p.members ++ members, - owners = p.owners ++ owners + members = p.members ++ (members -- p.owners), + owners = p.owners ++ (owners -- p.members) ) else p diff --git a/modules/search-solr-client/src/test/scala/io/renku/search/solr/documents/PartialProjectSpec.scala b/modules/search-solr-client/src/test/scala/io/renku/search/solr/documents/PartialProjectSpec.scala new file mode 100644 index 00000000..2bc7ffdb --- /dev/null +++ b/modules/search-solr-client/src/test/scala/io/renku/search/solr/documents/PartialProjectSpec.scala @@ -0,0 +1,114 @@ +/* + * Copyright 2024 Swiss Data Science Center (SDSC) + * A partnership between École Polytechnique Fédérale de Lausanne (EPFL) and + * Eidgenössische Technische Hochschule Zürich (ETHZ). + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.renku.search.solr.documents + +import io.renku.search.GeneratorSyntax +import io.renku.search.model.ModelGenerators.{idGen, projectMemberRoleGen} +import io.renku.search.model.projects +import io.renku.search.model.projects.MemberRole.{Member, Owner} +import io.renku.search.solr.client.SolrDocumentGenerators.partialProjectGen +import io.renku.search.solr.documents.PartialEntityDocument.Project +import munit.{FunSuite, ScalaCheckSuite} +import org.scalacheck.Prop + +class PartialProjectSpec extends ScalaCheckSuite with GeneratorSyntax: + + test("add should add the userId to the relevant bucket"): + Prop.forAll(partialProjectGen, idGen, projectMemberRoleGen) { + case (existing, userId, role) => + val updated = existing.add(userId, role) + + role match { + case Owner => + assertEquals(updated.owners, existing.owners + userId) + assertEquals(updated.members, existing.members) + case Member => + assertEquals(updated.members, existing.members + userId) + assertEquals(updated.owners, existing.owners) + } + } + + test( + "add should add the userId to the relevant bucket and remove from the other bucket if existed" + ): + Prop.forAll(idGen, idGen, projectMemberRoleGen) { case (projectId, userId, role) => + val existing = Project(projectId, Set(userId), Set(userId)) + + val updated = existing.add(userId, role) + + role match { + case Owner => + assertEquals(updated.owners, existing.owners) + assertEquals(updated.members, existing.members - userId) + case Member => + assertEquals(updated.members, existing.members) + assertEquals(updated.owners, existing.owners - userId) + } + } + + test("applyTo should add members and owners from the caller object"): + Prop.forAll(partialProjectGen, idGen, projectMemberRoleGen) { + case (existing, userId, role) => + val update = Project(existing.id).add(userId, role) + + val updated = existing.applyTo(update).asInstanceOf[Project] + + role match { + case Owner => + assertEquals(updated.owners, existing.owners + userId) + assertEquals(updated.members, existing.members) + case Member => + assertEquals(updated.members, existing.members + userId) + assertEquals(updated.owners, existing.owners) + } + } + + test( + "applyTo should add members and owners from the caller object except those that have been moved between buckets" + ): + Prop.forAll(idGen, idGen, projectMemberRoleGen) { case (projectId, userId, role) => + val existing = Project(projectId, Set(userId), Set(userId)) + + val update = Project(projectId).add(userId, role) + + val updated = existing.applyTo(update).asInstanceOf[Project] + + role match { + case Owner => + assertEquals(updated.owners, existing.owners) + assertEquals(updated.members, existing.members - userId) + case Member => + assertEquals(updated.members, existing.members) + assertEquals(updated.owners, existing.owners - userId) + } + } + + test("removeMember should remove the given userId from the correct bucket in the doc"): + Prop.forAll(partialProjectGen, idGen, projectMemberRoleGen) { + case (project, userId, role) => + val updated = project.add(userId, role) + assertEquals(updated.remove(userId), project) + } + + test("removeMember should do nothing if there's no member/owner with the given userId"): + Prop.forAll(partialProjectGen, idGen, idGen, projectMemberRoleGen) { + case (project, existingUserId, toDeleteUserId, role) => + val updated = project.add(existingUserId, role) + assertEquals(updated.remove(toDeleteUserId), updated) + } diff --git a/modules/search-solr-client/src/test/scala/io/renku/search/solr/documents/ProjectSpec.scala b/modules/search-solr-client/src/test/scala/io/renku/search/solr/documents/ProjectSpec.scala index e4a8f2f2..dfd33382 100644 --- a/modules/search-solr-client/src/test/scala/io/renku/search/solr/documents/ProjectSpec.scala +++ b/modules/search-solr-client/src/test/scala/io/renku/search/solr/documents/ProjectSpec.scala @@ -19,6 +19,7 @@ package io.renku.search.solr.documents import io.renku.search.model.ModelGenerators.{idGen, projectMemberRoleGen} +import io.renku.search.model.projects import io.renku.search.model.projects.MemberRole.{Member, Owner} import io.renku.search.solr.client.SolrDocumentGenerators.projectDocumentGen import munit.{FunSuite, ScalaCheckSuite} @@ -38,6 +39,18 @@ class ProjectSpec extends ScalaCheckSuite: } } + test( + "addMember should add the given userId to the correct bucket and remove from the other buckets" + ): + Prop.forAll(projectDocumentGen, idGen) { case (project, userId) => + val updated = project + .addMember(userId, projects.MemberRole.Member) + .addMember(userId, projects.MemberRole.Owner) + + assertEquals(updated.owners, (userId :: project.owners).distinct) + assertEquals(updated.members, project.members.filterNot(_ == userId).distinct) + } + test("addMember should not add duplicates"): Prop.forAll(projectDocumentGen, idGen, projectMemberRoleGen) { case (project, userId, role) =>