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

improving UX when a http exception is encountered #11490

Merged
merged 48 commits into from
Aug 12, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
3521bd7
Revert "Add missing ""
RehanChalana Jul 5, 2024
75f13fc
Created SimpleHttpResponse class to simplify HTTP Response handling
RehanChalana Jul 13, 2024
d05fb63
Refactored FetcherClientException to use SimpleHttpResponse, allowing…
RehanChalana Jul 13, 2024
fd0cd11
replaced API keys with [REDACTED] in the error messages
RehanChalana Jul 13, 2024
6e91ce3
Created SimpleHttpResponse instance from HttpURLConnection and inclu…
RehanChalana Jul 13, 2024
c4de9f5
improved UX when a http error is encountered
RehanChalana Jul 13, 2024
3c4a722
Update tools.md
RehanChalana Jul 14, 2024
2de617a
check-style fixes
RehanChalana Jul 14, 2024
71ff51c
Merge remote-tracking branch 'origin/issue_11223' into issue_11223
RehanChalana Jul 14, 2024
0919167
check-style fixes
RehanChalana Jul 14, 2024
cad3f7e
moved api_key removal regex to static Pattern with Pattern.compile
RehanChalana Jul 14, 2024
b21a7af
extract max response length to a class constant MAX_RESPONSE_LENGTH
RehanChalana Jul 16, 2024
7a30b01
moved ParserResult to Callable for achieving asynchronous execution
RehanChalana Jul 16, 2024
cf88105
changed the content of dialogue box to include http response and resp…
RehanChalana Jul 17, 2024
c02816e
updated SearchBasedParserFetcher
RehanChalana Jul 17, 2024
0294c11
updated PagedSearchBasedParserFetcher to not expose api keys in the F…
RehanChalana Jul 25, 2024
a0a7643
Merge remote-tracking branch 'origin/main' into issue_11223
koppor Aug 7, 2024
3228418
Add TODO
koppor Aug 7, 2024
de2dfb2
Merge remote-tracking branch 'origin/main' into issue_11223
koppor Aug 10, 2024
84351ef
More intuitive ordering
koppor Aug 10, 2024
95054b1
No error display, but new ParserResult from error
koppor Aug 10, 2024
3abb7a2
MAke use of SimpleHttpResponse
koppor Aug 10, 2024
91440e5
Fix NPE
koppor Aug 10, 2024
e6949bd
Add missing quotes and dots - and fix Citation Style processing l10n
koppor Aug 10, 2024
1d97b76
Fix handling of errors at CiteSeer
koppor Aug 10, 2024
7663f0e
"Fix" display of error messages
koppor Aug 10, 2024
1ab6c2a
Add TODO
koppor Aug 10, 2024
8ee95ce
feat: Added toString to SimpleHttpResponse
RehanChalana Aug 11, 2024
2ba9a6e
refactor: update logger to display SimpleHttpResponse
RehanChalana Aug 11, 2024
2f58945
Merge branch 'main' into issue_11223
koppor Aug 11, 2024
84bc323
Make FetcherException URL-aware
koppor Aug 11, 2024
7801226
Use FetcherException more often
koppor Aug 12, 2024
836b997
Add some debug
koppor Aug 12, 2024
89c845d
Keep response body
koppor Aug 12, 2024
ca3b01c
Move default implementation to "real" class
koppor Aug 12, 2024
61d5334
Merge remote-tracking branch 'origin/main' into issue_11223
koppor Aug 12, 2024
ded5c56
Keep URL and HTTP untranslated
koppor Aug 12, 2024
cb350b0
Delete 2x "ofUrl"
koppor Aug 12, 2024
7687bbd
Remove "ofUrl" and dummy constructors
koppor Aug 12, 2024
394b429
Make FetcherException(URL url, SimpleHttpResponse httpResponse) prote…
koppor Aug 12, 2024
d151e2b
Adapt test
koppor Aug 12, 2024
847c3bf
Fix connection opened twice
koppor Aug 12, 2024
1a3da17
Adapt test
koppor Aug 12, 2024
861d2b0
Merge branch 'main' into issue_11223
koppor Aug 12, 2024
ca979c9
Remove obsolete localization
koppor Aug 12, 2024
d6d16ac
Fix instanceOf Pattern
koppor Aug 12, 2024
3405e21
.formatted works.
koppor Aug 12, 2024
d92e2a5
Add CHANGELOG.md entry
koppor Aug 12, 2024
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
2 changes: 1 addition & 1 deletion docs/code-howtos/tools.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ See also: [https://help.github.com/articles/syncing-a-fork/](https://help.github
(As Administrator - one time)

1. Install [chocolatey](https://chocolatey.org)
2. `choco install git.install -y --params "/GitAndUnixToolsOnPath /WindowsTerminal"`
2. `choco install git.install -y --params "/GitAndUnixToolsOnPath /WindowsTerminal`
Copy link
Member

Choose a reason for hiding this comment

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

are you sure about that? That does not seem right...

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 am sorry i am not aware when i changed the file , must have been a mistake

Copy link
Member

@calixtus calixtus Jul 14, 2024

Choose a reason for hiding this comment

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

no need to be sorry, mistakes happen and we learn. as a general rule, always check your changes before committing them. its easy if you use "git gui" to commit instead of the command line or the integrates tools in the ide.

3. `choco install notepadplusplus`
4. If you want to have your JDK also managed via chocolatey: `choco install temurin`

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import java.util.concurrent.Callable;

import javafx.application.Platform;
import javafx.beans.property.ListProperty;
import javafx.beans.property.ObjectProperty;
import javafx.beans.property.SimpleListProperty;
Expand All @@ -14,11 +15,9 @@
import org.jabref.gui.DialogService;
import org.jabref.gui.StateManager;
import org.jabref.gui.importer.ImportEntriesDialog;
import org.jabref.gui.linkedfile.DownloadLinkedFileAction;
import org.jabref.gui.util.BackgroundTask;
import org.jabref.logic.importer.CompositeIdFetcher;
import org.jabref.logic.importer.ParserResult;
import org.jabref.logic.importer.SearchBasedFetcher;
import org.jabref.logic.importer.WebFetchers;
import org.jabref.logic.importer.*;
subhramit marked this conversation as resolved.
Show resolved Hide resolved
import org.jabref.logic.l10n.Localization;
import org.jabref.model.strings.StringUtil;
import org.jabref.model.util.OptionalUtil;
Expand Down Expand Up @@ -144,7 +143,20 @@ public void search() {
}

SearchBasedFetcher activeFetcher = getSelectedFetcher();
Callable<ParserResult> parserResultCallable = () -> new ParserResult(activeFetcher.performSearch(query));

ParserResult parserResult;
try{
parserResult = new ParserResult(activeFetcher.performSearch(query));
} catch (FetcherException e) {
if(e.getCause()!=null && e.getCause().getCause() instanceof FetcherClientException clientException) {
dialogService.showErrorDialogAndWait(e.getMessage(),clientException.getHttpResponse().responseBody(),clientException);
subhramit marked this conversation as resolved.
Show resolved Hide resolved
} else{
dialogService.showErrorDialogAndWait(e.getMessage(),e);
}
return;
}

Callable<ParserResult> parserResultCallable = () -> parserResult;
String fetcherName = activeFetcher.getName();

if (CompositeIdFetcher.containsValidId(query)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ private void onFailure(URLDownload urlDownload, Exception ex) {
String failedTitle = Localization.lang("Failed to download from URL");
int statusCode;
if (ex instanceof FetcherClientException clientException) {
statusCode = clientException.getStatusCode();
statusCode = clientException.getHttpResponse().statusCode();
if (statusCode == 401) {
dialogService.showInformationDialogAndWait(failedTitle, Localization.lang("401 Unauthorized: Access Denied. You are not authorized to access this resource. Please check your credentials and try again. If you believe you should have access, please contact the administrator for assistance.\nURL: %0 \n %1", urlDownload.getSource(), fetcherExceptionMessage));
} else if (statusCode == 403) {
Expand Down
62 changes: 62 additions & 0 deletions src/main/java/org/jabref/http/dto/SimpleHttpResponse.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package org.jabref.http.dto;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.net.HttpURLConnection;
import java.nio.charset.StandardCharsets;

public record SimpleHttpResponse(int statusCode, String responseBody , String responseMessage) {
subhramit marked this conversation as resolved.
Show resolved Hide resolved

public SimpleHttpResponse(int statusCode,String responseBody,String responseMessage) {
this.statusCode=statusCode;
this.responseBody=truncateResponseBody(responseBody);
this.responseMessage=responseMessage;
}

public SimpleHttpResponse(HttpURLConnection connection) throws IOException {
this(
connection.getResponseCode(),
getResponseBody(connection),
connection.getResponseMessage()
);
}

/**
* Truncates the response body to 1 KB if it exceeds that size.
* Appends "... (truncated)" to indicate truncation.
*
* @param responseBody the original response body
* @return the truncated response body
*/
private static String truncateResponseBody(String responseBody) {
byte[] bytes = responseBody.getBytes(StandardCharsets.UTF_8);
int maxLength = 1024; // 1 KB
subhramit marked this conversation as resolved.
Show resolved Hide resolved
if (bytes.length > maxLength) {
// Truncate the response body to 1 KB and append "... (truncated)"
return new String(bytes, 0, maxLength, StandardCharsets.UTF_8) + "... (truncated)";
subhramit marked this conversation as resolved.
Show resolved Hide resolved
}
// Return the original response body if it's within the 1 KB limit
return responseBody;
}

/**
* Reads the response body from the HttpURLConnection and returns it as a string.
* This method is used to retrieve the response body from the connection,
* which may contain error messages or other information from the server.
*
* @param connection the HttpURLConnection to read the response body from
* @return the response body as a string
* @throws IOException if an I/O error occurs while reading the response body
*/
private static String getResponseBody(HttpURLConnection connection) throws IOException {
try (BufferedReader in = new BufferedReader(new InputStreamReader(connection.getErrorStream()))) {
String inputLine;
StringBuilder content = new StringBuilder();
while ((inputLine = in.readLine()) != null) {
content.append(inputLine);
}
return truncateResponseBody(content.toString());
subhramit marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Original file line number Diff line number Diff line change
@@ -1,18 +1,20 @@
package org.jabref.logic.importer;

import org.jabref.http.dto.SimpleHttpResponse;

/**
* Should be thrown when you encounter an HTTP status code error &gt;= 400 and &lt; 500.
*/
public class FetcherClientException extends FetcherException {
private int statusCode;
private SimpleHttpResponse httpResponse;

public FetcherClientException(String errorMessage, Throwable cause) {
super(errorMessage, cause);
}

public FetcherClientException(String errorMessage, Throwable cause, int statusCode) {
public FetcherClientException(String errorMessage, Throwable cause, SimpleHttpResponse httpResponse) {
super(errorMessage, cause);
this.statusCode = statusCode;
this.httpResponse = httpResponse;
}

public FetcherClientException(String errorMessage) {
Expand All @@ -23,7 +25,13 @@ public FetcherClientException(String errorMessage, String localizedMessage, Thro
super(errorMessage, localizedMessage, cause);
}

public int getStatusCode() {
return statusCode;
public FetcherClientException(String errorMessage,SimpleHttpResponse httpResponse) {
super(errorMessage);
this.httpResponse=httpResponse;
}

public SimpleHttpResponse getHttpResponse() {
return httpResponse;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,13 @@ private List<BibEntry> getBibEntries(URL urlForQuery) throws FetcherException {
fetchedEntries.forEach(this::doPostCleanup);
return fetchedEntries;
} catch (IOException e) {
throw new FetcherException("A network error occurred while fetching from " + urlForQuery, e);
throw new FetcherException(("A network error occurred while fetching from " + urlForQuery).replaceAll
// // Regular expression to redact API keys from the error message
subhramit marked this conversation as resolved.
Show resolved Hide resolved
("(?i)(api[_-]?key)=.*","[REDACTED]"), e);
subhramit marked this conversation as resolved.
Show resolved Hide resolved
} catch (ParseException e) {
throw new FetcherException("An internal parser error occurred while fetching from " + urlForQuery, e);
throw new FetcherException(("An internal parser error occurred while fetching from " + urlForQuery).replaceAll
// Regular expression to redact API keys from the error message
("(?i)(api[_-]?key)=.*","[REDACTED]"), e);
}
}

Expand Down
4 changes: 3 additions & 1 deletion src/main/java/org/jabref/logic/net/URLDownload.java
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import javax.net.ssl.TrustManager;
import javax.net.ssl.X509TrustManager;

import org.jabref.http.dto.SimpleHttpResponse;
import org.jabref.logic.importer.FetcherClientException;
import org.jabref.logic.importer.FetcherServerException;
import org.jabref.logic.util.io.FileUtil;
Expand Down Expand Up @@ -395,8 +396,9 @@ public URLConnection openConnection() throws IOException {
connection = new URLDownload(newUrl).openConnection();
}
if ((status >= 400) && (status < 500)) {
SimpleHttpResponse httpResponse = new SimpleHttpResponse(lConnection);
LOGGER.info("HTTP {}, details: {}, {}", status, lConnection.getResponseMessage(), lConnection.getContentLength() > 0 ? lConnection.getContent() : "");
throw new IOException(new FetcherClientException("Encountered HTTP %s %s".formatted(status, lConnection.getResponseMessage())));
throw new IOException(new FetcherClientException("Encountered HTTP %s %s".formatted(status, lConnection.getResponseMessage()),httpResponse));
}
if (status >= 500) {
LOGGER.info("HTTP {}, details: {}, {}", status, lConnection.getResponseMessage(), lConnection.getContentLength() > 0 ? lConnection.getContent() : "");
Expand Down
Loading