Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 16 commits into from Jun 2, 2022

Conversation

mpro7
Copy link
Collaborator

@mpro7 mpro7 commented May 25, 2022

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?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

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

@mpro7 mpro7 self-assigned this May 25, 2022
@mpro7 mpro7 changed the title Dev 615 add value objects sbt project refactor: move value objects to separate project (DEV-615) May 31, 2022
@mpro7 mpro7 marked this pull request as ready for review May 31, 2022 08:48
Copy link
Collaborator

@BalduinLandolt BalduinLandolt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. as far as I remember, multiple assertTrue() statements must be combined with && otherwise only the last one is actually tested.
  2. 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

Comment on lines 19 to 33
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(_) {})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 48 to 67
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(_) {})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :) )

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

project/Dependencies.scala Show resolved Hide resolved
Copy link

@irinaschubert irinaschubert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

}

/**
* GroupDescriptions value object.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, only one description.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
* List Labels value object.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

/**
* GroupDescriptions value object.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, only one description.


import zio.prelude.Validation

sealed trait Group
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
* GroupIri value object.
*/
sealed abstract case class GroupIri private (value: String)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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].

/**
* SystemAdmin value object.
*/
sealed abstract case class SystemAdmin private (value: Boolean)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@mpro7
Copy link
Collaborator Author

mpro7 commented Jun 2, 2022

@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.

Copy link
Collaborator

@BalduinLandolt BalduinLandolt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

sonarcloud bot commented Jun 2, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 24 Code Smells

No Coverage information No Coverage information
1.0% 1.0% Duplication

@mpro7 mpro7 merged commit b55eb12 into main Jun 2, 2022
@mpro7 mpro7 deleted the DEV-615-add-value-objects-sbt-project branch June 2, 2022 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants