WIP : begin working on gridfs implementation by nicolasdejonghe · Pull Request #29 · zengularity/benji

nicolasdejonghe · 2019-05-28T10:01:01Z

Pull Request Checklist

Have you read through the contributor guidelines?
Have you updated the documentation?
Have you added tests for any changed functionality?

Fixes

Fixes #xxxx

Purpose

What does this PR do?

Background Context

Why did you take this approach?

References

Are there any relevant issues / PRs / mailing lists discussions?

cchantep · 2019-05-28T11:57:41Z

.gitignore

+vfs/src/test/resources/gcs-test.json
+vfs/src/test/resources/local.conf
+s3/src/test/resources/local.conf
+.metals


Ce genre de fichier dépendant de l'env de dev/préférences développeur ne doit pas se retrouver ici, mais dans le .git/info/exclude, le .gitignore doit rester commun/agnostique de ce genre de chose

build.sbt

cchantep · 2019-06-03T09:10:53Z

build.sbt

+    },
+    libraryDependencies ++= Seq(
+      "org.reactivemongo" %% "reactivemongo" % "0.1x",
+      "com.typesafe.play" %% "play-json" % playVer.value,


Est-ce qu'on a besoin de cette dépendance ?

cchantep · 2019-06-03T09:10:59Z

build.sbt

+      "org.reactivemongo" %% "reactivemongo" % "0.1x",
+      "com.typesafe.play" %% "play-json" % playVer.value,
+      Dependencies.slf4jApi,
+      "commons-io" % "commons-io" % "2.4" % Test)


cchantep · 2019-06-03T11:25:25Z

build.sbt

-      "com.typesafe.play" %% "play-json" % playVer.value,
-      Dependencies.slf4jApi,
-      "commons-io" % "commons-io" % "2.4" % Test)
+      "org.reactivemongo" %% "reactivemongo" % "0.17"


Ne correspond pas à une version existante : https://search.maven.org/ ou http://reactivemongo.org/

cchantep · 2019-06-06T17:45:38Z

