WebDav:listFolder - skip adding phantom items by mergen3107 · Pull Request #12034 · koreader/koreader

mergen3107 · 2024-06-16T02:59:14Z

Closes #12025

This is my try to fix the issue when a WebDav folder contains a phantom item with exact same name as that of the current folder itself. This phantom item is, of course, non-interactable.

So, in the for loop where all url-decoded items are added to the webdav_list table, I check whether current-iteration item is phantom ("path to current item with trailing slash" is equal to "path of current folder") or real.

Any suggestions or comments are appreciated! :D

This change is

mergen3107 · 2024-06-16T03:02:11Z

I tried to avoid solutions with table.remove for two reasons: I don't want to blindly remove just first items (what if they are real?) and also this operation duplicates a whole table lest the first item (what if that table is huge?).

mergen3107 · 2024-06-16T03:07:04Z

Tested on emulator 2024.04-186 with my cloud settings as on Kindle Scribe.

mergen3107 · 2024-06-16T03:09:50Z

Found two issues:

Phantom folder appears if I go to a folder with spaces in the name
Phantom folder appears if I go to next level of the folder. E.g. Books/ doesn't have the phantom item, but Books/123/ does.

mergen3107 · 2024-06-16T03:12:25Z

First case above happens because path (of current folder) has %20 in lieu of a white space. So /Deus Ex PC Build string is /Deus%20Ex%20PC%20Build. Is this a results of html decode?

mergen3107 · 2024-06-16T03:14:51Z

Second problems happens because I compare two different things now:

06/15/24-23:13:25 DEBUG LOGG item_name =  DeDRM 
06/15/24-23:13:25 DEBUG LOGG path =  /Kindle/DeDRM

I should probably strip all characters until including last slash.

mergen3107 · 2024-06-16T03:48:38Z

Number 1 is fixed by using unencoded folder_path instead of path.
Number 2 is fixed by comparing item_name with a string shortened from full folder_path by the length of item_name.
For example, item_name = 123 456, folder_path = /Books/123 456, and shorted string to compare item_name with is 123 456.

mergen3107 · 2024-06-16T03:49:43Z

I know it looks way too hacky, any suggestions are welcome!

Otherwise, emulator tests are great!

NiLuJe · 2024-06-16T04:10:45Z

It seems like isCurrentDirectory is what should be taking care of this.

It also seems that this is seven billion times simpler, which I like.

Then again, I'm not familiar with DAV, so I'm unsure of whatever edge-cases it's currently trying to deal with.

TL;DR: Does things horribly break with a simple

diff --git a/frontend/apps/cloudstorage/webdavapi.lua b/frontend/apps/cloudstorage/webdavapi.lua
index f0d36696f..a3414b103 100644
--- a/frontend/apps/cloudstorage/webdavapi.lua
+++ b/frontend/apps/cloudstorage/webdavapi.lua
@@ -122,14 +122,12 @@ function WebDavApi:listFolder(address, user, pass, folder_path, folder_mode)
             --logger.dbg("WebDav catalog item=", item)
             -- <d:href> is the path and filename of the entry.
             local item_fullpath = item:match("<[^:]*:href[^>]*>(.*)</[^:]*:href>")
-            if string.sub( item_fullpath, -1 ) == "/" then
-                item_fullpath = string.sub( item_fullpath, 1, -2 )
-            end
-            local is_current_dir = self:isCurrentDirectory( util.urlDecode(item_fullpath), address, folder_path )
 
-            local item_name = util.urlDecode( FFIUtil.basename( item_fullpath ) )
+            local item_name = util.urlDecode(FFIUtil.basename(item_fullpath))
             item_name = util.htmlEntitiesToUtf8(item_name)
 
