Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #10175: set FilesetEntry client path to String <256 chars #1706

Closed
wants to merge 1 commit into from

Conversation

mtbc
Copy link
Member

@mtbc mtbc commented Nov 4, 2013

Fixes http://trac.openmicroscopy.org.uk/ome/ticket/10175. To test, try importing images from absurdly long-named/deep paths. Cc: @manics.

--no-rebase as it's FS-specific.

@mtbc
Copy link
Member Author

mtbc commented Nov 4, 2013

If some of the used files have a shorter full path than the longest ones, this code takes the opportunity to squeeze an extra bit of depth into them. If it is important to start all the used files from the same parent, this PR could be changed accordingly.

String fsPathString;
while (true) {
fsPathString = fsPath.toString();
if (fsPathString.length() < 256) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be defined as a constant/property somewhere? Or is there no point since everything else is hard-coded?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping to figure out how to extract it from whatever's setting the VARCHAR(255) on that column in the first place, but I couldn't find it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.

@manics
Copy link
Member

manics commented Nov 4, 2013

Is it worth warning the user that the path was truncated?

@mtbc
Copy link
Member Author

mtbc commented Nov 4, 2013

I don't think so: they probably won't care about the higher-up path components anyway and we already bother them with quite enough things. One could slip a ".../" at the front if you think it warranted?

@joshmoore
Copy link
Member

I think clientPath must be inviolate since we've sold it as the way to identify filesets despite our munging of server-side paths, even if that means we have to go to using text. /cc @chris-allan, @jburel

@joshmoore
Copy link
Member

NB: Another alternative would be to log all these values in a property file with the fileset:

2013-11-04-fs059
2013-11-04-fs059.log
2013-11-04-fs059.client_paths

in which `client_paths includes:

2013-11-04-fs059/foo/bar.txt=c:\Users\Moore\foo\bar.txt
...

@jburel
Copy link
Member

jburel commented Nov 4, 2013

The main issue is the image name which is limited to 256.
The current strategy is a major change between pre-fs and FS. the image name == client path. This point was raised during Beta1 but we did not have a chance to look at that.

@joshmoore
Copy link
Member

If image.name == entry.clientPath is the problem, then I certainly don't have any objections to changing the image.name strategy, but the modification here is truncation of entry.clientPath, which I see as problematic.

@jburel
Copy link
Member

jburel commented Nov 4, 2013

I agree that we cannot truncate the clientPath, I did not save my last comment.

@mtbc
Copy link
Member Author

mtbc commented Nov 5, 2013

Unacceptable approach.

@mtbc mtbc closed this Nov 5, 2013
@mtbc mtbc deleted the trac-10175-long-image-path branch November 5, 2013 08:16
@mtbc
Copy link
Member Author

mtbc commented Nov 5, 2013

Superseded by #1710.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants