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

@Resource annotation are not processed by the language server #176

Closed
gayanper opened this issue Jan 7, 2019 · 17 comments
Closed

@Resource annotation are not processed by the language server #176

gayanper opened this issue Jan 7, 2019 · 17 comments
Labels
for: eclipse something that is specific for Eclipse for: vscode something that is specific for VSCode theme: spring index & symbols type: enhancement

Comments

@gayanper
Copy link
Contributor

gayanper commented Jan 7, 2019

fields and methods which are annotation @Resource are not processed and listed in the symbols list. But STS3 was able to list the @Resource.

@martinlippert
Copy link
Member

The current implementation takes Spring annotations into account, but nothing else (at the moment), therefore @Resource is not being processed for the symbols.

But it certainly makes sense to change that, so that other annotations are being processed, too. But I think @Resource is not the only one that we should take into account. I guess everything from the javax.inject package, too. Anything else?

@gayanper
Copy link
Contributor Author

gayanper commented Jan 7, 2019

@martinlippert agree, we should consider all javax.inject package, may we can start from there and add anything more on the way. Would you like a PR for this ?

@gayanper
Copy link
Contributor Author

gayanper commented Jan 7, 2019

@martinlippert i belive the @Resource annotation must belong to the BootJavaHoverProvider right ? There is no purpose over indexing the @Resource through SpringIndexer is it ? Let me know what you think.

@gayanper
Copy link
Contributor Author

gayanper commented Jan 7, 2019

image
How does this looks ?

@martinlippert
Copy link
Member

PRs are always welcome, of course... :-) Give us a few days to review and comment on them, but if it takes a while doesn't mean we don't like it or won't look at it. We are more than happy to review, comment, and merge PRs !!!

With regards to the screenshot, it would be good to see the source code to which the symbols belong, too. I wonder why there is a @Bean showing up in the highlighted line. We always try to show the exact annotation on the symbol that is in the source, so if the source contains @Resource, the symbol should also contain @Resource (just as an example).

@martinlippert
Copy link
Member

If you want to provide a special symbol, you need to implement a SymbolProvider and register that:

https://github.com/spring-projects/sts4/blob/master/headless-services/spring-boot-language-server/src/main/java/org/springframework/ide/vscode/boot/java/BootJavaLanguageServerComponents.java#L376

The hover providers are responsible for showing data from live running applications (which is a whole different game for non-boot spring apps). But yes, if you want to support the @Resource (or whatever) annotation there as well, you would need to implement a HoverProvider for that annotation type as well.

@gayanper
Copy link
Contributor Author

gayanper commented Jan 8, 2019

With regards to the screenshot, it would be good to see the source code to which the symbols belong, too. I wonder why there is a @Bean showing up in the highlighted line. We always try to show the exact

Basically the code was some thing like this

@Bean
@Singleton
@Named(value="jason-parser")
@Parser // this is a Qualifier i made for testing.
public JsonParser getJsonParser() {
 ...
}

I know this dosn't make much sense but i use the RestrictedDefaultSymbolProvider since it was used for conditions and these annotation where something like conditions.

@gayanper
Copy link
Contributor Author

gayanper commented Jan 8, 2019

If you want to provide a special symbol, you need to implement a SymbolProvider and register that:

@martinlippert for @Resource @Inject @Autowire annotation do you think it will be good a index them as a new symbol since these are injection points or some what like assignments. So would you think its a good idea to represent then with a "=@" ?

@gayanper
Copy link
Contributor Author

Any recommendations on the suggestions ?

@gayanper
Copy link
Contributor Author

gayanper commented Jan 15, 2019

How about these for above mentioned Injection annotation types and other annotations like Named, Singleton etc.

image

@gayanper
Copy link
Contributor Author

@martinlippert any feedback on this ? If you all like the way it is i can update tests and create PR.

@martinlippert
Copy link
Member

great to see you working on this, I just need a bit of time to process this... Sorry for the delay. Will be back as soon as possible on this.

@martinlippert
Copy link
Member

OK, I looked at this in more detail now, here is what I think:

The code example that you provided:

@Bean
@Singleton
@Named(value="jason-parser")
@Parser // this is a Qualifier i made for testing.
public JsonParser getJsonParser() {
 ...
}

produces the symbol in the way the screenshot shows this - and that looks perfectly fine to me.

Now moving on to the injection points like @Autowired, @Inject and @Resource:
I think they should indeed be showing up in the same way in the list of symbols, so they should produce similar symbol informations.

I also like the idea of having a specialized shortcut like @=, but that need to be chosen carefully, since some of those characters have a special meaning in Visual Studio Code and cause the symbol navigation to do special things. And I think the = belongs to that group. Maybe we can try something like @*, which would somehow relate to the @+ nicely.

An important feature for this would indeed be to show all the other annotations on that field or method (like @Qualifier) and so on, much in the same way than we do for the @Bean symbol.

I think we should start to collect a few samples here (code snippet + corresponding symbol) as a first step. We can use that data also in the unit tests.

As a side note:
At the moment, the symbol indexer creates a default symbol for every spring-related annotation. We need to avoid that in case this annotation showed up in a context of a @Bean or @Autowired or @Inject or @Resource annotation as soon as those symbols integrate the other annotations of the field or method. Otherwise we would end up having two symbols that contain the @Qualifier, for example - the default symbol and the symbol for the @Autowired annotation where the @Qualifier annotation belongs to. This is a bit tricky to realize due to the way we made the symbol indexer configurable for different annotations. The SymbolProvider implementations usually don't know anything about each other and don't share any state. That means if we create the default symbol, we would need to check whether the annotation is in the context of another one and then not create the default symbol.

Attention:
I committed a few changes to the Spring symbol indexer recently, so a few things got renamed and have changed, don't be surprised. The underlying mechanics haven't changed, so you can still apply your existing knowledge... :-)

@gayanper
Copy link
Contributor Author

gayanper commented Jan 18, 2019

Thanks for the feedback @martinlippert.

I think we should start to collect a few samples here (code snippet + corresponding symbol) as a first step. We can use that data also in the unit tests.

will try to come up with some code samples. May be the community members here can help me out as well.

As a side note:
At the moment, the symbol indexer creates a default symbol for every spring-related annotation. We need

Yes this might be a problem. What if we don't have explicit SymbolProviders for such supporting annotations like Qualifier, because they will never be annotated alone, But may be process them in the parent annotations like Autowire and Bean for example. That way we can void duplicate Symbols and just creating a Qualifier symbol without its context might not valuable as well.

@gayanper
Copy link
Contributor Author

As a side note:
At the moment, the symbol indexer creates a default symbol for every spring-related annotation. We need

@martinlippert i got your point on this, and implementation is wrong. Actually the screenshot i posted at first on Bean annotation is provided by the BeanSymboleProvider, not from my crazy changes :). But i now have some understanding what i need to do, Will get back to you after the modifications. Thanks for the support.

I also like the idea of having a specialized shortcut like @=, but that need to be chosen carefully, since some

I didn't checked on VSCode, but what about having "@>" for Injection annotations.

@martinlippert
Copy link
Member

At the moment @> is used for function definitions. Lets choose something random and change that later if needed.

@martinlippert
Copy link
Member

This one here can finally be closed. Symbols got implemented as part of #1312, completion support was done in #1298.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: eclipse something that is specific for Eclipse for: vscode something that is specific for VSCode theme: spring index & symbols type: enhancement
Projects
None yet
Development

No branches or pull requests

2 participants