Override third party libraries to check for filename casing #28

Hemant-Mann · 2018-01-10T12:20:42Z

Fix for #27

I have modified the League\Util\ContentListingFormatter class which checks whether the file is a part of the current directory or not.

The checks are done based on the property OCA\Files_external_dropbox\Storage::isCaseInsensitiveStorage

PVince81

Code looks fine.

@SergioBertolinSG can you test if that works correctly with all operations ? I think we used to have unit tests that you can run against your own account, but need to be run manually.

Hemant-Mann · 2018-01-10T12:29:28Z

Added some comments in code

SergioBertolinSG · 2018-01-10T13:12:20Z

OK now it is keeping the filenames coming from dropbox backend, good. But files/folder uploaded to OC are still being lowercased. (It requires some refreshes to see it).

Hemant-Mann · 2018-01-10T14:47:20Z

OK now it is keeping the filenames coming from dropbox backend, good. But files/folder uploaded to OC are still being lowercased. (It requires some refreshes to see it).

I have removed that code which was converting paths to lowercase

SergioBertolinSG · 2018-01-10T16:29:38Z

Ok the main issue works fine now.

I have observed that uploading a file changing the case of some characters in the filename leads to a weird dialog comparing with all the files in the folder:

Also both files appear in the folder. After a refresh only the last one appears.

Hemant-Mann · 2018-01-10T16:31:43Z

I have observed that uploading a file changing the case of some characters in the filename leads to a weird dialog comparing with all the files in the folder:

No idea why this is happening

SergioBertolinSG · 2018-01-10T17:11:20Z

Yes, this behaviour happens always, the conflict shows some random info when uploading the same file with different cases.

To reproduce:

Upload a file called for example bigHamster.jpg.
Rename in your filesystem the file to BigHamster.jpg.
Upload the same file with different name now (BigHamster.jpg).

Result:

@PVince81 what should be the right behaviour here? no conflict at all? Uploading directly? (it is the same file after all).

SergioBertolinSG · 2018-01-10T17:23:21Z

Renaming to a different case returns a warning:

The name "perrito.jpg" is already used in the folder "/DropboxAPINUEVA/MAYUSCULAS2". Please choose a different name.

Steps to reproduce:

Upload a file for example perrito.jpg.
Rename it to a different case combination for example PERRITO.jpg.
See the warning and refresh the page.

And it leads to duplicated files. The old filename only existing in owncloud, not in dropbox.

PVince81 · 2018-01-11T16:38:02Z

@SergioBertolinSG how do other case insensitive storages behave like SMB ? if they have the same issue then please ignore until we sort out these scenarios.

cc @jvillafanez

jvillafanez · 2018-01-12T08:39:07Z

Regarding SMB, uploading "FilE.txt" stores the file in SMB with the same case, and returns the file with the same case. The problem is that for SMB "FilE.txt" is the same that "file.txt" and "File.txt", so when if you have uploaded "FilE.txt" and check if "file.txt" exists, SMB says that the file exists.

Storing the file is case-sensitive but the filename comparison is case-insensitive. I think this sums up more or less what's happening with SMB.

SergioBertolinSG · 2018-01-16T16:04:56Z

@Hemant-Mann the behaviour should be the same as in SMB.

Hemant-Mann · 2018-01-19T11:36:57Z

Yes @SergioBertolinSG The same is the case for Dropbox

Storing the file is case-sensitive but the filename comparison is case-insensitive

SergioBertolinSG · 2018-01-19T12:00:03Z

This #28 (comment) and #28 (comment)

Should be addressed to merge this.

Hemant-Mann · 2018-01-19T12:44:37Z

@PVince81 Could you give a hint as to where should I look in code to address @SergioBertolinSG #28 (comment) ?

PVince81 · 2018-01-19T13:46:25Z

I don't know sorry, would need to step with a debugger.

PVince81 · 2018-01-26T09:27:58Z

@Hemant-Mann can you step there with a debugger ?

Hemant-Mann · 2018-01-29T06:59:23Z

Yeah will do
Last week was little busy

Hemant-Mann · 2018-01-29T08:49:35Z

Okay so I understood a few things
First of all consider I have the following file structure for Dropbox

DropboxV2/
├── Foo
│   └── bar
└── contacts.json

2 directories, 1 file

Now I try to upload this file ContacTs.json I get error from server
====> Response Headers

Request URL:http://localhost:8181/owncloud/remote.php/webdav/DropboxV2/ContacTs.json
Request Method:PUT
Status Code:412 Precondition failed
Remote Address:[::1]:8181
Referrer Policy:no-referrer

===> Response Body

<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\PreconditionFailed</s:exception>
  <s:message>An If-None-Match header was specified, but the ETag matched (or * was specified).</s:message>
  <s:header>If-None-Match</s:header>
