search

mschilli / net-google-drive-simple

Net::Google::Drive::Simple CPAN Module
Other
11 stars 22 forks source link

path_resolve and the first element of @parts #33

Closed NetworkNed closed 4 years ago

NetworkNed commented 4 years ago

In the google-drive-ls example program the directory to be listed was hard-coded, and I provide a simple update so that the directory name must be given as an argument. This tool may be a useful companion to the following discussion of the sub path_resolve in Simple.pm.

The existing code is basically unchanged since version 0.01 e01bfdcb84c48406423e7292ac6ba881202b9e56 by @mschilli, although a97631aed239d5445b4da00f12ac81cd3796f8d5 saw it refactored from sub children into the current path_resolve.

The primary argument passed to path_resolve is the path which it is required to resolve, and line 339 of the current master has split break this into components, stored in the array @parts.

The perldoc for split says "an empty leading field is produced when there is a positive-width match at the beginning of EXPR" and therefore /path/to/folder becomes an array not of ( "path", "to", "folder" ) but, due to this leading slash, ( "", "path", "to", "folder" ).

The subroutine relies on this empty first element, overwriting it with "root" on line 341.

I find the following couple of lines confusing - almost immediately the "root" is shifted back out from the front of the array into $folder_id on line 344, and then this is added to array @ids on the following line (345).

This is all well and good providing the path has been provided in the absolute style (I think it must always be absolute?), with a leading slash. But consider if the argument path/to/folder is provided instead - array @parts is first ( "path", "to", "folder" ) and then the first element is overwritten with root, ( "root", "to", "folder" ), and deleted ( "to", "folder" ), so that it is treated the same as had the argument /to/folder been given.

Consequently we have a situation where Net::Google::Drive::Simple will resolve /Folder correctly and give an appropriate error for /UnknownFlodder but return the root directory when Folder or UnknownFlodder is specified.

My fix uses a grep ensure that the array@parts is assigned with the empty element removed (line 339) and then assigns "root" to both $folder_id and $parent (line 341). We delete line 344) as superfluous because we no longer need to shift "root" out of the beginning of the array. In this revision it is never put in the array in the first place.

So far this revision has tested good with every combination of /path/to/folder, /path, path, /path/to and path/to I have tried it with. I cannot see any reason why my revision is not logically sound, but I fear I may be overlooking something. I appreciate your advice and I hope this pull request is correct and appropriate.

NetworkNed commented 4 years ago

Addendum: is there any reason not to simply declare @ids as an array containing the element "root" and dispense with line 344 where we push it in there from another variable?