Temp document service refactoring changes#541
Conversation
oracle-contributor-agreement
bot
added
the
OCA Verified
| + rq.setMethod(newMethod); | ||
| + } | ||
| + } catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException | NullPointerException ex) { | ||
| + |
There was a problem hiding this comment.
What would be the behaviour of the request if a notebook sends a request for let's day formatter but if that method is not implemented by the notebook then would it come in the catch block?
If yes, then can we add at least a FINE level logger so that exceptions are not swallowed.
There was a problem hiding this comment.
Yes if the method is not implemented we get a JSON-RPC error of UnsupportedRequestMethod exception .
In this try catch however that cannot be caught , here the reflection based errors are being caught . Hence that exception is not being swallowed here .
patches/java-notebooks.diff
Outdated
| private OperationContext initialContext; | ||
| private List<Object> additionalServices = new ArrayList<>(); | ||
| - | ||
| + private static final String NOTEBOOK_TEXT_DOC_URI_IDENTIFIER = "vscode-notebook-cell";// NOI18N |
There was a problem hiding this comment.
If you think vscode-notebook-cell is a property of NotebookDocumentService, then you can create a static member in the NotebookDocumentService and then reference it everywhere for identifier.
| + String newMethod = rq.getMethod().replaceFirst("textDocument", "notebookDocument"); | ||
| + rq.setMethod(newMethod); | ||
| + } | ||
| + } catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException | NullPointerException ex) { |
There was a problem hiding this comment.
What would be the behaviour of the request if a notebook sends a request for let's day formatter but if that method is not implemented by the notebook then would it come in the catch block?
Also, can we add at least a FINE level logger in the catch block so that exceptions are not swallowed.
| + // LSP Client sends requests for notebook cells with textDocument prefix | ||
| + // Our notebookDocumentService implementation requires different implementation for these requests | ||
| + try { | ||
| + Class<?> paramClass = p.getClass(); |
There was a problem hiding this comment.
Is there any alternate approach possible here, apart from using reflection?
There was a problem hiding this comment.
Core idea was to make it generic so in future we can just add new methods in NotebookDocumentServiceHandler
without making any changes in this part of the code . For now we could have just done this delegation for completion request in particular and not do any reflection .
| private static final Logger LOG = Logger.getLogger(LanguageServerImpl.class.getName()); | ||
| private NbCodeClientWrapper client; | ||
| private final TextDocumentServiceImpl textDocumentService = new TextDocumentServiceImpl(this); | ||
| + private final NotebookDocumentServiceHandler notebookDocService = Lookup.getDefault().lookup(NotebookDocumentServiceHandler.class); |
There was a problem hiding this comment.
Perhaps Lookup.getDefault().lookup(NotebookDocumentServiceHandler.class) can be moved into the getNotebookDocumentService() method so that the lookup is performed only when needed. Additionally, if NotebookDocumentServiceHandler is not yet loaded during Server class initialization, the current approach could lead to issues.
patches/java-notebooks.diff
Outdated
| sessionServices.add(inputService.getRegistry()); | ||
| sessionServices.add(workspaceService.getWorkspace()); | ||
| ((LanguageClientAware) getTextDocumentService()).connect(client); | ||
| + ((LanguageClientAware) getNotebookDocumentService()).connect(client); |
There was a problem hiding this comment.
A null check for getNotebookDocumentService() would be good, since we don't contribute our nbcode to Netbeans, but if we contribute the LSP code to Netbeans, it will not find any NotebookDocumentServiceHandler and fail with a NPE before starting the server itself.
patches/java-notebooks.diff
Outdated
|
|
||
| public void init(ClientCapabilities clientCapabilities, ServerCapabilities severCapabilities) { | ||
| + setServerSemanticTokenProvider(clientCapabilities,severCapabilities); | ||
| + setServerNotebookCaps(severCapabilities); |
There was a problem hiding this comment.
Since there is now proper isolation between the TextDocumentService and the NotebookDocumentService, would it be possible to move setServerNotebookCaps into the NotebookDocumentService as well?
I’m not entirely sure whether this is feasible, it’s just a thought, but if it is, it would be good to move this responsibility out as well for better separation of concerns.
| } | ||
| } | ||
| - | ||
| + public void testDelegationForNotebookCells() throws Exception { |
There was a problem hiding this comment.
Can we add one more test, for a method which is not implemented, so that we know that the exception is thrown and caught in the catch block?
There was a problem hiding this comment.
Also, add copyright 2025-2026
shivam71
force-pushed
the
document-service-refactor
branch
from
e68e21a to
35880ed
Compare
2 participants
No description provided.