build.sbt

  settings(Common.settings: _*).settings(
    name := "benji-gridfs",
    libraryDependencies ++= Seq(
+      Dependencies.slf4jApi,


je ne pense pas que c'est nécessaire

cchantep · 2019-06-06T17:47:06Z

gridfs/src/main/scala/GridFSTransport.scala

+
+final class GridFSTransport() {
+
+  def apply(uri: String, connectionOptions: MongoConnectionOptions) {


Type de retour ? Pour savoir comment créer il faut savoir ce qu'on veut créer et sur quoi ça dépend

cchantep · 2019-06-06T17:51:04Z

gridfs/src/main/scala/GridFSTransport.scala

+    //c'est l'idée, pour l'instant je tatonne encore sur comment récupérer la db de la connexion pour la passer à GridFS(...)
+    MongoConnection.parseURI(uri).map { parsedUri =>
+      val connection = driver.connection(parsedUri, options = connectionOptions)
+      GridFS[BSONSerializationPack.type](conn, "fs")


prefix en paramètre

le transport et même derrière storage et BucketRef et ObjectRef doivent resource la DB au moment des appels/opérations (à partir du nom de DB dans l'URI)

cchantep · 2019-06-12T10:54:34Z

gridfs/src/main/scala/GridFSTransport.scala

 import reactivemongo.api.gridfs.GridFS
-import reactivemongo.api.{ BSONSerializationPack, MongoConnectionOptions }
+import reactivemongo.api.BSONSerializationPack
+import scala.concurrent.ExecutionContext.Implicits.global


On n'utilise pas le contexte global, sauf pour des tests (et encore)

cchantep · 2019-06-12T10:56:59Z

gridfs/src/main/scala/GridFSTransport.scala

+    val gridfsdb = for {
+      mongoUri <- Future.fromTry(MongoConnection.parseURI(uri))
+      con = driver.connection(mongoUri)
+      dn <- Future(mongoUri.db.get)


Option.get est un code smell/bas practice

de la même façon qu'il ne faut pas essayer de sortir d'une Future on ne peut pas ignore le contexte optionnelle d'une valeur, il faut travailler avec/dedans (le .get n'aurait jamais dû être exposé dans l'API).

cchantep · 2019-06-12T10:58:52Z

gridfs/src/main/scala/GridFSFactory.scala

+ */
+final class GridFSFactory extends StorageFactory {
+  @SuppressWarnings(Array("org.wartremover.warts.TryPartial"))
+  def apply(injector: Injector, uri: URI): ObjectStorage = {


Copy paste ?

yep, je ne me suis pas encore penché là dessus

cchantep · 2019-06-12T15:31:30Z

gridfs/src/main/scala/GridFSTransport.scala

 import reactivemongo.api.gridfs.GridFS
 import reactivemongo.api.BSONSerializationPack
-import scala.concurrent.ExecutionContext.Implicits.global
+import scala.concurrent._


On évite les import en wildcard, qui peuvent cacher des imports dans la scope implicit, par ailleurs il y a une ligne en dessous pour ce package

cchantep · 2019-06-12T15:33:27Z

gridfs/src/main/scala/GridFSTransport.scala

+        case Some(name) =>
+          con.database(name).map(db =>
+            GridFS[BSONSerializationPack.type](db, prefix))
+        case None => Future.failed(new RuntimeException(s"Couldn't get the db from $mongoUri"))


Si possible utiliser les types d'Exception Benji qui sont dans core

cchantep · 2019-06-13T17:47:55Z

gridfs/src/main/scala/GridFSTransport.scala

+  def apply(uri: String, prefix: String)(implicit ec: ExecutionContext): GridFSTransport = {
+    val driver = new reactivemongo.api.MongoDriver
+
+    val gridfsdb = for {


Il ne faut pas assigner une Future issue de .database(..) (résolution de la DB depuis poil):

It’s generally a good practice not to assign the database and collection references to val (even to lazy val), as it’s better to get a fresh reference each time, to automatically recover from any previous issues (e.g. network failure).
http://reactivemongo.org/releases/0.1x/documentation/tutorial/connect-database.html

cchantep · 2019-06-18T08:21:28Z

gridfs/src/main/scala/GridFSTransport.scala

+import scala.util.{ Failure, Success, Try }

 import com.zengularity.benji.exception.BenjiUnknownError
+import scala.util.Success


Double import

cchantep · 2019-06-18T08:26:06Z

gridfs/src/main/scala/GridFSTransport.scala

+
+import reactivemongo.api.gridfs.GridFS
+import reactivemongo.api.BSONSerializationPack
+import scala.concurrent.{ ExecutionContext, Future }


Sur la forme, J'essaie de me tenir à un ordre pour les imports, en groupant d'abord les importe JVM/Java classique, après les imports scalalib (import scala...), les imports libs externes (ici reactivemongo), les imports internes/cross pkg (com.zengularity.benji...).

Adaptation des règles javastyle, c'est configurable dans Intellij (Order import)

cchantep · 2019-06-18T08:33:42Z

gridfs/src/main/scala/GridFSTransport.scala

+  def gridfs(prefix: String)(implicit ec: ExecutionContext): Future[GridFS[BSONSerializationPack.type]] = {
+    mongoUri.db match {
+      case Some(name) =>
+        for {


for est utile avec plus d'un <- (map), sinon un .map (plus explicite et concis dans ce cas)

cchantep · 2019-06-18T08:34:12Z

gridfs/src/main/scala/GridFSTransport.scala

+      case Some(name) =>
+        for {
+          db <- connection.database(name)
+          gridfs = GridFS[BSONSerializationPack.type](db, prefix)


Quelle différence entre avoir cette ligne et avoir yield GridFS[BSONSerializationPack.type](db, prefix) ?

cchantep · 2019-06-18T08:35:17Z

gridfs/src/main/scala/GridFSTransport.scala

+        } yield gridfs
+
+      case None =>
+        Future.failed(new BenjiUnknownError(s"Couldn't get the db from $mongoUri"))


La formulation actuelle des erreurs est plutôt "Fails to ..."

cchantep · 2019-06-18T08:37:18Z

gridfs/src/main/scala/GridFSTransport.scala

+      mongoUri <- MongoConnection.parseURI(uri)
+      con = driver.connection(mongoUri)
+    } yield (mongoUri, con)
+    val (mongoUri, connection) = (res.map(_._1), res.map(_._2))


Pourquoi sortir du for à partir de là ?

cchantep · 2019-06-18T08:38:55Z

gridfs/src/main/scala/GridFSTransport.scala

+    val (mongoUri, connection) = (res.map(_._1), res.map(_._2))
+    connection match {
+      case Success(connection) => new Success(GridFSTransport(driver, connection, mongoUri))
+      case Failure(_) => Failure[GridFSTransport](new BenjiUnknownError(s"Couldn't create the connection to $uri"))


Le détails de l'erreur/root cause est perdu (cf https://github.com/zengularity/benji/blob/master/core/src/main/scala/exception/BenjiException.scala#L17 )

cchantep · 2019-06-18T08:39:17Z

gridfs/src/main/scala/GridsFSStorage.scala

+package com.zengularity.benji.gridfs
+
+import com.zengularity.benji.{ Bucket, ObjectStorage }
+import scala.concurrent.ExecutionContext.Implicits.global


cchantep · 2019-06-18T08:41:16Z

gridfs/src/main/scala/GridFSTransport.scala

+    } yield (mongoUri, con)
+    val (mongoUri, connection) = (res.map(_._1), res.map(_._2))
+    connection match {
+      case Success(connection) => new Success(GridFSTransport(driver, connection, mongoUri))


Success est une case class, pas de new (par ailleurs même pour les class on tend à fournir une factory/apply dans companion pour découpler l'init)

cchantep · 2019-06-18T08:41:51Z

gridfs/src/main/scala/GridFSTransport.scala

@@ -0,0 +1,49 @@
+// /*


cchantep · 2019-06-18T08:52:19Z

gridfs/src/main/scala/GridFSTransport.scala

+    val (mongoUri, connection) = (res.map(_._1), res.map(_._2))
+    connection match {
+      case Success(connection) => new Success(GridFSTransport(driver, connection, mongoUri))
+      case Failure(_) => Failure[GridFSTransport](new BenjiUnknownError(s"Couldn't create the connection to $uri"))


Formulation message "Fails to ..."

cchantep · 2019-06-18T10:11:47Z

gridfs/src/main/scala/GridFSTransport.scala

+import scala.concurrent.{ ExecutionContext, Future }
+import scala.util.{ Failure, Success, Try }

+import reactivemongo.api.MongoConnection


Il y a un import reactivemongo.api.X plus bas => import reactivemongo.api.{ MongoConnection, X}

cchantep · 2019-06-18T10:13:54Z

gridfs/src/main/scala/GridFSTransport.scala

+    } yield (con, mongoUri)
+
+    res match {
+      case Success((connection, mongoUri)) => Success(new GridFSTransport(driver, connection, mongoUri))


Pourquoi sortir le case Success du yield précédent (ce qui au passage instancie/alloue un Tuple2 intermédiaire, même si ça n'est pas grave, est-ce utile ?)

cchantep · 2019-06-18T12:59:19Z

gridfs/src/main/scala/GridFSTransport.scala

-    res match {
-      case Success((connection, mongoUri)) => Success(new GridFSTransport(driver, connection, mongoUri))
-      case Failure(error) => Failure[GridFSTransport](new BenjiUnknownError(s"Fails to create the connection to $uri", Some(error)))
+    res.recoverWith {


recoverWith vs recover

cchantep · 2019-06-20T18:21:45Z

gridfs/src/main/scala/GridFSBucketRef.scala

+
+    filesAndChunksStats.transform {
+      case Success(_) => Success(true)
+      case Failure(_) => Success(false)


Une storage.transport.gridfs(name) ne devrait pas se retrouver ici

begin working on gridfs implementation

6aed298

cchantep reviewed May 28, 2019

View reviewed changes

nicolasdejonghe added 2 commits May 29, 2019 11:26

create folder architecture

daff698

added factory for gridfs

2711ffb

cchantep reviewed Jun 3, 2019

View reviewed changes

remove useless dependencies from gridfs in buil.sbt

c6b1e2b

cchantep reviewed Jun 3, 2019

View reviewed changes

nicolasdejonghe added 2 commits June 3, 2019 13:37

mongoreactive 0.17 => mongoreactive 0.17.0

617ecd3

WIP on gridfstransport

667e18e

cchantep reviewed Jun 6, 2019

View reviewed changes

nicolasdejonghe added 2 commits June 11, 2019 14:29

added connection to gridfs db

954ad72

wip on gridfstransport

661d443

cchantep reviewed Jun 12, 2019

View reviewed changes

work with option in gridfstransport

da6ee9a

cchantep reviewed Jun 12, 2019

View reviewed changes

nicolasdejonghe added 2 commits June 13, 2019 10:11

fix import

7c29a2e

change exception type

c656a44

cchantep reviewed Jun 13, 2019

View reviewed changes

WIP refacto

2706535

cchantep reviewed Jun 18, 2019

View reviewed changes

nicolasdejonghe added 2 commits June 18, 2019 11:53

remove global execution context

fa86969

refacto

4a6e9d7

cchantep reviewed Jun 18, 2019

View reviewed changes

refacto

b6ee310

cchantep reviewed Jun 18, 2019

View reviewed changes

nicolasdejonghe added 2 commits June 20, 2019 16:36

WIP GridfsBucketRef: created exists function

6b4aee1

WIP GridFSBucketRef: created create function

bf33963

cchantep reviewed Jun 20, 2019

View reviewed changes

cchantep force-pushed the master branch from 369c153 to cfc1391 Compare April 13, 2020 19:03


		final class GridFSTransport() {

		def apply(uri: String, connectionOptions: MongoConnectionOptions) {

Conversation

nicolasdejonghe commented May 28, 2019

Pull Request Checklist

Fixes

Purpose

Background Context

References

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants