Feature/url based fetcher#15458
Conversation
…fix null safety
This comment has been minimized.
This comment has been minimized.
|
please avoid closing and reopening prs or creating different ones for the same ones as this creates noise |
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
Code Review by Qodo
|
Sorry! I will keep that in mind. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hi, I'm facing issues with submodule modifications being detected |
Review Summary by QodoAdd URL-based entry creation to New Entry dialog
WalkthroughsDescription• Add "Enter URL" tab to New Entry dialog for creating @Misc entries • Implement GenericUrlBasedFetcher to fetch entries from URLs • Add URL validation with HTTP/HTTPS protocol support • Include comprehensive localization strings and unit tests Diagramflowchart LR
User["User enters URL"] -->|Input validation| Validator["URL Validator<br/>HTTP/HTTPS check"]
Validator -->|Valid URL| Fetcher["GenericUrlBasedFetcher"]
Fetcher -->|Create entry| Entry["@Misc BibEntry<br/>with URL field"]
Entry -->|Import| Library["Add to Library"]
File Changes1. jabgui/src/main/java/org/jabref/gui/newentry/NewEntryDialogTab.java
|
Code Review by Qodo
1. orElse("") in fetcher tests
|
| urlText = new SimpleStringProperty(""); | ||
| urlTextValidator = new FunctionBasedValidator<>(urlText, input -> { | ||
| if (StringUtil.isBlank(input)) { | ||
| return false; | ||
| } | ||
|
|
||
| String normalized = input.trim(); | ||
| String lower = normalized.toLowerCase(Locale.ROOT); | ||
|
|
||
| if (!lower.startsWith("http://") && !lower.startsWith("https://")) { | ||
| normalized = "https://" + normalized; | ||
| } | ||
|
|
||
| return URLUtil.isValidHttpUrl(normalized); | ||
| }, ValidationMessage.error(Localization.lang("Please enter a valid HTTP or HTTPS URL."))); |
There was a problem hiding this comment.
2. Url scheme not stored 🐞 Bug ≡ Correctness
The URL validator accepts schemeless inputs by prepending "https://" only for validation, but the worker/fetcher persists the original text, so entries can be created with invalid URL values (e.g., "example.com"). This breaks the promise of “valid URL” and can lead to non-clickable/unusable URL fields downstream.
Agent Prompt
### Issue description
The URL validator prepends `https://` for validation, but the actual stored URL comes from the original `urlText` value, which may be schemeless. This allows a value to be considered “valid” in the UI while persisting an invalid URL into the created `@Misc` entry.
### Issue Context
- Validation currently normalizes into a local variable only.
- `WorkerLookupUrl` passes the original `urlText` to `GenericUrlBasedFetcher`.
- `GenericUrlBasedFetcher` trims and stores the URL as-is.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/newentry/NewEntryViewModel.java[160-174]
- jabgui/src/main/java/org/jabref/gui/newentry/NewEntryViewModel.java[502-520]
- jablib/src/main/java/org/jabref/logic/importer/fetcher/GenericUrlBasedFetcher.java[16-27]
### Suggested fix
Apply the same normalization used during validation to the value that is persisted, e.g.:
- Normalize in `WorkerLookupUrl.call()` (preferred: single point before fetch/import), and pass `normalized` to the fetcher.
- `normalized = text.trim();`
- If missing scheme, prefix `https://`.
- Or move normalization into `GenericUrlBasedFetcher.fetchEntryFromUrl` so all callers get consistent behavior.
- Optionally update `urlText` with the normalized value so the UI and stored data match exactly.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Your code currently does not meet JabRef's code guidelines. IntelliJ auto format covers some cases. There seem to be issues with your code style and autoformat configuration. Please reformat your code (Ctrl+Alt+L) and commit, then push. |
There was a problem hiding this comment.
This is far away from reviewable...At least please take a look at #15458 (review)
|
|
||
| @FXML private TextArea bibtexText; | ||
|
|
||
| @FXML private Tab tabEnterUrl; |
There was a problem hiding this comment.
You can move this line below @FXML private Tab tabSpecifyBibtex;
| if (urlText != null) { | ||
| Platform.runLater(() -> urlText.requestFocus()); | ||
| } |
There was a problem hiding this comment.
Clipboard content should also be in the text field.
| public String getName() { | ||
| return "Generic URL Fetcher"; | ||
| } |
There was a problem hiding this comment.
Implement getName() in WebFetcher interface.
What's the use of a method if its only called by test?
| import org.jabref.model.entry.field.StandardField; | ||
| import org.jabref.model.entry.types.StandardEntryType; | ||
|
|
||
| public class GenericUrlBasedFetcher { |
There was a problem hiding this comment.
Quote from issue description: "Add one implementation GenericUrlBasedFetcher just generating a @misc entry with URL."
You mark this PR reviewable again without doing anything from #15458 (review)???
| return "Generic URL Fetcher"; | ||
| } | ||
|
|
||
| public List<BibEntry> fetchEntryFromUrl(String url) throws FetcherException { |
There was a problem hiding this comment.
FetcherException is never used?
| String trimmedUrl = url.trim(); | ||
| if (!trimmedUrl.startsWith("http://") && !trimmedUrl.startsWith("https://")) { | ||
| trimmedUrl = "https://" + trimmedUrl; | ||
| } | ||
|
|
||
| BibEntry entry = new BibEntry(StandardEntryType.Misc) | ||
| .withField(StandardField.URL, trimmedUrl); | ||
| return List.of(entry); |
There was a problem hiding this comment.
Too many flaws...
-
It creats ONE entry and puts it into a list? So this method always returns a list containing ONE element?!
-
This class is a "fetcher", but only creates a bibentry with the url? At least it should "fetch" something? Otherwise, is there any useful information for users? Please think about the requirement as a "user": what do you want for this feature?
-
I dont get the point of
trimmedUrl, I think its a very very bad name...Also, you addurl.trim().isEmpty()above, why don't you trim it at the very beginning, so you don't have to trim twice... -
You should at least check if the url is valid in viewmodel...
| } | ||
|
|
||
| return URLUtil.isValidHttpUrl(normalized); | ||
| }, ValidationMessage.error(Localization.lang("Please enter a valid HTTP or HTTPS URL."))); |
There was a problem hiding this comment.
-
Why add
http://in a validator? So you addhttp://for the url and expect validator to return error "Please enter a valid HTTP or HTTPS URL."?| -
What is
lowerandnormalized? Bad naming -
What if users use "http://1234" or even "1234", validity check still passes...
-
Line 169-171 is duplicate with Line 22-24 in GenericUrlBasedFetcher.java
| initializeLookupIdentifier(); | ||
| initializeInterpretCitations(); | ||
| initializeSpecifyBibTeX(); | ||
| initializeEnterUrl(); |
There was a problem hiding this comment.
Can you change the name? "EnterUrl" is the action of end users. Here, it should be something JabRef does.
Also for switchEnterUrl, tabEnterUrl, etc...
|
Plus, can you describe what you have done in commit messages, instead of update which file... |
|
The PR seems too far away from mergeable state plus the contributor is inactive since a week, so closing for future tries. |
|
This pull requests was closed without merging. You have been unassigned from the respective issue #15411. In case you closed the PR for yourself, you can re-open it. Please also check After submission of a pull request in CONTRIBUTING.md. |
Related issues and pull requests
Closes #15411
PR Description
Steps to test
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)Screenshot - Enter URL Tab