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

Add connection check #6565

Closed
wants to merge 6 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 @@ -27,6 +27,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
- We added support for jumping to target entry when typing letter/digit after sorting a column in maintable [#6146](https://github.com/JabRef/jabref/issues/6146)
- We added a new fetcher to enable users to search all available E-Libraries simultaneously. [koppor#369](https://github.com/koppor/jabref/issues/369)
- We added the field "entrytype" to the export sort criteria [#6531](https://github.com/JabRef/jabref/pull/6531)
- We added connection check function in network preference setting [#6560](https://github.com/JabRef/jabref/issues/6560)

### Changed

Expand Down
2 changes: 2 additions & 0 deletions src/main/java/org/jabref/gui/preferences/NetworkTab.fxml
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,7 @@
GridPane.rowIndex="5"/>
<Label fx:id="proxyAttentionLabel" styleClass="warning-message"
text="%Attention: Password is stored in plain text!" GridPane.columnIndex="2" GridPane.rowIndex="5"/>
<Button fx:id="checkConnectionButton" text="%Check connection" onAction="#checkConnection"
prefWidth="200.0" GridPane.columnIndex="1" GridPane.rowIndex="6"/>
</GridPane>
</fx:root>
6 changes: 6 additions & 0 deletions src/main/java/org/jabref/gui/preferences/NetworkTabView.java
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public class NetworkTabView extends AbstractPreferenceTabView<NetworkTabViewMode
@FXML private Label proxyPasswordLabel;
@FXML private CustomPasswordField proxyPassword;
@FXML private Label proxyAttentionLabel;
@FXML private Button checkConnectionButton;

private String proxyPasswordText = "";
private int proxyPasswordCaretPosition = 0;
Expand Down Expand Up @@ -120,4 +121,9 @@ private void proxyPasswordMask(MouseEvent event) {
proxyPasswordCaretPosition = 0;
}
}

@FXML
void checkConnection() {
viewModel.checkConnection();
}
}
58 changes: 58 additions & 0 deletions src/main/java/org/jabref/gui/preferences/NetworkTabViewModel.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.jabref.gui.preferences;

import java.net.MalformedURLException;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
Expand All @@ -8,13 +9,19 @@
import javafx.beans.property.SimpleBooleanProperty;
import javafx.beans.property.SimpleStringProperty;
import javafx.beans.property.StringProperty;
import javafx.scene.control.ButtonType;
import javafx.scene.control.DialogPane;
import javafx.scene.control.Label;
import javafx.scene.control.TextField;
import javafx.scene.layout.GridPane;

import org.jabref.Globals;
import org.jabref.gui.DialogService;
import org.jabref.gui.remote.JabRefMessageHandler;
import org.jabref.logic.l10n.Localization;
import org.jabref.logic.net.ProxyPreferences;
import org.jabref.logic.net.ProxyRegisterer;
import org.jabref.logic.net.URLDownload;
import org.jabref.logic.remote.RemotePreferences;
import org.jabref.logic.remote.RemoteUtil;
import org.jabref.model.strings.StringUtil;
Expand All @@ -25,6 +32,7 @@
import de.saxsys.mvvmfx.utils.validation.ValidationMessage;
import de.saxsys.mvvmfx.utils.validation.ValidationStatus;
import de.saxsys.mvvmfx.utils.validation.Validator;
import kong.unirest.UnirestException;

public class NetworkTabViewModel implements PreferenceTabViewModel {
private final BooleanProperty remoteServerProperty = new SimpleBooleanProperty();
Expand Down Expand Up @@ -215,6 +223,56 @@ public boolean validateSettings() {
return true;
}

/**
* Check the connection by using the given url. Used for validating the http proxy.
* The checking result will be appear when request finished.
* The checking result could be either success or fail, if fail, the cause will be displayed.
*/
public void checkConnection() {
DialogPane dialogPane = new DialogPane();
GridPane settingsPane = new GridPane();
settingsPane.setHgap(4.0);
settingsPane.setVgap(4.0);

Label messageLabel = new Label();
messageLabel.setText(Localization.lang("Warning: your settings will be saved.\nEnter any URL to check connection to:"));
Copy link
Member

Choose a reason for hiding this comment

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

Why the Warning: your settings will be saved.? I would just remove that.


TextField url = new TextField();
url.setText("http://");

settingsPane.add(messageLabel, 1, 0);

settingsPane.add(url, 1, 1);
dialogPane.setContent(settingsPane);
String title = Localization.lang("Check Proxy Setting");
dialogService.showCustomDialogAndWait(
title,
dialogPane,
ButtonType.OK, ButtonType.CANCEL)
.ifPresent(btn -> {
if (btn == ButtonType.OK) {
String connectionProblemText = Localization.lang("Problem with connection: ");
String connectionSuccessText = Localization.lang("Connection Successful!");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
String connectionSuccessText = Localization.lang("Connection Successful!");
String connectionSuccessText = Localization.lang("Connection successful!");

No capitalization in text.

storeProxySettings();
URLDownload dl;
try {
dl = new URLDownload(url.getText());
int connectionStatus = dl.checkConnection();
Copy link
Member

Choose a reason for hiding this comment

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

See below - this is IMHO not a proper design for the check. Moreover, any value in the 2xx class is a success. Thus, I would add a proper abstraction (see comment below).

if (connectionStatus == 200) {
dialogService.showInformationDialogAndWait(title, connectionSuccessText);
} else {
dialogService.showErrorDialogAndWait(title, connectionProblemText + Localization.lang("Request failed with status code ") + connectionStatus);
}
} catch (MalformedURLException e) {
dialogService.showErrorDialogAndWait(title, connectionProblemText + e.getMessage());
} catch (UnirestException e) {
dialogService.showErrorDialogAndWait(title, connectionProblemText + e.getMessage());
}
}
}
);
}

@Override
public List<String> getRestartWarnings() {
return restartWarning;
Expand Down
16 changes: 16 additions & 0 deletions src/main/java/org/jabref/logic/net/URLDownload.java
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import org.jabref.model.util.FileHelper;

import kong.unirest.Unirest;
import kong.unirest.UnirestException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -171,6 +172,21 @@ public String getMimeType() {
return "";
}

/**
* Check the connection by using the HEAD request.
* UnirestException can be thrown for invalid request.
* @return the status code of the response
Copy link
Member

Choose a reason for hiding this comment

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

Please add an empty line above

*/
public int checkConnection() {
Copy link
Member

Choose a reason for hiding this comment

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

This is a new method. So, you can work an a proper interface.

Why not?

Suggested change
public int checkConnection() {
public boolean canBeReached() {

See Item 69: Use exceptions only for exceptional conditions of Effective Java.

You can log the status code - and the exception (debug level). I think, the end user is not interested in the concrete response code, is he?

The mapping of response codes and exceptions to a proper user information is hard. All responses not falling in the class 2xx are errors. Thus, both the codes and the exceptions should be mapped to a user message. -- To design that properly is too hard. Just go for a boolean response.

Unirest.config().setDefaultHeader("User-Agent", "Mozilla/5.0 (Windows; U; WindowsNT 5.1; en-US; rv1.8.1.6) Gecko/20070725 Firefox/2.0.0.6");
try {
int statusCode = Unirest.head(source.toString()).asString().getStatus();
return statusCode;
} catch (UnirestException e) {
Copy link
Member

Choose a reason for hiding this comment

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

(obsolte with the refactoring to Boolean return value)

Why this statement? Can't the try..catch constrcut just be removed and a throws UnirestException added to the method?

throw e;
}
}

public boolean isMimeType(String type) {
String mime = getMimeType();

Expand Down
24 changes: 24 additions & 0 deletions src/test/java/org/jabref/logic/net/URLDownloadTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,20 @@

import java.io.File;
import java.io.IOException;
import java.net.MalformedURLException;
import java.net.URL;
import java.nio.charset.StandardCharsets;
import java.nio.file.Path;

import org.jabref.support.DisabledOnCIServer;

import kong.unirest.UnirestException;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;

public class URLDownloadTest {

Expand Down Expand Up @@ -91,4 +95,24 @@ public void downloadOfHttpsSucceeds() throws IOException {
Path path = ftp.toTemporaryFile();
assertNotNull(path);
}

@Test
public void testCheckConnectionSuccess() throws MalformedURLException {
URLDownload google = new URLDownload(new URL("http://www.google.com"));

int statusCode = google.checkConnection();
assertEquals(200, statusCode);
}

@Test
public void testCheckConnectionFail() throws MalformedURLException {
URLDownload nonsense = new URLDownload(new URL("http://nonsenseadddress"));
try {
int statusCode = nonsense.checkConnection();
fail();
Copy link
Member

Choose a reason for hiding this comment

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

(obsolte with the refactoring to Boolean return value)

Why this statement? Can't the try..catch constrcut just be removed and a throws UnirestException added to the method?

Please use AssertThrows for that. Details: https://howtodoinjava.com/junit5/expected-exception-example/

} catch (UnirestException e) {
return;
}
}

}