HPCC-30469 Avoid duplicated PathSepChar in getExternalPath() #17898
Conversation
|
https://track.hpccsystems.com/browse/HPCC-30469 |
|
A 9.0.x build has been tested. |
dali/base/dautils.cpp
Outdated
| //If the prefix is a PathSepChar, it should not be appended to the dir here because | ||
| //a PathSepChar will be appended to the dir inside the expandExternalPath() if the s | ||
| //is started with the "::". | ||
| if (!streq(prefix, iswin ? "\\" : "/")) |
There was a problem hiding this comment.
if (!isRootDirectory(prefix))
is better, but will merge as-is
There was a problem hiding this comment.
I don't think it's because it's "/" per se - I think it's problematic if there's a trailing '/' on the prefix.
There was a problem hiding this comment.
Yes, I've tested, and if any plane prefix has a trailing '/', the result will be wrong, i.e. it will have a double // at the junction between the end of the prefix and the filePath portion.
So the fix here should be to check for trailing pathsepchar, and avoid appending it to 'dir', that will then also be correct for when prefix == "/"
There was a problem hiding this comment.
Will change to isRootDirectory() and add the removeTrailingPathSepChar().
| @@ -5642,6 +5642,24 @@ const char *splitRelativePath(const char *full,const char *basedir,StringBuffer | |||
| return t; | |||
| } | |||
|
|
|||
| const char *getRelativePath(const char *path,const char *leadingPath) | |||
There was a problem hiding this comment.
note to myself - this is backported from 9.2
| @@ -256,6 +256,16 @@ extern TPWRAPPER_API void validateDropZoneAccess(IEspContext& context, const cha | |||
| if (!isHostInPlane(dropZone, hostReq, true)) | |||
| throw makeStringExceptionV(ECLWATCH_INVALID_INPUT, "Host %s is not valid DropZone plane %s", hostReq, targetDZNameOrHost); | |||
| } | |||
|
|
|||
| //If the dropZonePath is an absolute path, change it to a relative path. | |||
| if (isAbsolutePath(fileNameWithRelPath)) | |||
There was a problem hiding this comment.
can we also add some comment to CWsFileIOEx::onReadFileData in this PR (to reduce future confusion) to note that despite the "destRelativePath" saying it's relative for legacy reason, it also supports absolute paths?
(I'm also curious why ReadFileDataRequest uses fields with "Dest" in them, when this is only used for reading ..)
There was a problem hiding this comment.
Will add the comment.
All of the three methods in WsFileIO uses the "Dest". But, it is not 'good' for the ReadFileDataRequest.
| //is started with the "::". | ||
| //Also a trailing pathsepchar in the prefix should be removed. | ||
| if (!isRootDirectory(prefix)) | ||
| dir.append(prefix); |
There was a problem hiding this comment.
I don't think you want or need to special case root, "/" is just an empty path with a trailing "/", so it will be covered by the next line afaics (comments should also change to reflect).
There was a problem hiding this comment.
The removeTrailingPathSepChar() only removes the "/" if dir.length() > 1:
inline StringBuffer &removeTrailingPathSepChar(StringBuffer &path)
{
if (path.length()>1 && isPathSepChar(path.charAt(path.length()-1)))
{
Also add code to change the path to a relative path if needed. Signed-off-by: wangkx <[email protected]>
|
@ghalliday this PR has been approved. You may merge it. |
|
@ghalliday I noticed the test failure for workflow_9c.ecl. I just tested: 'ecl run roxie /home/lexis/src/HPCC-Platform/testing/regress/ecl/workflow_9c.ecl' in my local bare metal environment. No failure. |
ghalliday
merged commit 34941fa
into
hpcc-systems:candidate-9.0.x
Also add code to change the path to a relative path if needed.
Type of change:
Checklist:
Smoketest:
Testing: