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

replace internals of symbols machinery with more comprehensive model of spring components #1006

Closed
martinlippert opened this issue Mar 15, 2023 · 8 comments

Comments

@martinlippert
Copy link
Member

Instead of building the LSP-specific symbols directly while parsing source code, we should extract symbols from a more comprehensive internal model of Spring components that we build while parsing source code.

The internal model of Spring components can then also be provided via an additional API (LSP extension) to feed more information to the UI, for example to allow more sophisticated gutter icons, navigation, etc.

Related issues:

This could also allow the validation mechanisms to retrieve Spring component information.

@martinlippert
Copy link
Member Author

As a first step, we should collect the basic content for the internal model and what information it should provide. Initial thoughts are:

  • list of beans
    • bean name
    • bean type
    • location where the bean is defined
    • other beans/types that need to be injected and where (types, locations)
    • other beans where this bean could be injected into

This list of beans would not contain things like request mappings or other higher level concepts.

@martinlippert
Copy link
Member Author

martinlippert commented Mar 31, 2023

I am working on a spike for this in a branch (https://github.com/spring-projects/sts4/tree/spring-metamodel-spike). The initial steps focus on the internal implementation and source code parsing, which piggybacks on the symbol parsing infrastructure, but creates an independent model representation.

I haven't created an extension to the LSP interface to query this model from the outside yet, that is one of the many additional steps that I need to work on.

Nevertheless, I wanted to update you on the initial steps here:

  • you can get a list of beans from the model
  • each bean contains information about:
    • bean name
    • bean type
    • bean location
    • injection points

Each injection point has information about

  • name (the name of the field or constructor parameter)
  • type (the type of the field or constructor parameter)
  • location (the exact location of the field or the constructor parameter)

The injection points take @Autowired fields into account as well as constructor parameters, whether they are annotated with @Autowired or not.

In addition to that, the external API will get a method to find potential matching beans for an injection point. The client should NOT implement any logic how to match beans against injection points, etc. All that logic will go into the model to have one place where the spring specific knowledge about those things sits.

@Eskibear
Copy link
Contributor

Eskibear commented Apr 3, 2023

Some feedback from use cases in dashboard:

  • I expect the API e.g. getBeans to return correct results with no ambiguity, and to be an async call if it needs to wait some services to be loaded. For the moment, to visualize list of beans defined in workspace, dashboard sends workspace/symbol requests using the client instance from upstream API. Because it returns empty list at startup (maybe something like symbol service is not ready yet), dashboard is polling until the result is not empty. It's not good in my point of view and is sending too many requests. And we cannot distinguish whether it's because the service is not ready, or the list of beans is indeed empty (is it possible to have no beans? like when I haven't added any source code yet)
  • Injection points information would be very useful. Now jumping between beans and injection points is only available in live data. With this new model, I think it's possible to provide better navigation experience. E.g. the dashboard extension shows gutter icon in editor for each bean, with vscode's proposed api we can register item in context menu or a gutter, like "jumping to injection points of this bean" and "beans related to this injection point".

@martinlippert
Copy link
Member Author

  • I expect the API e.g. getBeans to return correct results with no ambiguity, and to be an async call if it needs to wait some services to be loaded. For the moment, to visualize list of beans defined in workspace, dashboard sends workspace/symbol requests using the client instance from upstream API. Because it returns empty list at startup (maybe something like symbol service is not ready yet), dashboard is polling until the result is not empty. It's not good in my point of view and is sending too many requests. And we cannot distinguish whether it's because the service is not ready, or the list of beans is indeed empty (is it possible to have no beans? like when I haven't added any source code yet)

I think this point is an interesting question. From the Spring Boot language server perspective, we don't really know exactly when things are done. The language server reacts to receiving information about projects and their corresponding classpath, so whenever we receive such an event, the indexer would update itself for that corresponding project. And whenever a project changes or a file inside of a project changes, the indexer again would update itself.

We could keep track (inside of the indexer) which projects are currently covered by the index and which not (yet). And in case a request comes in for a project that the indexer tracks, but has some indexing work in the queue, wait for this indexing work to be done before returning the result.

But how do we deal with index updates caused by other activities such as saving a file or updating the classpath? It feels to me like the indexer should have a notification mechanism that tells interested parties when stuff happens, like the index information for a certain file or project has changed, has been created, or has been deleted.

In that case, the dashboard could react to those update events and display the corresponding information whenever they arrive. @Eskibear What do you think?

@Eskibear
Copy link
Contributor

Eskibear commented Apr 3, 2023

It would be great that the indexer tracks which projects are covered. And for request related to a project not covered yet, it can immediately return null instead of an empty list. That would remove the ambiguity.

The notification mechanism sounds good to me (if it's not too complicated to implement), just like what you did when live processes connect/update/disconnect.

@martinlippert
Copy link
Member Author

The first steps towards this new index have arrived in the main branch and are available for you @Eskibear to try out:

Here is the extension API:
https://github.com/spring-projects/sts4/blob/main/vscode-extensions/vscode-spring-boot/lib/api.d.ts#L110

This is backed by the spring boot language server and corresponding LSP extensions:
https://github.com/spring-projects/sts4/tree/main/headless-services/commons/commons-lsp-extensions/src/main/java/org/springframework/ide/vscode/commons/protocol/spring

Feel free to take an early look and play around with this. There are only the first steps here, but a good starting point to get feedback here.

Important note:
Please keep in mind that the supertypes information for beans is meant to be used inside of the language server only, this is not meant to be part of the public API. We will work on removing that from the public API in the future.

The information is recorded by the language server in order to implement further additions to the protocol, so that you can ask the language server for possible beans that match a certain injection point, etc. I know that this is a feature you had in mind, but please do not implement this yet yourself on the client side. We would like to keep the logic for finding beans that match injection points in the language server to have most of the spring specific knowledge there and to keep it as closely aligned as possible with the logic that is used by the Spring framework itself at runtime when matching beans.

@Eskibear
Copy link
Contributor

Great to see the progress! Thank you @martinlippert !

Got it. Do not use supertypes field, and do not send custom requests to language server directly. It makes total sense. Ideally, downstream extensions should never ask language server directly, but to call the APIs.

/cc @testforstephen who will follow up and work on Spring related extensions.

@martinlippert
Copy link
Member Author

I am closing this item here as a first step towards providing the new index information. Further changes and improvements to this will be marked with the label theme: spring-index & symbols

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants