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 for issue #7719: The "Aux file" on "Edit group" doesn't support relative sub-directories path #7728

Closed
wants to merge 8 commits into from

Conversation

devinluo27
Copy link
Contributor

@devinluo27 devinluo27 commented May 10, 2021

Fixes #7719

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

Summary of Issue 7719

The "Aux file" on "Edit group" doesn't work when users are inputing a file with relative path. When users are entering, the validator is also checking. In previous version, in this line, the validator assumes the work directory to be like \home\xxxx. However, in DefaultFileUpdateMonitor.java, it treats it as a file inside the Jabref directory! Since in most cases, Jabref directory is different from the work directory, in DefaultFileUpdateMonitor.java, variable directory has undesirable value and causes the exception.

Way to Fix

I have checked many files and find that the best way to handle this inconsistence is to add workingDir to filepath if it is relative, as shown in my pr. Now, we make sure that the path in this line in always a right absolute path and it can avoid the exception mentioned in this issue.

@devinluo27
Copy link
Contributor Author

I will try to add some test cases and modify the CHANGELOG.md later.

resultingGroup = TexGroup.create(
groupName,
groupHierarchySelectedProperty.getValue(),
Path.of(texGroupFilePathProperty.getValue().trim()),

Choose a reason for hiding this comment

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

Thank you for looking at the issue!

Perhaps the changes should be added with

private Path expandPath(Path path) {
List<Path> fileDirectories = getFileDirectoriesAsPaths();
return FileHelper.find(path.toString(), fileDirectories).orElse(path);
}
private List<Path> getFileDirectoriesAsPaths() {
return metaData.getLatexFileDirectory(user)
.map(List::of)
.orElse(Collections.emptyList());
}

instead. Wouldn't they break paths relative to the "latex directory" otherwise? (refs #4792)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, thanks for your suggestion. Do you mean that it is better to add the preferencesService.getWorkingDir() to file path inexpandPath(Path path) method instead of in jabref/src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java. Could you explain it a little further?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my modification, this.filePath is always absolute. I thinks it is ok with FileHelper.find(path.toString(), fileDirectories).orElse(path); .

Copy link
Member

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 May 13, 2021

Choose a reason for hiding this comment

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

Quite frankly I am not really sure what is going on with getWorkingDir(), but the "working dir" is changed if you open a different library which is strange behavior.

I think you are right about it not breaking the "latex directory". There is a bug in the texGroupFilePathValidator that makes it not work as I'd expect (that is unrelated to your changes).

Do you mean that it is better to add the preferencesService.getWorkingDir() to file path inexpandPath(Path path) method instead of in jabref/src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java. Could you explain it a little further?

I think so. I'll try to think a bit more about it, but I would prefer if someone in the future will be able to use the TexGroup.expandPath method (or a similar method) to fix the bug in texGroupFilePathValidator. That the path to a valid absolute or relative .aux-file is created in the same method (DRY).

Choose a reason for hiding this comment

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

What are your thoughts about using the currentDatabase.getDatabasePath()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I print "working dir" out and it always turns out to be the home dir for current user(Mac OS).

I have tried some input and find that currentDatabase.getDatabasePath() is empty. I am not quite sure if metaData.getLatexFileDirectory(user) and currentDatabase.getDatabasePath() are the same. I think explicitly setting currentDatabase.getDatabasePath() to working dir doesn't sound good as well.

There is also another way to fix it. Remove the automatically added "working dir" in texGroupFilePathValidator and always let users input a absolute path or use the browser button. This can make it simple, since probably getWorkingDir()'s behaviour is wired.

Choose a reason for hiding this comment

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

I am not quite sure if metaData.getLatexFileDirectory(user) and currentDatabase.getDatabasePath() are the same.

  • getLatexFileDirectory can be manually set by the user and is intended to allow relative paths to a user-specified directory (the menu Library -> Library properties) for .aux files
  • getDatabasePath should give you the path to the .bib file. It should be used by the "Reveal in file explorer" function. (From the JavaDoc) Get the path where this database was last saved to or loaded from, if any.

I think explicitly setting currentDatabase.getDatabasePath() to working dir doesn't sound good as well.

Agreed.

There is also another way to fix it. Remove the automatically added "working dir" in texGroupFilePathValidator and always let users input a absolute path or use the browser button. This can make it simple, since probably getWorkingDir()'s behaviour is wired.

I think you are right in that the "working dir" should be removed from the texGroupFilePathValidator. Perhaps its behavior can be made more aligned with

public void texGroupBrowse() {
FileDialogConfiguration fileDialogConfiguration = new FileDialogConfiguration.Builder()
.addExtensionFilter(StandardFileType.AUX)
.withDefaultExtension(StandardFileType.AUX)
.withInitialDirectory(preferencesService.getWorkingDir()).build();
dialogService.showFileOpenDialog(fileDialogConfiguration)
.ifPresent(file -> texGroupFilePathProperty.setValue(
FileUtil.relativize(file.toAbsolutePath(), getFileDirectoriesAsPaths()).toString()
));
}

It seems like there is a wish to be able to use relative paths, which were the intent of the latex file directory. What are your thoughts about attempting to get it working again and updating the documentation?
I don't know if there is a "default" directory it would make sense to look for .aux files in, but perhaps it isn't too much to ask of a user to specify that directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have set the base for relative filePath as getLatexFileDirectory() and I have try some test cases manually and it can work. Point it out if there is any problem.

@koppor
Copy link
Member

koppor commented May 17, 2021

The general concept of JabRef's directories is written down at https://docs.jabref.org/fields/filelinks#directories-for-files. Does that help?

@devinluo27
Copy link
Contributor Author

The general concept of JabRef's directories is written down at https://docs.jabref.org/fields/filelinks#directories-for-files. Does that help?
Thanks. The description is clear and I have set the base directory to .aux file as LatexFileDirextory in my pr.

Copy link
Member

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 left a comment

Choose a reason for hiding this comment

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

My apologies for the late re-review. Mostly looks good already, but I'd ask for some minor changes.

@devinluo27 devinluo27 force-pushed the fix_for_7719 branch 2 times, most recently from a779428 to 57f1861 Compare May 20, 2021 15:32
Copy link
Member

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 left a comment

Choose a reason for hiding this comment

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

This looks good to me, and it solves the linked issue.

Two minor things for your next PR,

  1. In the description of your PR I added "Fixes The "Aux file" on "Edit group" doesn't support relative sub-directories path #7719". On GitHub, this automatically links the PR with the issue, please do that next time.
  2. Try to follow the commit guidelines by writing meaningful commit messages.

If the validation of relative paths becomes a problem again, a later PR can merge GroupDialogViewModel.getAbsoluteTexGroupPath with TexGroup.getFilePath so that the same code is being used by both methods. I believe it would require some refactoring to make it fit within MVVM so I'd argue it is out of scope for this PR (perhaps TexGroup contain too much logic).

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 20, 2021
Co-authored-by: Jonatan Asketorp <2598631+k3KAW8Pnf7mkmdSMPHz27@users.noreply.github.com>
@koppor
Copy link
Member

koppor commented May 20, 2021

I will try to add some test cases and modify the CHANGELOG.md later.

My I ask for test cases? The file handling is a very fragile thing - and used by many users. It should not break if code in that area is changed. Thus, test cases covering different cases should be there. Would you mind adding test cases? Maybe using a temporary directory as outlined at https://www.baeldung.com/junit-5-temporary-directory?

Can it be possible that input is a relative path and that getLatexFileDirectory is not present? What should be the expected behavior?

In the current case, it is undocumented - and a relative path is returned. Does that work for a user?

    /**
     * Gets the absolute path relative to the LatexFileDirectory, if given a relative path
     * @param input the user input path
     * @return an absolute path
     */
    private Path getAbsoluteTexGroupPath(String input) {
        Optional<Path> latexFileDirectory = currentDatabase.getMetaData().getLatexFileDirectory(preferencesService.getUser());
        return latexFileDirectory.map(path -> path.resolve(input)).orElse(Path.of(input));
    }

@koppor
Copy link
Member

koppor commented May 20, 2021

Note that I commited @k3KAW8Pnf7mkmdSMPHz27's suggestion and resolved the conflict in CHANGELOG.md. Thus, please pull the changes before working 😇

@devinluo27
Copy link
Contributor Author

Note that I commited @k3KAW8Pnf7mkmdSMPHz27's suggestion and resolved the conflict in CHANGELOG.md. Thus, please pull the changes before working 😇

Already pull the changes. I make a minor change in getAbsoluteTexGroupPath(). Now, if TexGroupPath is not set and the user input a relative file path, it will be rejected by validator, since in this case, we don't have a root for the relative path.

@k3KAW8Pnf7mkmdSMPHz27

This comment has been minimized.

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Member

Disregard my last comment. I guess the "correct" approach would be to mock most of the items required to create a GroupDialogViewModel, hence avoid making a GUI test.

@devinluo27
Copy link
Contributor Author

I think suggestions from @k3KAW8Pnf7mkmdSMPHz27 could work. I just got plenty of stuff to do recently, could anyone help out on the test cases?

@Siedlerchr
Copy link
Member

I am currenty looking into how to test

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Member

k3KAW8Pnf7mkmdSMPHz27 commented May 26, 2021

@Aloofluo will you be able to work on this at a later time (when you have less stuff to do)?

package org.jabref.gui.groups;

import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Optional;

import org.jabref.gui.DialogService;
import org.jabref.model.database.BibDatabaseContext;
import org.jabref.model.groups.AbstractGroup;
import org.jabref.model.metadata.MetaData;
import org.jabref.preferences.PreferencesService;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

class GroupDialogViewModelTest {
    private GroupDialogViewModel viewModel;
    private Path temporaryFolder;
    private BibDatabaseContext bibDatabaseContext;

    @BeforeEach
    void setUp(@TempDir Path temporaryFolder) throws Exception {
        this.temporaryFolder = temporaryFolder;
        bibDatabaseContext = new BibDatabaseContext();
        DialogService dialogService = mock(DialogService.class);

        AbstractGroup group = mock(AbstractGroup.class);
        when(group.getName()).thenReturn("Group");

        PreferencesService preferencesService = mock(PreferencesService.class);
        when(preferencesService.getKeywordDelimiter()).thenReturn(',');
        when(preferencesService.getUser()).thenReturn("MockedUser");

        MetaData metaData = mock(MetaData.class);
        when(metaData.getLatexFileDirectory(any(String.class))).thenReturn(Optional.empty());
        bibDatabaseContext.setMetaData(metaData);

        viewModel = new GroupDialogViewModel(dialogService, bibDatabaseContext, preferencesService, group);
    }

    @Test
    void validateExistingAbsolutePath() throws Exception {
        var anAuxFile = temporaryFolder.resolve("auxfile.aux").toAbsolutePath();
        Files.createFile(anAuxFile);
        viewModel.texGroupFilePathProperty().setValue(anAuxFile.toString());
        assertTrue(viewModel.texGroupFilePathValidatonStatus().isValid());
    }

    @Test
    void validateNonExistingAbsolutePath() throws Exception {
        var notAnAuxFile = temporaryFolder.resolve("notanauxfile.aux").toAbsolutePath();
        viewModel.texGroupFilePathProperty().setValue(notAnAuxFile.toString());
        assertFalse(viewModel.texGroupFilePathValidatonStatus().isValid());
    }
}

This is a bit of extra added to what @Siedlerchr looked into. I believe this is enough "mocking" to get it working, but it still needs relevant test cases, and some changes to return the temporaryFolder on some calls to getLatexFileDirectory.

@devinluo27
Copy link
Contributor Author

@k3KAW8Pnf7mkmdSMPHz27 Thanks for your work. I would have a try on crafting test cases in June.

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 added status: changes required Pull requests that are not yet complete and removed status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels May 28, 2021
@calixtus calixtus added the status: waiting-for-feedback The submitter or other users need to provide more information about the issue label Jun 3, 2021
@Siedlerchr
Copy link
Member

@Aloofluo It would be really great if you could finish this PR. If I recall correctly, it's just the test left. So it would be nice to have this finished.

@devinluo27
Copy link
Contributor Author

@Aloofluo It would be really great if you could finish this PR. If I recall correctly, it's just the test left. So it would be nice to have this finished.

Sorry for the delay. Thanks for the added tests.

@Siedlerchr
Copy link
Member

I was so free to do a follow up PR with the tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: changes required Pull requests that are not yet complete status: waiting-for-feedback The submitter or other users need to provide more information about the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The "Aux file" on "Edit group" doesn't support relative sub-directories path
5 participants