+            local is_current_dir = item_name == string.sub(folder_path, -#item_name)
+
             local is_not_collection = item:find("<[^:]*:resourcetype/>") or
                 item:find("<[^:]*:resourcetype></[^:]*:resourcetype>")

(Stripping the trailing slash from item_fullpath is redundant, basename already handles it sanely).

mergen3107 · 2024-06-16T04:21:46Z

@NiLuJe
Things didn't break, but they didn't work neither - phantom folders are back!

mergen3107 · 2024-06-16T04:24:21Z

Ah, did you mean, this diff is applied on top of this PR? I applied it on fresh nightly

mergen3107 · 2024-06-16T04:26:22Z

applied on top of this PR

@NiLuJe
All good in this case (on Kindle). I'll make a commit, thanks!

mergen3107 · 2024-06-16T04:35:17Z

Sorry, I think I got it finally (need some sleep).
Let me push commit with comments

mergen3107 · 2024-06-16T04:39:34Z

@NiLuJe
With last commit, it works the same good as original PR :D nothing broke (Kindle Scribe)

mergen3107 · 2024-06-16T04:40:02Z

Overall diff looks much cleaner, thank you!

mergen3107 · 2024-06-16T04:47:52Z

Tested on emulator as well, all good.

mergen3107 · 2024-06-16T04:49:11Z

(removing those lines made the phantom folders come back)

-            if string.sub( item_fullpath, -1 ) == "/" then
-                item_fullpath = string.sub( item_fullpath, 1, -2 )
-            end

It works in the current state of this PR.

hius07 · 2024-06-16T05:21:42Z

Dunno anything about the issue, but after your changes isCurrentDirectory() is not used anymore.
It is worth to investigate why rather complicated method was needed, instead of just one string comparison.
Maybe, some edge cases or something?

NiLuJe · 2024-06-16T06:27:20Z

Dunno anything about the issue, but after your changes isCurrentDirectory() is not used anymore.

That's possibly going to annoy Luacheck, for the record. I didn't mention that before ;p.

It is worth to investigate why rather complicated method was needed, instead of just one string comparison.
Maybe, some edge cases or something?

Yep, I'm not familiar with the codepaths involved (or DAV) so I can't really help there, but it is a concern I share ;).

NiLuJe · 2024-06-16T06:31:00Z

(removing those lines made the phantom folders come back)
-            if string.sub( item_fullpath, -1 ) == "/" then
-                item_fullpath = string.sub( item_fullpath, 1, -2 )
-            end
It works in the current state of this PR.

Huh.

What if we stop doing stupid things like passing URL-encoded crap to basename?

~~i.e., local item_name = FFIUtil.basename(util.urlDecode(item_fullpath))~~

EDIT: HTML entities, too.

local item_name = FFIUtil.basename(util.htmlEntitiesToUtf8(util.urlDecode(item_fullpath)))

(Dropping the following call that used to be separate).

NiLuJe · 2024-06-16T07:13:17Z

No context braindump because the torturous logic made my brain hurt:

