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

Review LSP4Jakarta diagnostic code and resolve remaining issues with matching qualified class names. #879

Closed
23 tasks done
mrglavas opened this issue Jul 22, 2024 · 5 comments
Assignees
Labels
bug Something isn't working Epic language client language server integration medium priority
Milestone

Comments

@mrglavas
Copy link
Contributor

mrglavas commented Jul 22, 2024

While reviewing #860 I noticed additional issues in the way that the LSP4Jakarta diagnostic collectors are matching qualified names. Specifically BeanValidationDiagnosticsCollector is checking if a type name ends with the simple name of several classes (constants defined in BeanValidationConstants).

image

image

For instance this code type.getCanonicalText().endsWith(BIG_DECIMAL) was likely intended to only match java.math.BigDecimal but will match a BigDecimal in any Java package (e.g. com.example.BigDecimal). There may yet still be other instances of qualified names that are not processed correctly. We should thoroughly review the code to ensure that we fully address these issues once and for all.

Code review check list for each of the diagnostic collectors within io.openliberty.tools.intellij.lsp4jakarta.lsp4ij:

  • AbstractDiagnosticsCollector - usage of nameEndsWith() in conjunction with isImportedJavaElement() should be investigated.
  • annotations.AnnotationDiagnosticsCollector - isValidAnnotation() uses endsWith() instead of equals()
  • beanvalidation.BeanValidationDiagnosticsCollector - see issues described in the section above
  • cdi.ManagedBeanDiagnosticsCollector - no issues but should remove unused unqualified names from ManagedBeanConstants
  • di.DependencyInjectionDiagnosticsCollector - no issues but should remove unused unqualified names from DependencyInjectionConstants
  • jax_rs.Jax_RSClassDiagnosticsCollector - no issues
  • jax_rs.ResourceMethodDiagnosticsCollector - no issues
  • jsonb.JsonbDiagnosticsCollector - no issues
  • jsonp.JsonpDiagnosticCollector - no issues
  • persistence.PersistenceEntityDiagnosticsCollector - no issues
  • persistence.PersistenceMapKeyDiagnosticsCollector - no issues
  • servlet.FilterDiagnosticsCollector - no issues but should remove unused unqualified names from ServletConstants
  • servlet.ListenerDiagnosticsCollector - no issues
  • servlet.ServletDiagnosticsCollector - no issues
  • websocket.WebSocketDiagnosticsCollector - isWrapper() uses unqualified names to check for classes in the java.lang package. Should remove unused unqualified names from WebSocketConstants.

Investigate and resolve issues in diagnostic collectors that were identified by the code review:

Expand test coverage for processing of qualified names:

@mrglavas
Copy link
Contributor Author

mrglavas commented Oct 7, 2024

I have started reviewing the LSP4Jakarta diagnostic collectors. Will document additional discoveries here.

@mrglavas
Copy link
Contributor Author

mrglavas commented Oct 9, 2024

AnnotationDiagnosticsCollector is checking whether two annotation names match using endsWith() instead of equals(). As an example, this means that if the expected name is jakarta.annotation.PostConstruct it would match on.PostConstruct.

    private static boolean isValidAnnotation(String annotationName, String[] validAnnotations) {
        if (annotationName != null) {
            for (String fqName : validAnnotations) {
                if (fqName.endsWith(annotationName)) {
                    return true;
                }
            }
        }
        return false;
    }

Should also delete the unused non-qualified class names from AnnotationConstants to help prevent accidental future usage.

@mrglavas
Copy link
Contributor Author

mrglavas commented Oct 9, 2024

Analysis of BeanValidationDiagnosticsCollector is in the description. Should replace the unqualified names with qualified names and use equals() for comparing them instead of endsWith().

@mrglavas
Copy link
Contributor Author

WebSocketDiagnosticsCollector is checking whether a value class is a primitive type wrapper using unqualified names for classes in the java.lang package.

    private boolean isWrapper(String valueClass) {
        return WebSocketConstants.WRAPPER_OBJS.contains(valueClass)
                || WebSocketConstants.RAW_WRAPPER_OBJS.contains(valueClass);
    }

This method should only use the qualified names to determine if a class is a wrapper.

@mrglavas mrglavas changed the title Review LSP4Jakarta diagnostic code and resolve remaining issues with matching qualified class names. Epic: Review LSP4Jakarta diagnostic code and resolve remaining issues with matching qualified class names. Oct 17, 2024
@mrglavas mrglavas added the Epic label Oct 17, 2024
@mrglavas mrglavas changed the title Epic: Review LSP4Jakarta diagnostic code and resolve remaining issues with matching qualified class names. Review LSP4Jakarta diagnostic code and resolve remaining issues with matching qualified class names. Oct 17, 2024
@mrglavas
Copy link
Contributor Author

All tasks have been completed on this epic.

@github-project-automation github-project-automation bot moved this from In Progress to Sprint Closed in Open Liberty Developer Experience Nov 20, 2024
@TrevCraw TrevCraw moved this from Sprint Closed to Closed in Open Liberty Developer Experience Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Epic language client language server integration medium priority
Projects
Development

No branches or pull requests

2 participants