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

Readd query syntax validation #7418

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
- We fixed an issue where the file path is invisible in dark theme. [#7382](https://github.com/JabRef/jabref/issues/7382)
- We fixed an issue where the secondary sorting is not working for some special fields. [#7015](https://github.com/JabRef/jabref/issues/7015)
- We fixed an issue where changing the font size makes the font size field too small. [#7085](https://github.com/JabRef/jabref/issues/7085)
- We fixed an issue where GUI freezes when searching long titles with some characters. [#7411](https://github.com/JabRef/jabref/issues/7411)
- We fixed an issue with TexGroups on Linux systems, where the modification of an aux-file did not trigger an auto-update for TexGroups. Furthermore, the detection of file modifications is now more reliable. [#7412](https://github.com/JabRef/jabref/pull/7412)
- We fixed an issue where the Unicode to Latex formatter produced wrong results for characters with a codepoint higher than Character.MAX_VALUE. [#7387](https://github.com/JabRef/jabref/issues/7387)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,14 @@ protected Node createContentPane() {
// Create text field for query input
TextField query = SearchTextField.create();
query.getStyleClass().add("searchBar");
query.textProperty().addListener((observable, oldValue, newValue) -> viewModel.validateQueryStringAndGiveColorFeedback(query, newValue));
query.focusedProperty().addListener((observable, oldValue, newValue) -> {
if (newValue) {
viewModel.validateQueryStringAndGiveColorFeedback(query, query.getText());
} else {
viewModel.setPseudoClassToValid(query);
}
});

viewModel.queryProperty().bind(query.textProperty());

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
package org.jabref.gui.importer.fetcher;

import java.util.SortedSet;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
import java.util.regex.Pattern;

import javafx.beans.property.ListProperty;
import javafx.beans.property.ObjectProperty;
Expand All @@ -10,11 +14,15 @@
import javafx.beans.property.StringProperty;
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;
import javafx.css.PseudoClass;
import javafx.scene.control.TextField;
import javafx.scene.control.Tooltip;

import org.jabref.gui.DialogService;
import org.jabref.gui.StateManager;
import org.jabref.gui.importer.ImportEntriesDialog;
import org.jabref.gui.util.BackgroundTask;
import org.jabref.gui.util.TaskExecutor;
import org.jabref.logic.importer.ParserResult;
import org.jabref.logic.importer.SearchBasedFetcher;
import org.jabref.logic.importer.WebFetchers;
Expand All @@ -29,8 +37,11 @@ public class WebSearchPaneViewModel {
private final ObjectProperty<SearchBasedFetcher> selectedFetcher = new SimpleObjectProperty<>();
private final ListProperty<SearchBasedFetcher> fetchers = new SimpleListProperty<>(FXCollections.observableArrayList());
private final StringProperty query = new SimpleStringProperty();
private final ExecutorService executor = Executors.newFixedThreadPool(1);
private final DialogService dialogService;
private final StateManager stateManager;
private final Pattern queryPattern;
private final Pattern laxQueryPattern;

public WebSearchPaneViewModel(PreferencesService preferencesService, DialogService dialogService, StateManager stateManager) {
this.dialogService = dialogService;
Expand All @@ -50,6 +61,13 @@ public WebSearchPaneViewModel(PreferencesService preferencesService, DialogServi
int newIndex = fetchers.indexOf(newFetcher);
preferencesService.storeSidePanePreferences(preferencesService.getSidePanePreferences().withWebSearchFetcherSelected(newIndex));
});

String allowedFields = "((author|abstract|journal|title|year|year-range):\\s?)?";
// Either a single word, or a phrase with quotes, or a year-range
String allowedTermText = "(((\\d{4}-\\d{4})|(\\w+)|(\"\\w+[^\"]*\"))\\s?)+";
queryPattern = Pattern.compile("^(" + allowedFields + allowedTermText + ")+$");
String laxFields = "(\\w+:\\s?)?";
laxQueryPattern = Pattern.compile("^(" + laxFields + allowedTermText + ")+$");
}

public ObservableList<SearchBasedFetcher> getFetchers() {
Expand Down Expand Up @@ -91,11 +109,62 @@ public void search() {

BackgroundTask<ParserResult> task;
task = BackgroundTask.wrap(() -> new ParserResult(activeFetcher.performSearch(getQuery().trim())))
.withInitialMessage(Localization.lang("Processing %0", getQuery().trim()));
.withInitialMessage(Localization.lang("Processing %0", getQuery().trim()));
task.onFailure(dialogService::showErrorDialogAndWait);

ImportEntriesDialog dialog = new ImportEntriesDialog(stateManager.getActiveDatabase().get(), task);
dialog.setTitle(activeFetcher.getName());
dialogService.showCustomDialogAndWait(dialog);
}

public void validateQueryStringAndGiveColorFeedback(TextField querySource, String queryString) {
if (!queryString.strip().isBlank() && !isPatternMatched(queryPattern, queryString)) {
if (isPatternMatched(laxQueryPattern, queryString)) {
setPseudoClassToUnsupported(querySource);
querySource.setTooltip(new Tooltip(Localization.lang("This query uses unsupported fields.")));
} else {
setPseudoClassToInvalid(querySource);
querySource.setTooltip(new Tooltip(Localization.lang("This query uses unsupported syntax.")));
}
} else if (containsYearAndYearRange(queryString)) {
setPseudoClassToInvalid(querySource);
querySource.setTooltip(new Tooltip(Localization.lang("The query cannot contain a year and year-range field.")));
} else {
setPseudoClassToValid(querySource);
}
}

private void setPseudoClassToUnsupported(TextField querySource) {
querySource.pseudoClassStateChanged(PseudoClass.getPseudoClass("invalid"), false);
querySource.pseudoClassStateChanged(PseudoClass.getPseudoClass("unsupported"), true);
}

public void setPseudoClassToValid(TextField querySource) {
querySource.pseudoClassStateChanged(PseudoClass.getPseudoClass("invalid"), false);
querySource.pseudoClassStateChanged(PseudoClass.getPseudoClass("unsupported"), false);
}

private void setPseudoClassToInvalid(TextField querySource) {
querySource.pseudoClassStateChanged(PseudoClass.getPseudoClass("invalid"), true);
}

private boolean containsYearAndYearRange(String queryString) {
return queryString.toLowerCase().contains("year:") && queryString.toLowerCase().contains("year-range:");
}

// Use Thread Pool to control the match time
private boolean isPatternMatched(Pattern pattern, String queryString) {
boolean isMatched = true;

try {
BackgroundTask.wrap(() -> pattern.matcher(queryString.strip()).matches())
Copy link
Member

Choose a reason for hiding this comment

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

This should only take a few milliseconds, so I wouldn't use a background task here.

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 this case, it's actually function matches() takes a lot of time that make GUI freeze. When we input a long title, it will take about 2 mintues, which will make GUI freeze

Copy link
Member

Choose a reason for hiding this comment

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

oooh that's surprising. The query parser is quicker, so this shouldn't be a problem any longer once use it.

This comment was marked as off-topic.

Copy link
Member

Choose a reason for hiding this comment

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

If you use the query parser instead of a regex, then it's good to go from my side.

Copy link
Member

Choose a reason for hiding this comment

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

I need to merge a certain number PR due to some personal reasons....

We have some quick win ideas at https://github.com/koppor/jabref/labels/good%20first%20issue. For instance
JabRef#286 should be a quick win.

Actually, I indeed spent a lot of time dealing with this bug.

We spend a lot time implementing the new query syntax and discussing how to unfreeze JabRef in certain cases. When it is possible to use the "query parser" as @tobiasdiez suggested, it should be OK. It is IMHO just about deleting the regex code etc. Then this PR will probably only be some lines of code.

.executeWith((TaskExecutor) executor)
.get(1000 * 10, TimeUnit.MILLISECONDS);
} catch (Exception e) {
isMatched = false;
}

executor.shutdown();
return isMatched;
}
}
3 changes: 3 additions & 0 deletions src/main/resources/l10n/JabRef_en.properties
Original file line number Diff line number Diff line change
Expand Up @@ -2258,6 +2258,9 @@ Remove\ redundant\ spaces=Remove redundant spaces
Replaces\ consecutive\ spaces\ with\ a\ single\ space\ in\ the\ field\ content.=Replaces consecutive spaces with a single space in the field content.
Remove\ digits=Remove digits
Removes\ digits.=Removes digits.
The\ query\ cannot\ contain\ a\ year\ and\ year-range\ field.=The query cannot contain a year and year-range field.
This\ query\ uses\ unsupported\ fields.=This query uses unsupported fields.
This\ query\ uses\ unsupported\ syntax.=This query uses unsupported syntax.

Presets=Presets

Expand Down