diff --git a/frontend/apps/cloudstorage/webdav.lua b/frontend/apps/cloudstorage/webdav.lua
index bf9795879..be2a23e85 100644
--- a/frontend/apps/cloudstorage/webdav.lua
+++ b/frontend/apps/cloudstorage/webdav.lua
@@ -67,7 +67,7 @@ function WebDav:uploadFile(url, address, username, password, local_path, callbac
 end
 
 function WebDav:createFolder(url, address, username, password, folder_name, callback_close)
-    local code_response = WebDavApi:createFolder(address .. WebDavApi:urlEncode(url .. "/" .. folder_name), username, password, folder_name)
+    local code_response = WebDavApi:createFolder(address .. WebDavApi.urlEncode(url .. "/" .. folder_name), username, password, folder_name)
     if code_response == 201 then
         if callback_close then
             callback_close()
diff --git a/frontend/apps/cloudstorage/webdavapi.lua b/frontend/apps/cloudstorage/webdavapi.lua
index f0d36696f..e3d1e7333 100644
--- a/frontend/apps/cloudstorage/webdavapi.lua
+++ b/frontend/apps/cloudstorage/webdavapi.lua
@@ -12,44 +12,23 @@ local lfs = require("libs/libkoreader-lfs")
 local WebDavApi = {
 }
 
-function WebDavApi:getJoinedPath( address, path )
-    local path_encoded = self:urlEncode( path ) or ""
-    local address_strip = address:sub(-1) == "/" and address:sub(1, -2) or address
-    local path_strip = path_encoded:sub(1, 1) == "/" and path_encoded:sub(2) or path_encoded
-    return address_strip .. "/" .. path_strip
+-- Trim leading & trailing slashes from string `s` (based on util.trim)
+function WebDavApi.trim_slashes(s)
+    local from = s:match"^/*()"
+    return from > #s and "" or s:match(".*[^/]", from)
 end
 
-function WebDavApi:isCurrentDirectory( current_item, address, path )
-    local is_home, is_parent
-    local home_path
-    -- find first occurence of / after http(s)://
-    local start = string.find( address, "/", 9 )
-    if not start then
-        home_path = "/"
-    else
-        home_path = string.sub( address, start )
+-- Trim trailing slashes from string `s` (based on util.rtrim)
+function WebDavApi.rtrim_slashes(s)
+    local n = #s
+    while n > 0 and s:find("^/", n) do
+        n = n - 1
     end
-    local item
-    if string.sub( current_item, -1 ) == "/" then
-        item = string.sub( current_item, 1, -2 )
-    else
-        item = current_item
-    end
-
-    if item == home_path then
-        is_home = true
-    else
-        local temp_path = string.sub( item, string.len(home_path) + 1 )
-        if string.sub( path, -1 ) == "/" then path = string.sub( path, 1, -2 ) end
-        if temp_path == path then
-            is_parent = true
-        end
-    end
-    return is_home or is_parent
+    return s:sub(1, n)
 end
 
--- version of urlEncode that doesn't encode the /
-function WebDavApi:urlEncode(url_data)
+-- Variant of util.urlEncode that doesn't encode the /
+function WebDavApi.urlEncode(url_data)
     local char_to_hex = function(c)
         return string.format("%%%02X", string.byte(c))
     end
@@ -60,28 +39,30 @@ function WebDavApi:urlEncode(url_data)
     return url_data
 end
 
+-- Append path to address with a slash separator, trimming any unwanted slashes in the process.
+function WebDavApi:getJoinedPath(address, path)
+    local path_encoded = self.urlEncode(path) or ""
+    -- Strip leading & trailing slashes from `path`
+    local sane_path = self.trim_slashes(path_encoded)
+    -- Strip trailing slashes from `address` for now
+    local sane_address = self.rtrim_slashes(address)
+    -- Join our final URL
+    return sane_address .. "/" .. sane_path
+end
+
 function WebDavApi:listFolder(address, user, pass, folder_path, folder_mode)
-    local path = self:urlEncode( folder_path )
+    local path = self.urlEncode(folder_path) or ""
     local webdav_list = {}
     local webdav_file = {}
 
-    local has_trailing_slash = false
-    local has_leading_slash = false
-    if string.sub( address, -1 ) == "/" then has_trailing_slash = true end
-    if path == nil or path == "/" then
-        path = ""
-    elseif string.sub( path, 1, 1 ) == "/" then
-        if has_trailing_slash then
-            -- too many slashes, remove one
-            path = string.sub( path, 2 )
-        end
-        has_leading_slash = true
-    end
-    if not has_trailing_slash and not has_leading_slash then
-        address = address .. "/"
-    end
-    local webdav_url = address .. path
-    if string.sub(webdav_url, -1) ~= "/" then
+    -- Strip leading & trailing slashes from `path`
+    path = self.trim_slashes(path)
+    -- Strip trailing slashes from `address` for now
+    address = self.rtrim_slashes(address)
+    -- Join our final URL, which *must* have a trailing / (it's a URL)
+    -- This is where we deviate from getJoinedPath ;).
+    local webdav_url = address .. "/" .. path
+    if webdav_url:sub(-1) ~= "/" then
         webdav_url = webdav_url .. "/"
     end
 
@@ -122,24 +103,18 @@ function WebDavApi:listFolder(address, user, pass, folder_path, folder_mode)
             --logger.dbg("WebDav catalog item=", item)
             -- <d:href> is the path and filename of the entry.
             local item_fullpath = item:match("<[^:]*:href[^>]*>(.*)</[^:]*:href>")
-            if string.sub( item_fullpath, -1 ) == "/" then
-                item_fullpath = string.sub( item_fullpath, 1, -2 )
-            end
-            local is_current_dir = self:isCurrentDirectory( util.urlDecode(item_fullpath), address, folder_path )
-
-            local item_name = util.urlDecode( FFIUtil.basename( item_fullpath ) )
-            item_name = util.htmlEntitiesToUtf8(item_name)
-
+            local item_name = FFIUtil.basename(util.htmlEntitiesToUtf8(util.urlDecode(item_fullpath)))
+            local is_current_dir = item_name == string.sub(folder_path, -#item_name)
             local is_not_collection = item:find("<[^:]*:resourcetype/>") or
-                item:find("<[^:]*:resourcetype></[^:]*:resourcetype>")
+                                      item:find("<[^:]*:resourcetype></[^:]*:resourcetype>")
+            local item_path = path .. "/" .. item_name
 
-            local item_path = (path == "" and has_trailing_slash) and item_name or path .. "/" .. item_name
             if item:find("<[^:]*:collection[^<]*/>") then
                 item_name = item_name .. "/"
                 if not is_current_dir then
                     table.insert(webdav_list, {
                         text = item_name,
-                        url = util.urlDecode( item_path ),
+                        url = item_path,
                         type = "folder",
                     })
                 end
@@ -147,7 +122,7 @@ function WebDavApi:listFolder(address, user, pass, folder_path, folder_mode)
                 or G_reader_settings:isTrue("show_unsupported")) then
                 table.insert(webdav_file, {
                     text = item_name,
-                    url = util.urlDecode( item_path ),
+                    url = item_path,
                     type = "file",
                 })
             end
diff --git a/frontend/util.lua b/frontend/util.lua
index 62c4cfd83..ddf2b906c 100644
--- a/frontend/util.lua
+++ b/frontend/util.lua
@@ -54,6 +54,23 @@ function util.trim(s)
    return from > #s and "" or s:match(".*%S", from)
 end
 
+--[[
+-- Trim leading & trailing character `c` from string `s`
+function util.trim_char(s, c)
+    local from = s:match"^"..c.."*()"
+    return from > #s and "" or s:match(".*[^"..c.."]", from)
+end
+
+-- Trim trailing character `c` from string `s`
+function util.rtrim_char(s, c)
+    local n = #s
+    while n > 0 and s:find("^"..c, n) do
+        n = n - 1
+    end
+    return s:sub(1, n)
+end
+--]]
+
 --[[--
 Splits a string by a pattern

mergen3107 · 2024-06-16T17:13:45Z

(don't know how to convert this to draft, closing for now)

Frenzie · 2024-06-16T18:39:25Z

I'm not sure if you can other than at the time of creation, maybe somewhere in the side menu.

NiLuJe · 2024-06-16T19:35:52Z

I'm not sure if you can other than at the time of creation, maybe somewhere in the side menu.

Yep, it's at the top of the right side-penel, bundled with the reviews.

Rest assured, it takes me 35s to find it every time I look for it ;).

benoit-pierre · 2024-06-16T19:38:16Z

You can set it a creation? I always switch it on after open the PR…

benoit-pierre · 2024-06-16T19:39:54Z

Oh, right, the drop down on the 'Create pull request' button. TIL

NiLuJe · 2024-06-16T19:46:22Z

Oh, right, the drop down on the 'Create pull request' button. TIL

Yeah, I also routinely forget where that one is hidden, although it is slightly more obvious than the other one (maaaybe? :D) ;).

mergen3107 added 2 commits June 15, 2024 22:55

skip adding item to list if the same as folder path

561edd3

add explanatory comment

8f827b2

mergen3107 changed the title ~~WebDav: skip adding item to list if the same as folder path~~ WebDav:listFolder - skip adding phantom items Jun 16, 2024

change path to folder_path and move misplaced comment

c3d1bfb

mergen3107 force-pushed the webdav_phantom_folder branch from 3373b99 to c3d1bfb Compare June 16, 2024 03:26

fix nested folders check

59cc47c

modify and rely on is_current_dir only

d0e7fae

mergen3107 mentioned this pull request Jun 16, 2024

WebDav: Simplify path manipulations to fix wonky heuristics #12038

Merged

mergen3107 closed this Jun 16, 2024

mergen3107 deleted the webdav_phantom_folder branch June 20, 2024 17:48

Uh oh!

Uh oh!

Conversation

mergen3107 commented Jun 16, 2024 • edited by Frenzie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergen3107 commented Jun 16, 2024

Uh oh!

mergen3107 commented Jun 16, 2024

Uh oh!

mergen3107 commented Jun 16, 2024

Uh oh!

mergen3107 commented Jun 16, 2024

Uh oh!

mergen3107 commented Jun 16, 2024

Uh oh!

mergen3107 commented Jun 16, 2024

Uh oh!

mergen3107 commented Jun 16, 2024

Uh oh!

NiLuJe commented Jun 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergen3107 commented Jun 16, 2024

Uh oh!

mergen3107 commented Jun 16, 2024

Uh oh!

mergen3107 commented Jun 16, 2024

Uh oh!

mergen3107 commented Jun 16, 2024

Uh oh!

mergen3107 commented Jun 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergen3107 commented Jun 16, 2024

Uh oh!

mergen3107 commented Jun 16, 2024

Uh oh!

mergen3107 commented Jun 16, 2024

Uh oh!

hius07 commented Jun 16, 2024

Uh oh!

NiLuJe commented Jun 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NiLuJe commented Jun 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NiLuJe commented Jun 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergen3107 commented Jun 16, 2024

Uh oh!

Frenzie commented Jun 16, 2024

Uh oh!

NiLuJe commented Jun 16, 2024

Uh oh!

benoit-pierre commented Jun 16, 2024

Uh oh!

benoit-pierre commented Jun 16, 2024

Uh oh!

NiLuJe commented Jun 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mergen3107 commented Jun 16, 2024 •

edited by Frenzie

Loading

NiLuJe commented Jun 16, 2024 •

edited

Loading

mergen3107 commented Jun 16, 2024 •

edited

Loading

NiLuJe commented Jun 16, 2024 •

edited

Loading

NiLuJe commented Jun 16, 2024 •

edited

Loading

NiLuJe commented Jun 16, 2024 •

edited

Loading

NiLuJe commented Jun 16, 2024 •

edited

Loading