</d:error>

So I put a breakpoint in frontend JS as well
https://github.com/owncloud/core/blob/master/apps/files/js/file-upload.js#L1063 (showConflict function is called)

Inside that function we try to find more info about the conflicting file here
https://github.com/owncloud/core/blob/master/apps/files/js/file-upload.js#L604
And this is where the problem is

So the function getFullPath is returning /DropboxV2 instead of /DropboxV2/ContacTs.json so backend is returning the INFO for the parent directory instead of the file ContacTs.json

Here are the 2 consequents AJAX requests sent to the server

Hemant-Mann · 2018-01-29T09:30:42Z

Also found 2 more things

Slowness is because calls of the function file_exists are not cached. I did not do this last time as it was interfering with upload process (Will open a PR to fix this)
Fixing of file name cases opened a new issue which is: duplicate files inserted in oc_filecache
Consider the above example ContacTs.json was not found in cache but dropbox returned that the file exists so cache was updated by inserting a new row for this file here
https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Scanner.php#L208
As mentioned by @SergioBertolinSG here Override third party libraries to check for filename casing #28 (comment)

Hemant-Mann · 2018-01-29T11:07:25Z

@PVince81 any ideas on how to fix the different file name issue in oc_filecache?

This is where data is fetched for ContacTs.json https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Scanner.php#L149

so $file = 'ContacTs.json and $data is what returned by lib/private/Files/Storage/Common.php::getMetaData

Checked in Debugger

Hemant-Mann · 2018-01-29T15:50:20Z

Correction this PR does not introduce this bug

As mentioned by @SergioBertolinSG here #28 (comment)

Behaviour is the same with master branch I checked

From what I have seen in the code, I guess this should be a problem for all case insensitive storage

Consider the $internalPath = 'Foo.txt' and a file foo.txt exists in external storage then
https://github.com/owncloud/core/blob/master/lib/private/Files/View.php#L1364
will say file exists and correspondingly cache will be updated follow these

https://github.com/owncloud/core/blob/master/lib/private/Files/View.php#L1369
https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Scanner.php#L311
https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Scanner.php#L149
https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Scanner.php#L208 (where $file = 'Foo.txt', $data = (array) $fileId = -1)

jvillafanez · 2018-01-30T08:09:26Z

As long as the behaviour is acceptable and ownCloud doesn't do weird things, I'd just return whatever Dropbox is doing. If you have contacts.json and Dropbox says that contacTs.json exists, then just go with it.

As far as I know, there are only a couple of problematic behaviours: uploading a file with the same name but different casing and renaming a file with different casing. If ownCloud just rejects both operations, I think it's fine as long as the users know this.

Hemant-Mann · 2018-02-01T06:11:30Z

If you have contacts.json and Dropbox says that contacTs.json exists, then just go with it.

Yes this is the current behaviour

But a new file contacTs.json is added in oc_filecache and as a result 2 files are displayed in folder listing
I think this is what @SergioBertolinSG is pointing out

Hemant-Mann · 2018-02-01T11:06:33Z

So should we merge this?

PVince81 · 2018-02-01T11:10:13Z

maybe we need a more generic solution in general when dealing with any kind of case insensitive storages: owncloud/core#17161. From @jvillafanez's research this is not easy to achieve.

If this PR makes Dropbox works a little better then let's merge it as a step forward.

patrickjahns · 2018-02-04T15:15:30Z

lib/Storage/CacheableFlysystemAdapter.php

+	 * This property is used to check whether the storage is case insensitive or not
+	 * @var boolean
+	 */
+	protected $isCaseInsensitiveStorage = false;


tabs vs. spaces -> please unify

Okay let me correct this

@patrickjahns I checked in the editor it is indented using Tabs only
the diff is because previously this block was indented using tab size: 2 whereas the whole file was indented using tab size: 4

patrickjahns · 2018-02-05T20:01:31Z

Moved to development and adjusted base branch

Hemant-Mann · 2018-02-13T14:07:57Z

Merge this?

patrickjahns · 2018-02-13T14:27:58Z

@SergioBertolinSG
You had a look at this - what was the latest testing results with this PR ?

SergioBertolinSG · 2018-02-13T14:36:54Z

#27 is fixed here but
#28 (comment) and #28 (comment) are introduced.

Which is a different behaviour than WND.

Hemant-Mann · 2018-02-14T05:28:20Z

@SergioBertolinSG
As I have pointed out in various comments this PR does not introduce these

#28 (comment) and #28 (comment) are introduced.

You can verify with master branch the behaviour is the same. Both of your comments can be reproduced easily

a) Regarding the incorrect conflict dialog box it related to file-upload.js in ownCloud core. I have mentioned the details here #28 (comment)

