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

Exception if linked file has masked umlauts / invalid characters in path (from Citavi export in my case) #8786

Closed
2 tasks done
claell opened this issue May 11, 2022 · 14 comments · Fixed by #8851
Closed
2 tasks done
Assignees
Labels
component: external-files component: import-load good first issue An issue intended for project-newcomers. Varies in difficulty. [outdated] type: bug Confirmed bugs or reports that are very likely to be bugs

Comments

@claell
Copy link
Contributor

claell commented May 11, 2022

JabRef version

5.6 (latest release)

Operating system

Windows

Details on version and operating system

Windows 10

Checked with the latest development build

  • I made a backup of my libraries before testing the latest development version.
  • I have tested the latest development version and the problem persists

Steps to reproduce the behaviour

  1. Have a bibliography with file links that contain masked umlauts (probably this shouldn't happen in the first place, but apparently Citavi exports that way)
  2. Open the entry view for it in JabRef and click on the General tab
  3. Get an exception (see appendix)

Appendix

grafik

Log File
java.nio.file.InvalidPathException: Illegal char <"> at index 69: Attachments/Generatives Design zur Optimierung additiv gefertigter K{"u}hlk{"o}rper (2021).pdf
	at java.base/sun.nio.fs.WindowsPathParser.normalize(Unknown Source)
	at java.base/sun.nio.fs.WindowsPathParser.parse(Unknown Source)
	at java.base/sun.nio.fs.WindowsPathParser.parse(Unknown Source)
	at java.base/sun.nio.fs.WindowsPath.parse(Unknown Source)
	at java.base/sun.nio.fs.WindowsFileSystem.getPath(Unknown Source)
	at java.base/java.nio.file.Path.resolve(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.model.util.FileHelper.find(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.model.util.FileHelper.lambda$find$0(Unknown Source)
	at java.base/java.util.stream.ReferencePipeline$7$1.accept(Unknown Source)
	at java.base/java.util.ArrayList$ArrayListSpliterator.tryAdvance(Unknown Source)
	at java.base/java.util.stream.ReferencePipeline.forEachWithCancel(Unknown Source)
	at java.base/java.util.stream.AbstractPipeline.copyIntoWithCancel(Unknown Source)
	at java.base/java.util.stream.AbstractPipeline.copyInto(Unknown Source)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(Unknown Source)
	at java.base/java.util.stream.FindOps$FindOp.evaluateSequential(Unknown Source)
	at java.base/java.util.stream.AbstractPipeline.evaluate(Unknown Source)
	at java.base/java.util.stream.ReferencePipeline.findFirst(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.model.util.FileHelper.find(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.model.util.FileHelper.find(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.gui.fieldeditors.LinkedFileViewModel.lambda$new$0(Unknown Source)
	at org.jabref.merged.module@5.6.60000/de.saxsys.mvvmfx.utils.validation.FunctionBasedValidator.lambda$new$2(Unknown Source)
	at org.jabref.merged.module@5.6.60000/de.saxsys.mvvmfx.utils.validation.FunctionBasedValidator.validate(Unknown Source)
	at org.jabref.merged.module@5.6.60000/de.saxsys.mvvmfx.utils.validation.FunctionBasedValidator.<init>(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.gui.fieldeditors.LinkedFileViewModel.<init>(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.gui.fieldeditors.LinkedFilesEditorViewModel.lambda$parseToFileViewModel$1(Unknown Source)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(Unknown Source)
	at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(Unknown Source)
	at java.base/java.util.stream.AbstractPipeline.copyInto(Unknown Source)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(Unknown Source)
	at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(Unknown Source)
	at java.base/java.util.stream.AbstractPipeline.evaluate(Unknown Source)
	at java.base/java.util.stream.ReferencePipeline.collect(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.gui.fieldeditors.LinkedFilesEditorViewModel.parseToFileViewModel(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.gui.util.BindingsHelper.lambda$bindContentBidirectional$4(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.gui.util.BindingsHelper$BidirectionalListBinding.changed(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.binding.ExpressionHelper$Generic.fireValueChangedEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.binding.ExpressionHelper.fireValueChangedEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.beans.property.StringPropertyBase.fireValueChangedEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.beans.property.StringPropertyBase.markInvalid(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.beans.property.StringPropertyBase.set(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.beans.property.StringPropertyBase.set(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.beans.property.StringProperty.setValue(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.beans.property.StringProperty.setValue(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.gui.util.BindingsHelper.bindBidirectional(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.gui.util.BindingsHelper.bindBidirectional(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.gui.fieldeditors.AbstractEditorViewModel.bindToEntry(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.gui.fieldeditors.LinkedFilesEditorViewModel.bindToEntry(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.gui.fieldeditors.LinkedFilesEditor.bindToEntry(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.gui.entryeditor.FieldsEditorTab.setupPanel(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.gui.entryeditor.FieldsEditorTab.bindToEntry(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.gui.entryeditor.EntryEditorTab.notifyAboutFocus(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.gui.entryeditor.EntryEditor.setEntry(Unknown Source)
	at java.base/java.util.Optional.ifPresent(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.gui.LibraryTab.lambda$createMainTable$15(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.collections.ListListenerHelper$Generic.fireValueChangedEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.collections.ListListenerHelper.fireValueChangedEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.collections.ObservableListBase.fireChange(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.collections.ListChangeBuilder.commit(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.collections.ListChangeBuilder.endChange(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.collections.ObservableListBase.endChange(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.scene.control.SelectedItemsReadOnlyObservableList.lambda$new$0(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.collections.ListListenerHelper$Generic.fireValueChangedEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.collections.ListListenerHelper.fireValueChangedEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.collections.ObservableListBase.fireChange(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.collections.ListChangeBuilder.commit(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.collections.ListChangeBuilder.endChange(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.collections.ObservableListBase.endChange(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.scene.control.ReadOnlyUnbackedObservableList._endChange(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.scene.control.MultipleSelectionModelBase$SelectedIndicesList._endChange(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.scene.control.ControlUtils.updateSelectedIndices(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.scene.control.TableView$TableViewArrayListSelectionModel.fireCustomSelectedCellsListChangeEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.scene.control.TableView$TableViewArrayListSelectionModel.clearAndSelect(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.scene.control.TableView$TableViewSelectionModel.clearAndSelect(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.scene.control.behavior.TableCellBehaviorBase.simpleSelect(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.scene.control.behavior.TableCellBehaviorBase.doSelect(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.scene.control.behavior.CellBehaviorBase.mousePressed(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.scene.control.inputmap.InputMap.handle(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.CompositeEventHandler$NormalEventHandlerRecord.handleBubblingEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.CompositeEventHandler.dispatchBubblingEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.CompositeEventDispatcher.dispatchBubblingEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.EventUtil.fireEventImpl(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.event.EventUtil.fireEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.event.Event.fireEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.scene.Scene$MouseHandler.process(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.scene.Scene.processMouseEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/javafx.scene.Scene$ScenePeerListener.mouseEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.tk.quantum.GlassViewEventHandler$MouseEventNotification.run(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.tk.quantum.GlassViewEventHandler$MouseEventNotification.run(Unknown Source)
	at java.base/java.security.AccessController.doPrivileged(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.tk.quantum.GlassViewEventHandler.lambda$handleMouseEvent$2(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.tk.quantum.QuantumToolkit.runWithoutRenderLock(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.javafx.tk.quantum.GlassViewEventHandler.handleMouseEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.glass.ui.View.handleMouseEvent(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.glass.ui.View.notifyMouse(Unknown Source)
	at org.jabref.merged.module@5.6.60000/com.sun.glass.ui.win.WinApplication._runLoop(Native Method)
	at org.jabref.merged.module@5.6.60000/com.sun.glass.ui.win.WinApplication.lambda$runLoop$3(Unknown Source)
	at java.base/java.lang.Thread.run(Unknown Source)
@Siedlerchr
Copy link
Member

Hm. JabRef can't do anything about this when the file name is illegal...
For citavi, we plan an importer Refs #8322

@claell
Copy link
Contributor Author

claell commented May 11, 2022

Yes, I understand that.

I was mainly talking about the currently uncaught exception in such cases (which doesn't provide a good UX).

@claell claell changed the title Exception if linked file has masked umlauts (from Citavi export in my case) Exception if linked file has masked umlauts / invalid characters in path (from Citavi export in my case) May 12, 2022
@ThiloteE ThiloteE added component: import-load [outdated] type: bug Confirmed bugs or reports that are very likely to be bugs and removed component: import-load labels May 12, 2022
@ThiloteE
Copy link
Member

@claell I am not sure what to do with this. Yes, we see there is a bug, but could you explain more about what you expect to be done with the UI? I think there was another issue that also complained about better error message UI. Maybe it should be discussed over there and this one should stay open as reference to the citavi importer?

@Siedlerchr
Copy link
Member

Siedlerchr commented May 12, 2022

@ThiloteE Sorry, but this has nothing do with java special character. It's that Windows doesn't allow these characters in a fie name https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file and java
And Java just passes the path down to Windows, which then results in the exception

Relevant part:

at org.jabref@5.6.60000/org.jabref.model.util.FileHelper.find(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.model.util.FileHelper.find(Unknown Source)
	at org.jabref@5.6.60000/org.jabref.gui.fieldeditors.LinkedFileViewModel.lambda$new$0(Unknown Source)

@ThiloteE
Copy link
Member

Thanks for the explanation Christoph.

Similar (probably also unresolved) problem in #7381

If the filename contains something like {"{O}} instead of Ö, the same error is thrown. I migrated from Mendeley to JabRef and my bib file was generated by Mendeley originally.

@Siedlerchr
Copy link
Member

Maybe we can also run Latex2Unicode on the file field...

@claell
Copy link
Contributor Author

claell commented May 13, 2022

Hm, yes looks exactly as #7381 (#7381 (comment) also mentions that only the endless loops were fixed, while the issue still happens when accessing the entry information).

@claell
Copy link
Contributor Author

claell commented May 13, 2022

The question is: Does JabRef want to support such things? Is this supported by BibTeX? Else it should probably show a warning, possibly also propose a fix.

However, if BibTeX allows this stuff, then JabRef should work with it (possibly propose a change for better conformity with JabRef and consistency with the rest of the library).

@Siedlerchr
Copy link
Member

@claell Well, the path contains illegal characters in this way and JabRef (when generating file names) ensures it contains no illegal chars for every platform to make sure it's cross-platform, but does not check any existing files name fields.
I suggest a combination of approaches:

Solution:

  1. Check for any exception occurring:

    Path resolvedFile = directory.resolve(fileName);
    if (Files.exists(resolvedFile)) {
    return Optional.of(resolvedFile);
    } else {

  2. Log the exception with a LOGGER.error

  3. Return empty Optional

Additionally: Add an integrity checker if any file name contains illegal chars
The code is similar to one of the cleaner:

/**
* Replaces illegal characters in given directoryName by '_'.
* Directory name may contain directory separators, e.g. 'deep/in/a/tree'; these are left untouched.
*
* @param badFileName the fileName to clean
* @return a clean filename
*/
public static String cleanDirectoryName(String badFileName) {
StringBuilder cleanName = new StringBuilder(badFileName.length());
for (int i = 0; i < badFileName.length(); i++) {
char c = badFileName.charAt(i);
if (FileNameCleaner.isCharLegal(c)) {

@Siedlerchr Siedlerchr added the good first issue An issue intended for project-newcomers. Varies in difficulty. label May 13, 2022
@the-star-sea
Copy link
Contributor

Can I do this job?I am a new bee of opensource

@ThiloteE
Copy link
Member

Sure :-)

As a general advice: Check out https://github.com/JabRef/jabref/blob/main/CONTRIBUTING.md for a start. Also, https://devdocs.jabref.org/getting-into-the-code/guidelines-for-setting-up-a-local-workspace is worth having a look at. Feel free to ask, if you have any questions here on GitHub or also at JabRef's Gitter chat.

Try to open a (draft) pull request early on, so that people can see you are working on the issue and so that they can see the direction the pull request is heading towards. This way, you will likely receive valuable feedback.

@the-star-sea
Copy link
Contributor

Hello, could you assign this to me? I think this issue is quite interesting.

@the-star-sea
Copy link
Contributor

the-star-sea commented May 26, 2022

I have code as the instruction.But how can I verify it?

/**
     * Detect illegal characters in given filename.
     *
     * @param fileName the fileName to detect
     * @return Boolean whether there is a illegal name
     */
    public static Boolean detectBadFileName(String fileName) {
        for (int i = 0; i < fileName.length(); i++) {
            char c = fileName.charAt(i);
            if (!isCharLegal(c)) {
                return true;
            }
        }
        return false;
    }

    private static boolean isCharLegal(char c) {
         int[] ILLEGAL_CHARS = {
                 0, 1, 2, 3, 4, 5, 6, 7, 8, 9,
                 10, 11, 12, 13, 14, 15, 16, 17, 18, 19,
                 20, 21, 22, 23, 24, 25, 26, 27, 28, 29,
                 30, 31, 34,
                 42,
                 58,
                 60, 62, 63,
                 123, 124, 125
        };
        return Arrays.binarySearch(ILLEGAL_CHARS, c) < 0;
    }
    /**
     * Converts a relative filename to an absolute one, if necessary. Returns
     * an empty optional if the file does not exist.
     */
    public static Optional<Path> find(String fileName, Path directory) {
        Objects.requireNonNull(fileName);
        Objects.requireNonNull(directory);
        // Explicitly check for an empty String, as File.exists returns true on that empty path, because it maps to the default jar location
        // if we then call toAbsoluteDir, it would always return the jar-location folder. This is not what we want here
        if (detectBadFileName(fileName)) {
            LOGGER.error("Invalid characters in path");
            return Optional.empty();
        }

@the-star-sea
Copy link
Contributor

here are my tests.All are passed.I wanna do a pull request
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: external-files component: import-load good first issue An issue intended for project-newcomers. Varies in difficulty. [outdated] type: bug Confirmed bugs or reports that are very likely to be bugs
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants