-
Notifications
You must be signed in to change notification settings - Fork 29
Resolve symbolic links to mags and store them in db #8350
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
Changes from 14 commits
1bde0f7
47846d7
ea9fc8f
c4c95b7
66ddcf6
8a6cec9
68b1c1b
c72f048
cfd388b
9c7e8bd
d839176
3553612
e7772b2
e001ca1
9d43831
a47472f
91337f9
592e39d
9f87a5e
331b1b2
cf31e60
1edeee3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -21,6 +21,7 @@ import com.scalableminds.webknossos.datastore.models.datasource.{ | |||||
ThinPlateSplineCorrespondences, | ||||||
DataLayerLike => DataLayer | ||||||
} | ||||||
import com.scalableminds.webknossos.datastore.services.MagPathInfo | ||||||
import com.scalableminds.webknossos.schema.Tables._ | ||||||
import controllers.DatasetUpdateParameters | ||||||
|
||||||
|
@@ -29,6 +30,7 @@ import models.organization.OrganizationDAO | |||||
import play.api.i18n.{Messages, MessagesProvider} | ||||||
import play.api.libs.json._ | ||||||
import play.utils.UriEncoding | ||||||
import slick.dbio.DBIO | ||||||
import slick.jdbc.PostgresProfile.api._ | ||||||
import slick.jdbc.TransactionIsolation.Serializable | ||||||
import slick.lifted.Rep | ||||||
|
@@ -707,18 +709,45 @@ class DatasetDAO @Inject()(sqlClient: SqlClient, datasetLayerDAO: DatasetLayerDA | |||||
} | ||||||
} | ||||||
|
||||||
class DatasetMagsDAO @Inject()(sqlClient: SqlClient)(implicit ec: ExecutionContext) extends SimpleSQLDAO(sqlClient) { | ||||||
private def parseRow(row: DatasetMagsRow): Fox[Vec3Int] = | ||||||
case class DatasetMagInfo(datasetId: ObjectId, | ||||||
dataLayerName: String, | ||||||
mag: Vec3Int, | ||||||
path: String, | ||||||
realPath: String, | ||||||
hasLocalData: Boolean) | ||||||
|
||||||
object DatasetMagInfo { | ||||||
implicit val jsonFormat: Format[DatasetMagInfo] = Json.format[DatasetMagInfo] | ||||||
} | ||||||
class DatasetMagsDAO @Inject()(sqlClient: SqlClient)(implicit ec: ExecutionContext) | ||||||
extends SQLDAO[MagPathInfo, DatasetMagsRow, DatasetMags](sqlClient) { | ||||||
protected val collection = DatasetMags | ||||||
|
||||||
protected def idColumn(x: DatasetMags): Rep[String] = x._Dataset | ||||||
|
||||||
protected def isDeletedColumn(x: DatasetMags): Rep[Boolean] = false | ||||||
|
||||||
protected def parse(row: DatasetMagsRow): Fox[MagPathInfo] = | ||||||
for { | ||||||
mag <- Vec3Int.fromList(parseArrayLiteral(row.mag).map(_.toInt)) ?~> "could not parse mag" | ||||||
} yield | ||||||
MagPathInfo(row.datalayername, | ||||||
mag, | ||||||
row.path.getOrElse("uninitialized"), | ||||||
row.realpath.getOrElse("uninitialized"), | ||||||
hasLocalData = row.haslocaldata) | ||||||
|
||||||
private def parseMag(magArrayLiteral: String): Fox[Vec3Int] = | ||||||
for { | ||||||
mag <- Vec3Int.fromList(parseArrayLiteral(magArrayLiteral).map(_.toInt)) ?~> "could not parse mag" | ||||||
} yield mag | ||||||
|
||||||
def findMagForLayer(datasetId: ObjectId, dataLayerName: String): Fox[List[Vec3Int]] = | ||||||
for { | ||||||
rows <- run(DatasetMags.filter(r => r._Dataset === datasetId.id && r.datalayername === dataLayerName).result) | ||||||
.map(_.toList) | ||||||
rowsParsed <- Fox.combined(rows.map(parseRow)) ?~> "could not parse mag row" | ||||||
} yield rowsParsed | ||||||
mags <- Fox.combined(rows.map(r => parseMag(r.mag))) ?~> "could not parse mag row" | ||||||
} yield mags | ||||||
|
||||||
def updateMags(datasetId: ObjectId, dataLayersOpt: Option[List[DataLayer]]): Fox[Unit] = { | ||||||
val clearQuery = q"DELETE FROM webknossos.dataset_mags WHERE _dataset = $datasetId".asUpdate | ||||||
|
@@ -733,6 +762,53 @@ class DatasetMagsDAO @Inject()(sqlClient: SqlClient)(implicit ec: ExecutionConte | |||||
replaceSequentiallyAsTransaction(clearQuery, insertQueries) | ||||||
} | ||||||
|
||||||
def updateMagPathsForDataset(datasetId: ObjectId, magPaths: List[MagPathInfo]): Fox[Unit] = | ||||||
for { | ||||||
_ <- Fox.successful(()) | ||||||
updateQueries = magPaths.map(pathInfo => { | ||||||
|
updateQueries = magPaths.map(pathInfo => { | |
updateQueries = magPathInfos.map(magPathInfo => { |
(Seems cleaner to me if the variable names match, and also match the type. Or, for the inner-most, use an obvious abbreviation like m. That way the reader doesn’t have to understand that these things are all the same)
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please follow capitalization from schema.sql here, dataLayerName and hasLocalData
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use None for this case? I feel like having a custom string literal in the json could cause headaches down the line. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this to avoid having options in the DatasetMagInfo, since this is only a theoretical case either way. The path and realPath are set when the dataset is reported, so it would only be empty between the two report functions that are separate at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn’t call it a theoretical case, since there is a real time difference between the two report functions. That means that this value will, at least for short durations, occur in the database and downstream functions will have to be able to handle it. If a function lists all datasets that use the same layer, anad does string comparison, this would return all that have "uninitialized", which could become a nonsensical list. I’d rather have the Option if we cannot prevent that this value is ever inserted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,12 +14,13 @@ import com.scalableminds.webknossos.datastore.models.datasource.{ | |
DataLayerLike => DataLayer | ||
} | ||
import com.scalableminds.webknossos.datastore.rpc.RPC | ||
import com.scalableminds.webknossos.datastore.services.DataSourcePathInfo | ||
import com.typesafe.scalalogging.LazyLogging | ||
import models.folder.FolderDAO | ||
import models.organization.{Organization, OrganizationDAO} | ||
import models.team._ | ||
import models.user.{User, UserService} | ||
import net.liftweb.common.{Box, Full} | ||
import net.liftweb.common.{Box, Empty, EmptyBox, Full} | ||
import play.api.libs.json.{JsObject, Json} | ||
import security.RandomIDGenerator | ||
import utils.WkConf | ||
|
@@ -33,6 +34,7 @@ class DatasetService @Inject()(organizationDAO: OrganizationDAO, | |
dataStoreDAO: DataStoreDAO, | ||
datasetLastUsedTimesDAO: DatasetLastUsedTimesDAO, | ||
datasetDataLayerDAO: DatasetLayerDAO, | ||
datasetMagsDAO: DatasetMagsDAO, | ||
teamDAO: TeamDAO, | ||
folderDAO: FolderDAO, | ||
dataStoreService: DataStoreService, | ||
|
@@ -338,6 +340,35 @@ class DatasetService @Inject()(organizationDAO: OrganizationDAO, | |
_ <- datasetDAO.updateUploader(dataset._id, Some(_uploader)) ?~> "dataset.uploader.forbidden" | ||
} yield () | ||
|
||
private def updateRealPath(pathInfo: DataSourcePathInfo)(implicit ctx: DBAccessContext): Fox[Unit] = | ||
if (pathInfo.magPathInfos.isEmpty) { | ||
Fox.successful(()) | ||
} else { | ||
val dataset = datasetDAO.findOneByDataSourceId(pathInfo.dataSourceId).futureBox | ||
dataset.flatMap { | ||
case Full(dataset) => datasetMagsDAO.updateMagPathsForDataset(dataset._id, pathInfo.magPathInfos) | ||
case Empty => // Dataset reported but ignored (non-existing/forbidden org) | ||
Fox.successful(()) | ||
case e: EmptyBox => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh, TIL about the EmptyBox trait. Looks like we should have used this in a lot of places where we match against Failure (but miss ParamFailure?). But I guess we instantiate so few ParamFailures that this hasn’t caused any real problems. |
||
Fox.failure("dataset.notFound", e) | ||
} | ||
} | ||
|
||
def updateRealPaths(pathInfos: List[DataSourcePathInfo])(implicit ctx: DBAccessContext): Fox[Unit] = | ||
for { | ||
_ <- Fox.serialCombined(pathInfos)(updateRealPath) | ||
} yield () | ||
|
||
def getPathsForDatalayer(datasetId: ObjectId, layerName: String): Fox[List[(DatasetMagInfo, List[DatasetMagInfo])]] = | ||
|
||
for { | ||
magInfos <- datasetMagsDAO.findPathsForDatasetAndDatalayer(datasetId, layerName) | ||
magInfosAndLinkedMags <- Fox.serialCombined(magInfos)(magInfo => | ||
for { | ||
pathInfos <- datasetMagsDAO.findAllByRealPath(magInfo.realPath) | ||
} yield (magInfo, pathInfos.filter(!_.equals(magInfo)))) | ||
|
||
} yield magInfosAndLinkedMags | ||
|
||
|
||
def publicWrites(dataset: Dataset, | ||
requestingUserOpt: Option[User], | ||
organization: Option[Organization] = None, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
START TRANSACTION; | ||
|
||
do $$ begin ASSERT (select schemaVersion from webknossos.releaseInformation) = 125, 'Previous schema version mismatch'; end; $$ LANGUAGE plpgsql; | ||
|
||
ALTER TABLE webknossos.dataset_mags ADD COLUMN realPath TEXT; | ||
ALTER TABLE webknossos.dataset_mags ADD COLUMN path TEXT; | ||
ALTER TABLE webknossos.dataset_mags ADD COLUMN hasLocalData BOOLEAN NOT NULL DEFAULT false; | ||
|
||
UPDATE webknossos.releaseInformation SET schemaVersion = 126; | ||
|
||
|
||
COMMIT TRANSACTION; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
START TRANSACTION; | ||
|
||
do $$ begin ASSERT (select schemaVersion from webknossos.releaseInformation) = 126, 'Previous schema version mismatch'; end; $$ LANGUAGE plpgsql; | ||
|
||
ALTER TABLE webknossos.dataset_mags DROP COLUMN realPath; | ||
ALTER TABLE webknossos.dataset_mags DROP COLUMN path; | ||
ALTER TABLE webknossos.dataset_mags DROP COLUMN hasLocalData; | ||
|
||
UPDATE webknossos.releaseInformation SET schemaVersion = 125; | ||
|
||
COMMIT TRANSACTION; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,26 +1,26 @@ | ||
_dataSet,dataLayerName,mag,scale | ||
'59e9cfbdba632ac2ab8b23b3','color_1','(1,1,1)' | ||
'59e9cfbdba632ac2ab8b23b3','color_1','(2,2,2)' | ||
'59e9cfbdba632ac2ab8b23b3','color_1','(4,4,4)' | ||
'59e9cfbdba632ac2ab8b23b3','color_1','(8,8,8)' | ||
'59e9cfbdba632ac2ab8b23b3','color_1','(16,16,16)' | ||
'59e9cfbdba632ac2ab8b23b3','color_2','(1,1,1)' | ||
'59e9cfbdba632ac2ab8b23b3','color_2','(2,2,2)' | ||
'59e9cfbdba632ac2ab8b23b3','color_2','(4,4,4)' | ||
'59e9cfbdba632ac2ab8b23b3','color_2','(8,8,8)' | ||
'59e9cfbdba632ac2ab8b23b3','color_2','(16,16,16)' | ||
'59e9cfbdba632ac2ab8b23b3','color_3','(1,1,1)' | ||
'59e9cfbdba632ac2ab8b23b3','color_3','(2,2,2)' | ||
'59e9cfbdba632ac2ab8b23b3','color_3','(4,4,4)' | ||
'59e9cfbdba632ac2ab8b23b3','color_3','(8,8,8)' | ||
'59e9cfbdba632ac2ab8b23b3','color_3','(16,16,16)' | ||
'59e9cfbdba632ac2ab8b23b5','color','(1,1,1)' | ||
'59e9cfbdba632ac2ab8b23b5','color','(2,2,1)' | ||
'59e9cfbdba632ac2ab8b23b5','color','(4,4,1)' | ||
'59e9cfbdba632ac2ab8b23b5','color','(8,8,2)' | ||
'59e9cfbdba632ac2ab8b23b5','color','(16,16,4)' | ||
'59e9cfbdba632ac2ab8b23b5','segmentation','(1,1,1)' | ||
'59e9cfbdba632ac2ab8b23b5','segmentation','(2,2,1)' | ||
'59e9cfbdba632ac2ab8b23b5','segmentation','(4,4,1)' | ||
'59e9cfbdba632ac2ab8b23b5','segmentation','(8,8,2)' | ||
'59e9cfbdba632ac2ab8b23b5','segmentation','(16,16,4)' | ||
_dataSet,dataLayerName,mag,scale,path,realPath,hasLocalData | ||
'59e9cfbdba632ac2ab8b23b3','color_1','(1,1,1)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b3','color_1','(2,2,2)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b3','color_1','(4,4,4)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b3','color_1','(8,8,8)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b3','color_1','(16,16,16)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b3','color_2','(1,1,1)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b3','color_2','(2,2,2)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b3','color_2','(4,4,4)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b3','color_2','(8,8,8)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b3','color_2','(16,16,16)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b3','color_3','(1,1,1)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b3','color_3','(2,2,2)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b3','color_3','(4,4,4)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b3','color_3','(8,8,8)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b3','color_3','(16,16,16)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b5','color','(1,1,1)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b5','color','(2,2,1)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b5','color','(4,4,1)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b5','color','(8,8,2)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b5','color','(16,16,4)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b5','segmentation','(1,1,1)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b5','segmentation','(2,2,1)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b5','segmentation','(4,4,1)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b5','segmentation','(8,8,2)','uninitialized','uninitialized',false | ||
'59e9cfbdba632ac2ab8b23b5','segmentation','(16,16,4)','uninitialized','uninitialized',false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason to validate explicitly and log errors here, rather than using a playBodyParser to build an
Action[List[DataSourcePathInfo]]
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that was just copied from the function above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So should this be changed for all methods in this controller?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it for updateAll in #8402 – but I’d say it’s not super important to update this now. Both is valid. The older variant has the benefit of controller-side logging (but I don’t think I ever saw this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #8406 for that.