-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Websocket server (via HTTP server implementation) #14823
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
Conversation
PR Compliance Guide 🔍(Compliance updated until commit 08ee138)Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label Previous compliance checksCompliance check up to commit c71f355
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||||||||
Repo is 404 - could you make it public - or at least give access to us? ^^ |
|
Nice work - I just added one null check and some |
|
Thanks @koppor I thought I created a public repo.. Now it should be accessible. |
|
Note that your PR will not be reviewed/accepted until you have gone through the mandatory checks in the description and marked each of them them exactly in the format of |
|
I need to create a MADR w/ web sockets vs. http server - refs https://stackoverflow.com/a/36057105/873282 |
| return GrizzlyHttpServerFactory | ||
| .createHttpServer(uri, resourceConfig, serviceLocator); | ||
| // Create server without starting so we can attach add-ons before listeners bind | ||
| HttpServer server = GrizzlyHttpServerFactory.createHttpServer(uri, resourceConfig, /* start */ false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: instead of using /* start */ false, extract the boolean as a constant START=true and use !START here.
|
|
||
| // Clear previous registrations from the static engine to ensure isolation | ||
| WebSocketEngine.getEngine().unregisterAll(); | ||
| WebSocketEngine.getEngine().register("", "/ws", new org.jabref.http.server.ws.JabRefWebSocketApp(serviceLocator)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use import
| // Now start the server | ||
| try { | ||
| server.start(); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use more specific IOException as signature-wise it can't throw any other exception
|
|
||
| // Ensure a RemoteMessageHandler is always available; register a no-op logger handler if none provided | ||
| RemoteMessageHandler existingHandler = serviceLocator.getService(RemoteMessageHandler.class); | ||
| if (existingHandler == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My IntelliJ says, "Condition 'existingHandler == null' is always 'false' "
Can you check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mine also said, but I don't believe ^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to me like getService either always returns a ServiceLocator object or throws a MultiException, but I may be wrong, just skimmed through
| } | ||
| return obj.get(key).getAsString(); | ||
| } catch (Exception e) { | ||
| LOGGER.debug("Failed to parse JSON in WebSocket message: {}", e.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer the full stacktrace
| LOGGER.debug("Failed to parse JSON in WebSocket message: {}", e.toString()); | |
| LOGGER.debug("Failed to parse JSON in WebSocket message", e); |
| return null; | ||
| } | ||
| return obj.get(key).getAsString(); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JsonSyntaxException
|
I tried to implement a simple browser extension and a http echo server using Python - with the help of Zencoder - see It's life at https://github.com/koppor/browser-extension-http-localhost-interaction. No server running:
Server running:
Thus, I don't see any reason to introduce web sockets for the commands Manifest excerpt: "host_permissions": [
"http://localhost:*/*"
],The reason, when Web Sockets should be used, is for the communication from JabRef to the browser. Proposal:
Regarding the implementation of the commands, I am working on following to Mirrir the UiCommands
|
|
I was about to merge #14855 here - and to remove all command handling, because its also possible with REST (now) -- but then nothing was left over... I thought, there was some webSocket variable so that one can send something from JabRef to the plugin... Thus, I convert to draft... Next step: Look at https://github.com/LyzardKing/JabRef-Connector?rgh-link-date=2026-01-14T11%3A15%3A39Z to use the API endpoint of #14855. |
|
Your pull request conflicts with the target branch. Please merge with your code. For a step-by-step guide to resolve merge conflicts, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line. |
|
I thought it would have needed a websocket, but thinking more about it, for one way communication it doesn't really make sense. Sending messages form jabref to the the extension was something I looked into, to check for example if an element already exists, or other information. In the end it didn't seem particularly useful. This PR can be closed then, and I can change the extension to run with the post commands. |
Just for completeness here - and to start an ADR: I am a bit "pedantic" using REST. My aim is that the whole URI design is based on proper resources (see https://martinfowler.com/articles/richardsonMaturityModel.html for details). API: jabref/jabsrv/src/test/rest-api.http Line 94 in a9b48f0
The "thing" is that adding something to a library is very well doable with the API: jabref/jabsrv/src/test/commands.http Line 1 in a9b48f0
Books I like:
outlook: "Proper" resource design will also be challenging for queries of the existence of hundreds of DOIs. Then the URI might get too long - thus |


User description
This replaces #14817
This PR adds a websocket for remote communication. Specifically to link with browsers (replacing the problematic native messaging).
A prototype extension is available at: https://github.com/LyzardKing/JabRef-Connector
Updating the old one required too many changes, so the new version exists (for now).
Steps to test
Running Jabref, in the General Settings page enable the HTTP server
The default port is 23119 (as before) and is the same in the extension.
Steps to test
Mandatory checks
CHANGELOG.mdin a way that is understandable for the average user (if change is visible to the user)PR Type
Enhancement
Description
Add WebSocket server support for remote browser communication
Implement JSON-based command protocol (ping, focus, open, add)
Register RemoteMessageHandler in ServiceLocator for WebSocket handlers
Add comprehensive WebSocket documentation and unit tests
Diagram Walkthrough
File Walkthrough
6 files
Pass CLI message handler to HTTP serverPass message handler when starting HTTP serverAccept and forward RemoteMessageHandler parameterStore and pass RemoteMessageHandler to ServerRegister WebSocket add-on and handler in ServiceLocatorImplement WebSocket application with command handlers2 files
Add WebSocket and servlet module dependenciesAdd Grizzly WebSocket and Jakarta Servlet dependencies1 files
Add unit tests for WebSocket message handling1 files
Document WebSocket endpoint and usage patterns