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

added quarkus.http.root-path property to CodeLensURL #174

Merged
merged 1 commit into from
Sep 10, 2021

Conversation

AlexXuChen
Copy link
Contributor

@AlexXuChen AlexXuChen commented Sep 1, 2021

Added quarkus.http.root-path property from application.properties to CodeLens URL.

Closes redhat-developer/quarkus-ls/issues/368

Signed-off-by: Alexander Chen alchen@redhat.com

*THE CHANGE:
image

@angelozerr
Copy link
Contributor

At first LSP4MP should be not linked to quarkus, it means that it should not use quarkus.http.root-path in the code. LSP4MP provides a JaxRSContext where you can set some value.

For instance Quarkus provides quarkus.http.port property and if you see the LSP4MP code it doesn't use it. The quarkus.http.port is retrieved from quarkus-ls side and update the JaxRscontext#setServerPort with the proper value of quarkus.http.port:

https://github.com/redhat-developer/quarkus-ls/blob/d931b173582344dad9357a0ecd1180af0984b718/quarkus.jdt.ext/com.redhat.microprofile.jdt.quarkus/src/main/java/com/redhat/microprofile/jdt/internal/quarkus/jaxrs/java/QuarkusJaxRsCodeLensParticipant.java#L48

After that LSP4MP get the serverPort to build the proper local base URL at

localBaseURL = new StringBuilder("http://localhost:").append(getLocalServerPort()).toString();

You should try to mimic this strategy. At first I will remove getLocalBaseUrl and getServerPort from MicroProfileJavaCodeLensParams

because it's parameters (we should not update it) and move those methods in JaxRsContext, the idea is to work with JaxRsContext and not with MicroProfileJavaCodeLensParams to build the URL:

private static void collectURLCodeLenses(IJavaElement[] elements, String rootPath, Collection<CodeLens> lenses,
			JaxRsContext context, IJDTUtils utils, IProgressMonitor monitor)

I will add in JaxRsContext a new field with getter/setter rootPath. QuarkusJaxRsCodeLensParticipant will update this rootPath with quarkus.http.root-path.

The JaxRsContext will use this info to build the URL:

public String getLocalBaseURL() {
   StringBuilder localBaseURL = new StringBuilder("http://localhost:");
  localBaseURL .append(getLocalServerPort()).toString();
   if (rootPath != null) {
      localBaseURL .append(getRootPath());
   }
  return localBaseURL.toString();
}

@AlexXuChen AlexXuChen force-pushed the issue368-quarkusls branch 3 times, most recently from baf966e to 30574e3 Compare September 3, 2021 15:06
@angelozerr
Copy link
Contributor

@AlexXuChen this PR doesn't fix all issues from redhat-developer/quarkus-ls/issues/368.

I re-read it and it seems @ApplicationPath Java annotation play the same role than the quarkus property quarkus.http.root-path. As @ApplicationPath is a part of MicroProfile spec; it should be managed on LSP4MP side.

According to @clochardpagan comments:

  • there is no option to work with host differing from localhost. That's a problem.
  • there is no option to work with https:// at all. Bad, but can be fixed via SSL termination or/and proxy.

To fix this problem an idea is to provide a setting to declare a base URL for that. Or perhaps we could declare an array of baseURL to generate several Codelens URL for serveral server. @fbricon @rgrunber what do you think about that?

@AlexXuChen those issues could be done in an another PRs, but please change your commit which closes the issue 368, because this PR doesn't covers all issues.

Copy link
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could localBaseURL that's used in MicroProfileJavaCodeLensParams be removed ? I don't think it's used anywhere now that JaxRsContext took over the same functionality.

There's also extraneous whitespace throughout the files. Just need to remove them.

@AlexXuChen
Copy link
Contributor Author

AlexXuChen commented Sep 10, 2021

Could localBaseURL that's used in MicroProfileJavaCodeLensParams be removed ? I don't think it's used anywhere now that JaxRsContext took over the same functionality.

Originally MicroProfileJavaCodeLensParams was used before the JaxRsContext was required, and that was the only use of localBaseURL from the params. So yes, I believe it's okay to remove since it is unused.

However, I do believe the original intention was to remove MicroProfileJavaCodeLensParams entirely from the CodeLens participant, however I didn't since it is required to determine if the server is available (stores and sets checkServerAvailable)

@rgrunber
Copy link
Contributor

Yeah, there seems to be a bit of overlap between the MicroProfile params and the JaxRsContext. Maybe there's some parameters that need to reside in params for a little while before they get transfered over to the JaxRsContent but the localBaseURL stood out as the single getter in MicroProfile params wans't being called anywhere after you switched over and looked pretty much the same as the new one in JaxRsContext.

Signed-off-by: Alexander Chen <alchen@redhat.com>
Copy link
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried this out (with some modifications on quarkus-ls) and it seems to be working as expected.

@rgrunber rgrunber merged commit 9e086cd into eclipse:master Sep 10, 2021
@AlexXuChen AlexXuChen deleted the issue368-quarkusls branch September 10, 2021 20:20
@rgrunber rgrunber added this to the 0.4.0 milestone Sep 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CodeLens URL does not respect quarkus.http.root-path property
3 participants