b) Regarding duplicate file issue #28 (comment) it is also related to ownCloud core @PVince81 have mentioned here #28 (comment) (This behaviour should be easily reproduced with other case insensitive storages)

SergioBertolinSG · 2018-02-14T11:17:01Z

@Hemant-Mann Ok I have check those using current master and you are right.

So lets merge this and open separated issues about them.

patrickjahns · 2018-02-15T10:15:43Z

@SergioBertolinSG
please create respective issues

SergioBertolinSG · 2018-02-16T12:33:03Z

Opened here:

Renaming of files to a different case leads to file duplication. #33

Files conflict dialog is not prepared to deal with case-sensitive storages.
(This one affects all case-sensitive storages)
owncloud/core#30502

Override third party libraries to check for filename casing

d20a4bd

PVince81 reviewed Jan 10, 2018

View reviewed changes

Add comments regarding changes in vendor files

d247073

Hemant-Mann changed the base branch from master to release/1.0.1 January 10, 2018 12:33

Don't Convert filename to lowercase while uploading files

24d4c06

SergioBertolinSG mentioned this pull request Jan 11, 2018

Test files_external_dropbox 1.0.1 #22

Closed

PVince81 mentioned this pull request Jan 29, 2018

Slow performance #26

Closed

Hemant-Mann mentioned this pull request Jan 29, 2018

File Exists Stat Cache #29

Merged

patrickjahns reviewed Feb 4, 2018

View reviewed changes

patrickjahns changed the base branch from release/1.0.1 to master February 5, 2018 20:00

patrickjahns added this to the development milestone Feb 5, 2018

fix(indentation): Convert Spaces to Tabs

7ae1dad

Hemant-Mann mentioned this pull request Feb 9, 2018

1.0.1 release checklist #19

Closed

11 tasks

patrickjahns merged commit 6a9d236 into master Feb 15, 2018

patrickjahns deleted the file_names_case branch February 15, 2018 10:15

PVince81 mentioned this pull request Apr 24, 2018

File and folder names are converted to lowercase #27

Closed

PVince81 modified the milestones: development, QA Nov 23, 2018

davitol mentioned this pull request Nov 29, 2018

Tracking results for Dropbox External Storage Release 1.1.0 #44

Closed

Override third party libraries to check for filename casing #28

Override third party libraries to check for filename casing #28

Conversation

Hemant-Mann commented Jan 10, 2018

PVince81 left a comment

Choose a reason for hiding this comment

Hemant-Mann commented Jan 10, 2018

SergioBertolinSG commented Jan 10, 2018

Hemant-Mann commented Jan 10, 2018

SergioBertolinSG commented Jan 10, 2018 • edited Loading

Hemant-Mann commented Jan 10, 2018 • edited Loading

SergioBertolinSG commented Jan 10, 2018

SergioBertolinSG commented Jan 10, 2018 • edited Loading

PVince81 commented Jan 11, 2018

jvillafanez commented Jan 12, 2018

SergioBertolinSG commented Jan 16, 2018

Hemant-Mann commented Jan 19, 2018

SergioBertolinSG commented Jan 19, 2018

Hemant-Mann commented Jan 19, 2018

PVince81 commented Jan 19, 2018

PVince81 commented Jan 26, 2018

Hemant-Mann commented Jan 29, 2018

Hemant-Mann commented Jan 29, 2018

Hemant-Mann commented Jan 29, 2018

Hemant-Mann commented Jan 29, 2018 • edited Loading

Hemant-Mann commented Jan 29, 2018 • edited Loading

jvillafanez commented Jan 30, 2018

Hemant-Mann commented Feb 1, 2018

Hemant-Mann commented Feb 1, 2018

PVince81 commented Feb 1, 2018

patrickjahns Feb 4, 2018

Choose a reason for hiding this comment

Hemant-Mann Feb 6, 2018

Choose a reason for hiding this comment

Hemant-Mann Feb 6, 2018

Choose a reason for hiding this comment

patrickjahns commented Feb 5, 2018

Hemant-Mann commented Feb 13, 2018

patrickjahns commented Feb 13, 2018

SergioBertolinSG commented Feb 13, 2018

Hemant-Mann commented Feb 14, 2018

SergioBertolinSG commented Feb 14, 2018

patrickjahns commented Feb 15, 2018

SergioBertolinSG commented Feb 16, 2018

SergioBertolinSG commented Jan 10, 2018 •

edited

Loading

Hemant-Mann commented Jan 10, 2018 •

edited

Loading

SergioBertolinSG commented Jan 10, 2018 •

edited

Loading

Hemant-Mann commented Jan 29, 2018 •

edited

Loading

Hemant-Mann commented Jan 29, 2018 •

edited

Loading