PLUGINAPI-42 Migrate javax -> jakarta (backward compatibility) #92
Conversation
…hich is using javax.servlet.*
jacek-poreda-sonarsource
force-pushed
the
task/PLUGINAPI-42-replace-javax
branch
from
d6ced8d to
8c10020
Compare
ericg138
force-pushed
the
task/PLUGINAPI-42-replace-javax
branch
from
1212873 to
85d0de2
Compare
ericg138
force-pushed
the
task/PLUGINAPI-42-replace-javax
branch
from
85d0de2 to
3e275a1
Compare
|
SonarQube Quality Gate
|
jacek-poreda-sonarsource
requested review from
damien-urruty-sonarsource,
dbmeneses,
marcin-majewski-sonarsource and
johann-beleites-sonarsource
as code owners
|
Related changes in https://github.com/SonarSource/sonar-enterprise/pull/8091 |
| /** | ||
| * Implementation of {@link HttpRequest} based on a delegate of {@link HttpServletRequest} from the Jakarta Servlet API. | ||
| */ | ||
| public class JakartaHttpRequest implements HttpRequest { |
There was a problem hiding this comment.
why is this class in the API?
Shouldn't it be up to the products implementing the API to implement the HttpRequest with the framework they want to use?
There was a problem hiding this comment.
Indeed, it makes sense to drop it here completely and let products implement it
| /** | ||
| * Implementation of {@link HttpResponse} based on a delegate of {@link HttpServletResponse} from the Jakarta Servlet API. | ||
| */ | ||
| public class JakartaHttpResponse implements HttpResponse { |
There was a problem hiding this comment.
Same comment as JakartaHttpRequest
| /** | ||
| * Returns the raw request object from the Servlet API that matches this request. | ||
| */ | ||
| Object getRawRequest(); |
There was a problem hiding this comment.
This seems to defeat the purpose of having our own objects that represent HTTP requests and responses.
If the goal is for this to be used only by the products implementing the API, we should cast classes instead, since they are in control of the implementations at runtime.
For example, if a given version of SonarQube is creating filter contexts with HttpRequest of type JakartaHttpRequest, it's save for its filters to cast HttpRequest to JakartaHttpRequest.
There was a problem hiding this comment.
Hmm, so we wanted to return "raw" requests objects as we wanted to avoid creating more classes that mimic javax.servlet.* or jakarta.servlet.* classes, like for example Cookies. Though maybe indeed you are right as it defeats the purpose and with a little more effort and we could get rid of all classes instead of taking shortcuts.
Also, SAML library which we use requires us to pass javax.servlet.HttpRequest, see: org.sonar.auth.saml.SamlAuthenticator#initSamlAuth(java.lang.String, javax.servlet.http.HttpServletRequest, javax.servlet.http.HttpServletResponse). We saw that they are working on changing it, though it might take time for them: SAML-Toolkits/java-saml#395, SAML-Toolkits/java-saml#115.
| /** | ||
| * @deprecated since 9.16 use {@link #httpRequest} instead | ||
| */ | ||
| @Deprecated(since = "9.16", forRemoval = true) | ||
| private HttpServletRequest request; |
There was a problem hiding this comment.
this is a private field, not really an API that needs to be deprecated. It's not like we can replace its use.
| /** | ||
| * Returns the raw response object from the Servlet API that matches this response. | ||
| */ | ||
| Object getRawResponse(); |
| public Context(@Nullable String username, HttpRequest httpRequest) { | ||
| this.username = username; | ||
| this.httpRequest = httpRequest; | ||
| this.request = (HttpServletRequest) httpRequest.getRawRequest(); |
There was a problem hiding this comment.
This would imply that the Object returned by getRawRequest is of type HttpServletRequest, which defeats the purpose of a framework agnostic API.
I'd suggest we add the constructor Context(String, HttpRequest, HttpServletContext) instead. It's up to the products using this context to still provide the HttpServletContext during the deprecation period (which in practice will force them to still use the old servlet framework).
Note that the constructors of this class should only be used by the products implementing the API in production code, so compliance with the deprecation policy is not critical (plugins might use it in tests though, but that's only broken once they upgrade the API). We could add a javadoc comment warning that it's not intended for plugins to instantiate this class, except in tests.
So that means that at the end of the deprecation policy, we could immediately replace this constructor with Context(username, HttpRequest) and completely get rid of the dependency to servlet in the API. Following the same idea, we could perhaps drop the constructor Context(String, HttpServletRequest) now.
If we wanted to be super strict about the deprecation policy, the only solutions would be to go through 2 deprecation periods, or to introduce a completely new interface and deprecate this one.
There was a problem hiding this comment.
Yes, that make sense
| this.httpRequest = httpRequest; | ||
| this.username = username; | ||
| this.password = password; | ||
| this.request = (HttpServletRequest) httpRequest.getRawRequest(); |
There was a problem hiding this comment.
it'd probably be better for products to pass both HttpRequest and HttpServletRequest.
| /** | ||
| * Implementation of {@link HttpRequest} based on a delegate of {@link HttpServletRequest} from the Javax Servlet API. | ||
| */ | ||
| public class JavaxHttpRequest implements HttpRequest { |
There was a problem hiding this comment.
seems like this class is only used for backward compatibility during the deprecation policy.
The goal should be for the API not to depend on any servlet frameworks after the deprecation policy.
What if we move this class to org.sonar.api.security (where's it's needed) and make it package-private? That way plugins can't use it and we could drop it after the deprecation period.
We cannot do much about duplication as we don't want to break existing plugins.