EPMLSTRCMW-311 Make All Id Types Wrapped #237

ForeverRainmaN · 2022-02-15T18:04:12Z

Wrapped id's into a Wrapped trait
Refactored test code according to new changes

GrigoriiBerezin

Good job. But there are few moments to discuss

GrigoriiBerezin · 2022-02-22T08:53:50Z

controllers/src/main/scala/cromwell/pipeline/controller/utils/FromStringUnmarshallers.scala

+      ProjectId.from(projectId) match {
+        case Validated.Valid(content)  => content
+        case Validated.Invalid(errors) => throw new RuntimeException(errors.head)
+      }


Looks like it has the same behavior as apply method, you can use ProjectId(projectId, Enable.Unsafe)

GrigoriiBerezin · 2022-02-22T08:55:25Z

controllers/src/main/scala/cromwell/pipeline/controller/utils/PathMatchers.scala

 import cromwell.pipeline.model.wrapper

 import java.nio.file.{ Path, Paths }

 object PathMatchers {
-  val ProjectId: PathMatcher1[dto.ProjectId] = Segment.map(dto.ProjectId(_))
+  val ProjectId: PathMatcher1[wrapper.ProjectId] = Segment.flatMap(wrapper.ProjectId.from(_).toOption)


We should think of generic way implementation to handle all wrappers to PathMatcher, but need to discuss with @myazinn

I agree. It should relatively be easy to do.

GrigoriiBerezin · 2022-02-22T08:58:54Z

model/src/main/scala/cromwell/pipeline/model/wrapper/ProjectConfigurationId.scala

+  override type Type = String
+  override type Wrapper = ProjectConfigurationId
+  override type Error = String


[OPT] It's not an override actually, key word override should be removed

GrigoriiBerezin · 2022-02-22T08:59:33Z

model/src/main/scala/cromwell/pipeline/model/wrapper/ProjectConfigurationId.scala

+
+  val pattern: String = "^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$"
+
+  def randomId: ProjectConfigurationId = new ProjectConfigurationId(UUID.randomUUID().toString)


Do we need this method? I see that we using it only in one place for creating dummy instance

GrigoriiBerezin · 2022-02-22T09:05:26Z

model/src/main/scala/cromwell/pipeline/model/wrapper/RepositoryId.scala

+
+  implicit lazy val repositoryIdFormat: Format[RepositoryId] = wrapperFormat
+
+  val pattern = "^[0-9]+$"


GrigoriiBerezin · 2022-02-22T09:06:12Z

repositories/src/main/scala/cromwell/pipeline/datastorage/dao/entry/RunEntry.scala

+import java.time.Instant
+


Why was this import moved?

codecov-commenter · 2022-03-01T08:23:05Z

Codecov Report

Merging #237 (c706125) into develop (ac85bbc) will decrease coverage by 0.20%.
The diff coverage is 78.57%.

@@             Coverage Diff             @@
##           develop     #237      +/-   ##
===========================================
- Coverage    89.95%   89.74%   -0.21%     
===========================================
  Files           54       54              
  Lines          876      878       +2     
  Branches         6        5       -1     
===========================================
  Hits           788      788              
- Misses          88       90       +2

Impacted Files	Coverage Δ
...ine/controller/utils/FromStringUnmarshallers.scala	`66.66% <0.00%> (ø)`
.../pipeline/datastorage/dao/entry/ProjectEntry.scala	`95.00% <ø> (ø)`
...well/pipeline/datastorage/dao/entry/RunEntry.scala	`92.30% <ø> (ø)`
...ao/repository/ProjectConfigurationRepository.scala	`100.00% <ø> (ø)`
...datastorage/dao/repository/ProjectRepository.scala	`100.00% <ø> (ø)`
...ine/datastorage/dao/repository/RunRepository.scala	`100.00% <ø> (ø)`
...la/cromwell/pipeline/datastorage/dto/Project.scala	`77.77% <ø> (ø)`
...ipeline/datastorage/dto/ProjectConfiguration.scala	`63.63% <ø> (-3.04%)`	⬇️
.../scala/cromwell/pipeline/datastorage/dto/Run.scala	`50.00% <ø> (ø)`
...cromwell/pipeline/service/AggregationService.scala	`100.00% <ø> (ø)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac85bbc...c706125. Read the comment docs.

myazinn · 2022-03-17T12:10:08Z

services/src/test/scala/cromwell/pipeline/service/ProjectServiceTest.scala

@@ -106,7 +107,7 @@ class ProjectServiceTest extends AsyncWordSpec with Matchers with MockitoSugar {
      }

      "return none if project not found" taggedAs Service in {
-        val projectId = ProjectId("projectId")
+        val projectId = ProjectId.random


IMO it is a bit inconsistent that you are removing id = ProjectConfigurationId.randomId in ProjectConfigurationServiceTestImpl while adding it here. We should stick to either of this approach, but not change them back and forth (randomId looks better IMO)

myazinn · 2022-03-17T12:13:28Z

repositories/src/main/scala/cromwell/pipeline/datastorage/dto/Project.scala

@@ -41,25 +42,12 @@ final case class LocalProject(
    )
 }

-final case class PostProject(name: String)
+final case class PostProject(name: ProjectId)


This is an incorrect change. Project name is a human-friendly thing (such as "My pet project"), this is not a projectId that we control and must be unique.

myazinn · 2022-03-17T12:14:46Z

model/src/main/scala/cromwell/pipeline/model/wrapper/RunId.scala

@@ -12,7 +10,6 @@ object RunId extends Wrapped.Companion {
  type Wrapper = RunId
  type Error = String
  val pattern: String = "^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$"
-  implicit lazy val runIdFormat: Format[RunId] = wrapperFormat


Is this change required for your MR?

ForeverRainmaN force-pushed the EPMLSTRCMW-311-Make-All-Id-Types-Wrapped branch from 60f393f to 447c854 Compare February 21, 2022 11:21

ForeverRainmaN changed the title ~~refactor: wrapped id cc into Wrapper with companion objects~~ EPMLSTRCMW-311 Make All Id Types Wrapped Feb 21, 2022

ForeverRainmaN requested a review from myazinn February 21, 2022 11:23

ForeverRainmaN changed the title ~~EPMLSTRCMW-311 Make All Id Types Wrapped~~ EPMLSTRCMW-311-Make All Id Types Wrapped Feb 21, 2022

ForeverRainmaN requested review from GrigoriiBerezin and Dmitrii-Shevchenko February 21, 2022 11:24

ForeverRainmaN force-pushed the EPMLSTRCMW-311-Make-All-Id-Types-Wrapped branch from 447c854 to 0396ecc Compare February 21, 2022 11:28

ForeverRainmaN changed the title ~~EPMLSTRCMW-311-Make All Id Types Wrapped~~ EPMLSTRCMW-311 Make All Id Types Wrapped Feb 21, 2022

ForeverRainmaN force-pushed the EPMLSTRCMW-311-Make-All-Id-Types-Wrapped branch from 0396ecc to 4d6399d Compare February 21, 2022 11:43

GrigoriiBerezin requested changes Feb 22, 2022

View reviewed changes

ForeverRainmaN force-pushed the EPMLSTRCMW-311-Make-All-Id-Types-Wrapped branch from 4d6399d to d171b1a Compare March 1, 2022 08:22

ForeverRainmaN force-pushed the EPMLSTRCMW-311-Make-All-Id-Types-Wrapped branch 4 times, most recently from 2f628d2 to 55713d2 Compare March 1, 2022 11:37

refactor: Wrapped id cc into Wrapper with companion objects

c706125

ForeverRainmaN force-pushed the EPMLSTRCMW-311-Make-All-Id-Types-Wrapped branch from 55713d2 to c706125 Compare March 1, 2022 11:46

ForeverRainmaN force-pushed the EPMLSTRCMW-311-Make-All-Id-Types-Wrapped branch from 0646ebf to c706125 Compare March 16, 2022 09:38

myazinn suggested changes Mar 17, 2022

View reviewed changes

Dmitrii-Shevchenko approved these changes Mar 21, 2022

View reviewed changes

GrigoriiBerezin approved these changes Mar 24, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EPMLSTRCMW-311 Make All Id Types Wrapped #237

EPMLSTRCMW-311 Make All Id Types Wrapped #237

ForeverRainmaN commented Feb 15, 2022 •

edited

Loading

GrigoriiBerezin left a comment

GrigoriiBerezin Feb 22, 2022

GrigoriiBerezin Feb 22, 2022

myazinn Feb 22, 2022

GrigoriiBerezin Feb 22, 2022

GrigoriiBerezin Feb 22, 2022

GrigoriiBerezin Feb 22, 2022

GrigoriiBerezin Feb 22, 2022

codecov-commenter commented Mar 1, 2022 •

edited

Loading

myazinn Mar 17, 2022

myazinn Mar 17, 2022

myazinn Mar 17, 2022


		val pattern: String = "^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$"

		def randomId: ProjectConfigurationId = new ProjectConfigurationId(UUID.randomUUID().toString)


		implicit lazy val repositoryIdFormat: Format[RepositoryId] = wrapperFormat

		val pattern = "^[0-9]+$"

EPMLSTRCMW-311 Make All Id Types Wrapped #237

Are you sure you want to change the base?

EPMLSTRCMW-311 Make All Id Types Wrapped #237

Conversation

ForeverRainmaN commented Feb 15, 2022 • edited Loading

GrigoriiBerezin 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

codecov-commenter commented Mar 1, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ForeverRainmaN commented Feb 15, 2022 •

edited

Loading

codecov-commenter commented Mar 1, 2022 •

edited

Loading