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

fix dataset upload if there's only one layer #2840

Merged
merged 10 commits into from Jul 9, 2018

Conversation

@youri-k
Copy link
Contributor

commented Jul 2, 2018

  • upload of a dataset with one layer now works
  • previously the layer folder got truncated as a common prefix

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • upload a dataset zip which only contains one layer and no properties.json

Issues:


  • Ready for review

@youri-k youri-k self-assigned this Jul 2, 2018

@youri-k youri-k requested review from jstriebel and rschwanhold Jul 2, 2018

import collection.JavaConverters._
val zipEntries = zip.entries.asScala.filter(e => !e.isDirectory && (includeHiddenFiles || !isFileHidden(e))).toList

//color, mask, segmentation

This comment has been minimized.

Copy link
@jstriebel

jstriebel Jul 2, 2018

Contributor

Please expand this comment to explain why this is needed.

@@ -100,11 +100,24 @@ object ZipIO {

def isFileHidden(e: ZipEntry): Boolean = new File(e.getName).isHidden || e.getName.startsWith("__MACOSX")

def stripPathFrom(path: Path, string: String): Option[String] = {
if(path.toString.contains(string))

This comment has been minimized.

Copy link
@jstriebel

jstriebel Jul 2, 2018

Contributor

Could this directly operate on the path?

This comment has been minimized.

Copy link
@jstriebel

jstriebel Jul 2, 2018

Contributor

Could it happen that string is just a substring of a directory? That looks dangerous to me then.

This comment has been minimized.

Copy link
@youri-k

youri-k Jul 5, 2018

Author Contributor

You're right I'm looking into it

val commonPrefix = if (truncateCommonPrefix) {
PathUtils.commonPrefix(zipEntries.map(e => Paths.get(e.getName)))
val commonPrefixNotFixed = PathUtils.commonPrefix(zipEntries.map(e => Paths.get(e.getName)))
val strippedPaths = List("color", "mask", "segmentation").flatMap(stripPathFrom(commonPrefixNotFixed, _))

This comment has been minimized.

Copy link
@jstriebel

jstriebel Jul 2, 2018

Contributor

Why is it correct to strip either color or mask or segmentation? Why never all?

This comment has been minimized.

Copy link
@youri-k

youri-k Jul 5, 2018

Author Contributor

Because a dataLayer only has one category it belongs to.
Norman just confirmed this

This comment has been minimized.

Copy link
@jstriebel

jstriebel Jul 5, 2018

Contributor

Ok, perfect. Maybe add a comment about it then, it's not super obvious at first sight.

else
None
def stripPathFrom(path: Path, string: String): Option[Path] = {
for (i <- 0 until path.getNameCount) {

This comment has been minimized.

Copy link
@jstriebel

jstriebel Jul 5, 2018

Contributor

I would prefer this to be recursive, but I think it's still fine

This comment has been minimized.

Copy link
@youri-k

youri-k Jul 5, 2018

Author Contributor

Sounds good I'll do it later

def stripPathFrom(path: Path, string: String): Option[Path] = {
for (i <- 0 until path.getNameCount) {

if (path.subpath(i, i + 1).toString.contains(string))

This comment has been minimized.

Copy link
@jstriebel

jstriebel Jul 5, 2018

Contributor

Couldn't we use == string now?

This comment has been minimized.

Copy link
@youri-k

youri-k Jul 5, 2018

Author Contributor

No because if there are multiple layers of the same category which are named e.g. color_1, color_2...
So in the case that one of those layers gets copied and is the only layer in the new dataset, this wouldn't get stripped.

This comment has been minimized.

Copy link
@jstriebel

jstriebel Jul 6, 2018

Contributor

Maybe just add a comment that shows the possible problematic folder-structures. I think there's quite some invisible knowledge in those lines, that can only be figured out by looking at several datasets.

@jstriebel
Copy link
Contributor

left a comment

👍

jstriebel and others added 3 commits Jul 6, 2018
@jstriebel

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2018

@youri-k : Please add a change note to the changelog

youri-k added 3 commits Jul 9, 2018

@youri-k youri-k merged commit 5fd00df into master Jul 9, 2018

2 checks passed

ci/circleci: build_test_deploy Your tests passed on CircleCI!
Details
coverage/coveralls Coverage remained the same at 52.251%
Details

@philippotto philippotto deleted the fix-dataset-zip-upload branch Aug 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.