From 1b00746f23688f8f9e98697b9ce0ccfdd744e6bc Mon Sep 17 00:00:00 2001 From: Shane Canon Date: Tue, 11 Jun 2019 20:47:30 -0700 Subject: [PATCH 1/7] This is a potential fix to issue #4669. The fix simply catches the recusrive symlink error and moves on. It is possibble that the try/except belongs in the utils but I wanted to limit the scope. --- notebook/services/contents/filemanager.py | 18 +++++++++++------ .../services/contents/tests/test_manager.py | 20 +++++++++++++++++++ 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/notebook/services/contents/filemanager.py b/notebook/services/contents/filemanager.py index 7f445a8f71..ec1d84b3b7 100644 --- a/notebook/services/contents/filemanager.py +++ b/notebook/services/contents/filemanager.py @@ -334,12 +334,18 @@ def _dir_model(self, path, content=True): self.log.debug("%s not a regular file", os_path) continue - if self.should_list(name): - if self.allow_hidden or not is_file_hidden(os_path, stat_res=st): - contents.append( - self.get(path='%s/%s' % (path, name), content=False) - ) - + try: + if self.should_list(name): + if self.allow_hidden or not is_file_hidden(os_path, stat_res=st): + contents.append( + self.get(path='%s/%s' % (path, name), content=False) + ) + except OSError as e: + # Ignore recursive links and move on + if e.errno==62: + continue + else: + raise e model['format'] = 'json' return model diff --git a/notebook/services/contents/tests/test_manager.py b/notebook/services/contents/tests/test_manager.py index 0e6b0fb2b2..85cea48213 100644 --- a/notebook/services/contents/tests/test_manager.py +++ b/notebook/services/contents/tests/test_manager.py @@ -129,6 +129,26 @@ def test_bad_symlink(self): # broken symlinks should still be shown in the contents manager self.assertTrue('bad symlink' in contents) + @dec.skipif(sys.platform == 'win32' and sys.version_info[0] < 3) + def test_recursive_symlink(self): + with TemporaryDirectory() as td: + cm = FileContentsManager(root_dir=td) + path = 'test recursive symlink' + _make_dir(cm, path) + os_path = cm._get_os_path(path) + os.symlink("recursive", os.path.join(os_path, "recursive")) + file_model = cm.new_untitled(path=path, ext='.txt') + + model = cm.get(path) + + contents = { + content['name']: content for content in model['content'] + } + self.assertTrue('untitled.txt' in contents) + self.assertEqual(contents['untitled.txt'], file_model) + # recusrive symlinks should not be shown in the contents manager + self.assertFalse('recusrive' in contents) + @dec.skipif(sys.platform == 'win32' and sys.version_info[0] < 3) def test_good_symlink(self): with TemporaryDirectory() as td: From 088b212fc741e27b7e373428f6caba72573d916b Mon Sep 17 00:00:00 2001 From: Shane Canon Date: Tue, 11 Jun 2019 21:42:11 -0700 Subject: [PATCH 2/7] It looks like too many levels is error number 40 on linux. --- notebook/services/contents/filemanager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notebook/services/contents/filemanager.py b/notebook/services/contents/filemanager.py index ec1d84b3b7..d10b69b267 100644 --- a/notebook/services/contents/filemanager.py +++ b/notebook/services/contents/filemanager.py @@ -342,7 +342,7 @@ def _dir_model(self, path, content=True): ) except OSError as e: # Ignore recursive links and move on - if e.errno==62: + if e.errno==62 or e.errno==40: continue else: raise e From 4531fc82ef5e39a0b7954df81b98e50415f5509c Mon Sep 17 00:00:00 2001 From: Shane Canon Date: Wed, 12 Jun 2019 09:00:52 -0700 Subject: [PATCH 3/7] Switch to using errno instead of hardcoding the number. --- notebook/services/contents/filemanager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notebook/services/contents/filemanager.py b/notebook/services/contents/filemanager.py index d10b69b267..06bdff7a5e 100644 --- a/notebook/services/contents/filemanager.py +++ b/notebook/services/contents/filemanager.py @@ -342,7 +342,7 @@ def _dir_model(self, path, content=True): ) except OSError as e: # Ignore recursive links and move on - if e.errno==62 or e.errno==40: + if e.errno==errno.ELOOP: continue else: raise e From f586668e5e686bc49d7aa5d6dc9c1c94824930cf Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Tue, 25 Jun 2019 10:23:24 +0200 Subject: [PATCH 4/7] Fix spelling, smarter assert methods --- notebook/services/contents/tests/test_manager.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/notebook/services/contents/tests/test_manager.py b/notebook/services/contents/tests/test_manager.py index 85cea48213..b1b3e2c6cd 100644 --- a/notebook/services/contents/tests/test_manager.py +++ b/notebook/services/contents/tests/test_manager.py @@ -144,10 +144,10 @@ def test_recursive_symlink(self): contents = { content['name']: content for content in model['content'] } - self.assertTrue('untitled.txt' in contents) + self.assertIn('untitled.txt', contents) self.assertEqual(contents['untitled.txt'], file_model) - # recusrive symlinks should not be shown in the contents manager - self.assertFalse('recusrive' in contents) + # recursive symlinks should not be shown in the contents manager + self.assertNotIn('recursive', contents) @dec.skipif(sys.platform == 'win32' and sys.version_info[0] < 3) def test_good_symlink(self): From 4bad3762e9a7b6aad8ed8dbffeb048879a8a9f5d Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Tue, 8 Oct 2019 17:04:30 +0100 Subject: [PATCH 5/7] Log unrecognised errors and continue listing directory --- notebook/services/contents/filemanager.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/notebook/services/contents/filemanager.py b/notebook/services/contents/filemanager.py index 06bdff7a5e..b7ba7c5c7a 100644 --- a/notebook/services/contents/filemanager.py +++ b/notebook/services/contents/filemanager.py @@ -341,11 +341,13 @@ def _dir_model(self, path, content=True): self.get(path='%s/%s' % (path, name), content=False) ) except OSError as e: - # Ignore recursive links and move on - if e.errno==errno.ELOOP: - continue - else: - raise e + # ELOOP: recursive symlink + if e.errno != errno.ELOOP: + self.log.warning( + "Unknown error checking if file %r is hidden", + os_path, + exc_info=True, + ) model['format'] = 'json' return model From bc41afb363795bba20de38693d1c74e8ba1a5aca Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Tue, 8 Oct 2019 17:05:52 +0100 Subject: [PATCH 6/7] Skip recursive symlink test entirely on Windows --- notebook/services/contents/tests/test_manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notebook/services/contents/tests/test_manager.py b/notebook/services/contents/tests/test_manager.py index b1b3e2c6cd..d456f91e6c 100644 --- a/notebook/services/contents/tests/test_manager.py +++ b/notebook/services/contents/tests/test_manager.py @@ -149,7 +149,7 @@ def test_recursive_symlink(self): # recursive symlinks should not be shown in the contents manager self.assertNotIn('recursive', contents) - @dec.skipif(sys.platform == 'win32' and sys.version_info[0] < 3) + @dec.skipif(sys.platform == 'win32') def test_good_symlink(self): with TemporaryDirectory() as td: cm = FileContentsManager(root_dir=td) From c1a31177436e77931bccefa57ad55dfc5e0973f7 Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Tue, 8 Oct 2019 17:17:07 +0100 Subject: [PATCH 7/7] Fix which test is skipped on Windows --- notebook/services/contents/tests/test_manager.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/notebook/services/contents/tests/test_manager.py b/notebook/services/contents/tests/test_manager.py index d456f91e6c..371fb2a655 100644 --- a/notebook/services/contents/tests/test_manager.py +++ b/notebook/services/contents/tests/test_manager.py @@ -129,7 +129,7 @@ def test_bad_symlink(self): # broken symlinks should still be shown in the contents manager self.assertTrue('bad symlink' in contents) - @dec.skipif(sys.platform == 'win32' and sys.version_info[0] < 3) + @dec.skipif(sys.platform == 'win32') def test_recursive_symlink(self): with TemporaryDirectory() as td: cm = FileContentsManager(root_dir=td) @@ -149,7 +149,7 @@ def test_recursive_symlink(self): # recursive symlinks should not be shown in the contents manager self.assertNotIn('recursive', contents) - @dec.skipif(sys.platform == 'win32') + @dec.skipif(sys.platform == 'win32' and sys.version_info[0] < 3) def test_good_symlink(self): with TemporaryDirectory() as td: cm = FileContentsManager(root_dir=td)