Caught an error when accessing an invalid path. fixes #10548 #12038
Conversation
There was a problem hiding this comment.
I checked org.jabref.gui.util.FileDialogConfiguration.Builder#withInitialDirectory(java.nio.file.Path). It even checks for existance. - This is also reported at the issue somehow. The catch should be around when calling FileChooser.showOpenMultipleDialog.
Did you try it out? The issue talks about an ssh connection.
Test cases for org.jabref.gui.util.FileDialogConfiguration.Builder#withInitialDirectory(java.nio.file.Path) should be added.
Maybe a JUnit test can wrap https://github.com/google/jimfs and could help.
Maybe, just moving the try/catch around the right position and try out on macOS (!) is enough.
| fileDialogConfiguration = builder | ||
| .withInitialDirectory(getInitialDirectory()) | ||
| .build(); | ||
| } catch (IllegalArgumentException e) { // something went wrong with the directory, fallback to user directory |
There was a problem hiding this comment.
Comment should go one line below
I think, the catch is at the wrong positon. See again the issue
java.lang.IllegalArgumentException: Folder parameter must be a valid folder
at [email protected]/com.sun.glass.ui.CommonDialogs.convertFolder(CommonDialogs.java:239)
at [email protected]/com.sun.glass.ui.CommonDialogs.showFileChooser(CommonDialogs.java:191)
at [email protected]/com.sun.javafx.tk.quantum.QuantumToolkit.showFileChooser(QuantumToolkit.java:1719)
at [email protected]/javafx.stage.FileChooser.showDialog(FileChooser.java:419)
at [email protected]/javafx.stage.FileChooser.showOpenMultipleDialog(FileChooser.java:376)
at [email protected]/org.jabref.gui.JabRefDialogService.showFileOpenDialogAndGetMultipleFiles(JabRefDialogService.java:398)
```
There was a problem hiding this comment.
Yep thats my bad, misread the stacktrace and didnt check it again.
What test cases are you looking for to test withInitialDirectory? There are already test cases present and it seems like they cover all cases? Or are you looking for a test case specific to the directory returned by Directories#getUserDirectory()
There was a problem hiding this comment.
Mabye add a test case for a mounted path? how does it look like and why does it fail?
There was a problem hiding this comment.
The catch needs to be around the open/show methhod
There was a problem hiding this comment.
I'm not sure its possible to simulate a mounted directory in an automated test.
Correct me if I'm wrong, but since mounted directories are handled at the os-level and are treated identically to any other path at the application level, a path with a successful mount is no different to any other directory inside of a unit test, meaning that even if it was possible to simulate one in code, it wouldn't be any different to a regular path?
Did you instead mean that I should test for a failed mounted path?
I'm pretty sure I can use mockito for this to simulate an error being thrown by org.jabref.gui.DialogService#showFileOpenDialogAndGetMultipleFiles(org.jabref.gui.util.FileDialogConfiguration) when it gets called to replicate what happened in the original issue, but I don't see a way that I can write a useful test for org.jabref.gui.util.FileDialogConfiguration.Builder#withInitialDirectory(java.nio.file.Path) as requested.
There was a problem hiding this comment.
@Boston54 Did you try with https://github.com/google/jimfs?
There was a problem hiding this comment.
Yep thats my bad, misread the stacktrace and didnt check it again.
Did you move the block? Mayb,e we can let this through without any additonal test and blindly moving on. But the code should make the impression it could fix the issue.
There was a problem hiding this comment.
@koppor I was looking through jimfs and I've added it to the project, but to me it seems as though it isn't useful for simulating a true mounted directory (but you can 'simulate it' by basically just having a normal path that uses mount naming conventions and using your imagination).
I'm still using it though in the test I've written along with mockio to simulate what happened in the original issue (showFileOpenDialogAndGetMultipleFiles() throwing an IllegalArgumentException).
And yes, I've moved the block to be around the correct area. Since you are okay with not having a test that specifically simulates the mounted directory, I'll have a final check over it soon and then push it.
Siedlerchr
added
the
status: changes required
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.
You can check review dog's comments at the tab "Files changed" of your pull request.
There was a problem hiding this comment.
Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.
You can check review dog's comments at the tab "Files changed" of your pull request.
There was a problem hiding this comment.
Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.
You can check review dog's comments at the tab "Files changed" of your pull request.
There was a problem hiding this comment.
Your code currently does not meet JabRef's code guidelines.
We use OpenRewrite to ensure "modern" Java coding practices.
The issues found can be automatically fixed.
Please execute the gradle task rewriteRun, check the results, commit, and push.
You can check the detailed error output by navigating to your pull request, selecting the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".
There was a problem hiding this comment.
Your code currently does not meet JabRef's code guidelines.
We use OpenRewrite to ensure "modern" Java coding practices.
The issues found can be automatically fixed.
Please execute the gradle task rewriteRun, check the results, commit, and push.
You can check the detailed error output by navigating to your pull request, selecting the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".
|
Looks good to me now, please fix the openRewrite failure and then it's fine |
|
I discarded the changes in the |
There was a problem hiding this comment.
Your code currently does not meet JabRef's code guidelines.
We use OpenRewrite to ensure "modern" Java coding practices.
The issues found can be automatically fixed.
Please execute the gradle task rewriteRun, check the results, commit, and push.
You can check the detailed error output by navigating to your pull request, selecting the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".
|
@Boston54 managed to |
koppor
removed
the
status: changes required
Catches an error caused when the 'open library' button tries to open to a failed mounted directory on MacOS, making the button instead open to the user home directory.
This is achieved by adding a try-catch statement for an
IllegalArgumentExceptioninorg.jabref.gui.importer.actions.OpenDatabaseAction.execute(), avoiding the potential of a race condition between when the path is checked to exist and when it is actually opened. If the path returned bygetInitialDirectory()is inaccessible then the user home directory is used instead, as retrieved byDirectories.getUserDirectory().Closes #10548
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if applicable)