refactor: move value objects to separate project (DEV-615) #2069

mpro7 · 2022-05-25T15:52:10Z

PR Checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

What is the current behavior?

Issue Number: DEV-615

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

BalduinLandolt

Another big one, nice job!

It mostly looks good (and most of my comments are cosmetic or can be ignored).

One issue needs addressing though:

I think the way the ZIO Tests are written is not yet entirely correct

as far as I remember, multiple assertTrue() statements must be combined with && otherwise only the last one is actually tested.
I think multiple test() in one suite() should be combined with comma, not plus.

I used this file as a reference: https://github.com/zio/zio-schema/blob/main/zio-schema/shared/src/test/scala/zio/schema/ValidationSpec.scala

dsp-value-objects/src/main/scala/dsp/valueobjects/Group.scala

BalduinLandolt · 2022-05-31T12:37:17Z

dsp-value-objects/src/main/scala/dsp/valueobjects/Iri.scala

+      if (value.isEmpty) {
+        Validation.fail(V2.BadRequestException(IriErrorMessages.GroupIriMissing))
+      } else {
+        val isUUID: Boolean = V2UuidValidation.hasUUIDLength(value.split("/").last)
+
+        if (!V2IriValidation.isKnoraGroupIriStr(value)) {
+          Validation.fail(V2.BadRequestException(IriErrorMessages.GroupIriInvalid))
+        } else if (isUUID && !V2UuidValidation.isUUIDVersion4Or5(value)) {
+          Validation.fail(V2.BadRequestException(IriErrorMessages.UuidInvalid))
+        } else {
+          val validatedValue = Validation(
+            V2IriValidation.validateAndEscapeIri(value, throw V2.BadRequestException(IriErrorMessages.GroupIriInvalid))
+          )
+
+          validatedValue.map(new GroupIri(_) {})


eventually it would be nice to be able to simplify these validations. It has nested conditions to the point where it gets hard to read the code. But that's for the validation epic, not for this task.

Also, I general question I have: these validation functions (like uuidHasLength() etc.), shouldn't they ideally live in the value object?
And things like UUIDs, shouldn't they themselves be value objects with their own validation?
(Again, not scope of this PR, just in terms of the design)

Thanks for the comment. Value objects refactor it's planned in next steps.

BalduinLandolt · 2022-05-31T12:38:40Z

dsp-value-objects/src/main/scala/dsp/valueobjects/Iri.scala

+  object ListIri { self =>
+    def make(value: String): Validation[Throwable, ListIri] =
+      if (value.isEmpty) {
+        Validation.fail(V2.BadRequestException(IriErrorMessages.ListIriMissing))
+      } else {
+        val isUUID: Boolean = V2UuidValidation.hasUUIDLength(value.split("/").last)
+
+        if (!V2IriValidation.isKnoraListIriStr(value)) {
+          Validation.fail(V2.BadRequestException(IriErrorMessages.ListIriInvalid))
+        } else if (isUUID && !V2UuidValidation.isUUIDVersion4Or5(value)) {
+          Validation.fail(V2.BadRequestException(IriErrorMessages.UuidInvalid))
+        } else {
+          val validatedValue = Validation(
+            V2IriValidation.validateAndEscapeIri(
+              value,
+              throw V2.BadRequestException(IriErrorMessages.ListIriInvalid)
+            )
+          )
+
+          validatedValue.map(new ListIri(_) {})


And again, these different IRI types share a lot of the same logic, so there probably should be a shared method for checking these things. (in a later PR :) )

Thanks for the comment. Value objects refactor it's planned in next steps.

dsp-value-objects/src/main/scala/dsp/valueobjects/Iri.scala

project/Dependencies.scala

webapi/src/main/scala/org/knora/webapi/messages/StringFormatter.scala

...rc/main/scala/org/knora/webapi/messages/admin/responder/listsmessages/ListsMessagesADM.scala

...pi/src/main/scala/org/knora/webapi/messages/v2/responder/valuemessages/ValueMessagesV2.scala

dsp-value-objects/src/main/scala/dsp/valueobjects/V2.scala

irinaschubert

I have only cosmetics to add. Apart from that it looks good to me and I'm curious to use these value objects in the user slice :)

irinaschubert · 2022-06-01T07:18:19Z

dsp-value-objects/src/main/scala/dsp/valueobjects/Group.scala

+  }
+
+  /**
+   * GroupDescriptions value object.


I'm not sure anymore if we discussed the naming of this already. But I think that "description" should be singular. There is only one description per group (and all other entities), just possibly in multiple languages. What do you think?

This is indeed a little bit misleading. Since value objects are going to be refactored in next step, this can be a subject of discussion.

agree, only one description.

Fine, but technically it is still the Seq of values, which means that the way how the translations/internationalisation is implemented is wrong. Nothing stops user from adding more than one description in the same language.

dsp-value-objects/src/main/scala/dsp/valueobjects/Iri.scala

irinaschubert · 2022-06-01T07:51:59Z

dsp-value-objects/src/main/scala/dsp/valueobjects/List.scala

+  /**
+   * List Position value object.
+   */
+  sealed abstract case class Position private (value: Int)


Suggested change

sealed abstract case class Position private (value: Int)

sealed abstract case class ListPosition private (value: Int)

irinaschubert · 2022-06-01T07:52:11Z

dsp-value-objects/src/main/scala/dsp/valueobjects/List.scala

+  /**
+   * List Labels value object.
+   */
+  sealed abstract case class Labels private (value: Seq[V2.StringLiteralV2])


Suggested change

sealed abstract case class Labels private (value: Seq[V2.StringLiteralV2])

sealed abstract case class ListLabels private (value: Seq[V2.StringLiteralV2])

irinaschubert · 2022-06-01T07:52:54Z

dsp-value-objects/src/main/scala/dsp/valueobjects/List.scala

+  }
+
+  /**
+   * List Labels value object.


The same thing I mentioned for description. Shouldn't this be singular because there is only one label (just in different languages)?

dsp-value-objects/src/test/scala/dsp/valueobjects/GroupSpec.scala

dsp-value-objects/src/test/scala/dsp/valueobjects/IriSpec.scala

webapi/src/main/scala/org/knora/webapi/http/handler/KnoraExceptionHandler.scala

webapi/src/main/scala/org/knora/webapi/responders/admin/ListsResponderADM.scala

subotic · 2022-06-01T13:54:06Z

dsp-value-objects/src/main/scala/dsp/valueobjects/Group.scala

+  }
+
+  /**
+   * GroupDescriptions value object.


agree, only one description.

subotic · 2022-06-01T13:57:06Z

dsp-value-objects/src/main/scala/dsp/valueobjects/Group.scala

+
+import zio.prelude.Validation
+
+sealed trait Group


we don't need to define a trait for Group. We only discussed it in the context of IRI.

subotic · 2022-06-01T14:00:07Z

dsp-value-objects/src/main/scala/dsp/valueobjects/Iri.scala

+  /**
+   * GroupIri value object.
+   */
+  sealed abstract case class GroupIri private (value: String)


all IRI case classes need to extend the IRI sealed trait, or it will not work as intended.

subotic · 2022-06-01T14:11:12Z

dsp-value-objects/src/main/scala/dsp/valueobjects/Project.scala

+  /**
+   * Project Keywords value object.
+   */
+  sealed abstract case class Keywords private (value: Seq[String])


hmm, can be both. Either a Seq[Keyword] on the level of the entity, or a Seq[String] inside a Keywords value object. Maybe it is easier to go with the Seq[String], as this is what is coming through the API? It could even be a Set[String].

dsp-value-objects/src/main/scala/dsp/valueobjects/Project.scala

subotic · 2022-06-01T14:16:44Z

dsp-value-objects/src/main/scala/dsp/valueobjects/User.scala

+  /**
+   * SystemAdmin value object.
+   */
+  sealed abstract case class SystemAdmin private (value: Boolean)


yes, but this will probably be (re)moved. Permissions will be given through roles, and shouldn't be a flag on the user (in the future).

dsp-value-objects/src/test/scala/dsp/valueobjects/GroupSpec.scala

webapi/src/main/scala/org/knora/webapi/http/handler/KnoraExceptionHandler.scala

mpro7 · 2022-06-02T07:18:20Z

@BalduinLandolt @irinaschubert I only applied some of things you've pointed out. These which are related more to the next part, which would be the real refactor, should be also discussed first. The comments I think are important in the next step I left unresolved which should help to define the way how we shape value objects.

BalduinLandolt

I didn't go through it in extreme detail this time... but I'd say it's good now.

One thing though: Leaving conversations unresolved is maybe not the most persistent way of keeping track of what needs to be done in the future refactoring steps.
Would you mind creating a confluence page (or a Jira-Story, if you find that better) where you collect the main points that need to be addressed or discussed?

sonarcloud · 2022-06-02T08:03:24Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
24 Code Smells

No Coverage information
1.0% Duplication

mpro7 added 9 commits May 13, 2022 08:59

add new project and deps

0ca212a

move value objects to separate ptoject

aea668a

move & refactor error messages

c844fa0

copy temporarily + adjust v2 validation related stuff

e23f1e5

fix all errors but StringLiteralV2

d3f4761

fix StringLiteralV2 class related issues

a5c1265

fix admin tests

afc3830

move tests to new project

4586b24

rewrite tests to ZIO

47da536

mpro7 self-assigned this May 25, 2022

mpro7 added 4 commits May 30, 2022 08:54

Merge branch 'main' into DEV-615-add-value-objects-sbt-project

00f4cbc

fixt tests

2cebe77

refactor IRI type property names

6ec5708

cleanup

430caed

mpro7 changed the title ~~Dev 615 add value objects sbt project~~ refactor: move value objects to separate project (DEV-615) May 31, 2022

mpro7 marked this pull request as ready for review May 31, 2022 08:48

mpro7 requested review from BalduinLandolt, subotic and irinaschubert May 31, 2022 08:50

BalduinLandolt requested changes May 31, 2022

View reviewed changes

irinaschubert approved these changes Jun 1, 2022

View reviewed changes

review changes

20b65cf

mpro7 requested a review from BalduinLandolt June 1, 2022 13:04

subotic approved these changes Jun 1, 2022

View reviewed changes

more review changes

6bd4eb4

BalduinLandolt approved these changes Jun 2, 2022

View reviewed changes

even more review changes

43f4f46

mpro7 merged commit b55eb12 into main Jun 2, 2022

mpro7 deleted the DEV-615-add-value-objects-sbt-project branch June 2, 2022 08:50

refactor: move value objects to separate project (DEV-615) #2069

refactor: move value objects to separate project (DEV-615) #2069

mpro7 commented May 25, 2022 •

edited

BalduinLandolt left a comment

BalduinLandolt May 31, 2022

mpro7 Jun 1, 2022

BalduinLandolt May 31, 2022

mpro7 Jun 1, 2022

irinaschubert left a comment

irinaschubert Jun 1, 2022

mpro7 Jun 1, 2022

subotic Jun 1, 2022

mpro7 Jun 2, 2022

irinaschubert Jun 1, 2022

irinaschubert Jun 1, 2022

irinaschubert Jun 1, 2022

subotic Jun 1, 2022

subotic Jun 1, 2022

subotic Jun 1, 2022

subotic Jun 1, 2022

subotic Jun 1, 2022

mpro7 commented Jun 2, 2022 •

edited

BalduinLandolt left a comment

sonarcloud bot commented Jun 2, 2022

	sealed abstract case class Position private (value: Int)
	sealed abstract case class ListPosition private (value: Int)

	sealed abstract case class Labels private (value: Seq[V2.StringLiteralV2])
	sealed abstract case class ListLabels private (value: Seq[V2.StringLiteralV2])

refactor: move value objects to separate project (DEV-615) #2069

refactor: move value objects to separate project (DEV-615) #2069

Conversation

mpro7 commented May 25, 2022 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

BalduinLandolt 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

irinaschubert 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpro7 commented Jun 2, 2022 • edited

BalduinLandolt left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 2, 2022

mpro7 commented May 25, 2022 •

edited

mpro7 commented Jun 2, 2022 •

edited