SLCORE-1032 Wrong token error during sync - [New design] #1187
Conversation
kirill-knize-sonarsource
force-pushed
the
feature/kk/wrong-token-during-sync-new-design
branch
5 times, most recently
from
0483733 to
84a4ae9
Compare
|
kirill-knize-sonarsource
force-pushed
the
feature/kk/wrong-token-during-sync-new-design
branch
7 times, most recently
from
75de64c to
5361898
Compare
kirill-knize-sonarsource
force-pushed
the
feature/kk/wrong-token-during-sync-new-design
branch
from
86acfa4 to
bffb584
Compare
|
There was a problem hiding this comment.
I left some ideas for improvement, but it looks mostly good to me. You could remove [New design] from the PR title and the commit message. And the last thing, I am not sure we should merge this now, we should maybe make sure the notification is implemented on the IDE side?
| @@ -47,19 +53,21 @@ | |||
|
|
|||
| @Named | |||
| @Singleton | |||
| public class ServerApiProvider { | |||
| public class ServerApiProvider implements ConnectionManager { | |||
There was a problem hiding this comment.
I don't see a strong reason to keep this interface and have a single implementation. Why about keeping only a ConnectionManager class?
| private final URI sonarCloudUri; | ||
|
|
||
| public ServerApiProvider(ConnectionConfigurationRepository connectionRepository, ConnectionAwareHttpClientProvider awareHttpClientProvider, HttpClientProvider httpClientProvider, | ||
| SonarCloudActiveEnvironment sonarCloudActiveEnvironment) { | ||
| SonarCloudActiveEnvironment sonarCloudActiveEnvironment, SonarLintRpcClient client) { |
There was a problem hiding this comment.
Indentation issue?
| } | ||
| cacheConnectionIdPerVersion.put(connectionId, version); | ||
| } | ||
| serverApiProvider.tryGetConnectionWithoutCredentials(connectionId) |
There was a problem hiding this comment.
I checked a bit more around this use case. In fact, the api/system/status call is not authenticated. So instead of having a method at the serverApiProvider level, I would find it more logical that the decision to not use authentication be made in SystemApi. But this has some impact in other places
| * It's not a big problem because we don't use such requests during scheduled sync. | ||
| * They are mostly related to setting up the connection or other user-triggered actions. | ||
| */ | ||
| ServerApi getTransientConnection(String token, @Nullable String organization, String baseUrl); |
There was a problem hiding this comment.
I wonder if this method should be located in ConnectionManager. As it's "transient" and by definition we won't keep state for it, and it's not (yet) a connection. I think it would simplify a bit the current ServerApiProvider which has a lot of entrypoints
| return lastNotificationTime.plus(WRONG_TOKEN_NOTIFICATION_INTERVAL).isBefore(Instant.now()); | ||
| } | ||
|
|
||
| private void notifyClientAboutWrongTokenIfNeeded() { |
There was a problem hiding this comment.
I wonder if it's the responsibility of the ServerConnection to notify to the client. I would find it more logical to have it in the ConnectionManager.
| return Optional.empty(); | ||
| } | ||
| return serverApi.get().hotspot().fetch(hotspotKey, cancelMonitor); | ||
| return serverApiProvider.withValidConnectionFlatMapOptionalAndReturn(connectionId,api -> api.hotspot().fetch(hotspotKey, cancelMonitor)); |
There was a problem hiding this comment.
The name of this method is a bit long. In fact this makes me wonder if Api classes should return Optionals at all, maybe they should instead let the errors bubble up?
| @@ -137,4 +145,44 @@ private HttpClient getClientFor(EndpointParams params, Either<TokenDto, Username | |||
| userPass -> httpClientProvider.getHttpClientWithPreemptiveAuth(userPass.getUsername(), userPass.getPassword())); | |||
| } | |||
|
|
|||
| @Override | |||
| public ServerConnection getConnectionOrThrow(String connectionId) { | |||
There was a problem hiding this comment.
As shared earlier, I find the surface of methods quite large, so for people not familiar it might be complex to know which one to use. Do you think there is a way to simplify? Maybe we should at least document the different usages
| @VisibleForTesting | ||
| public Optional<ServerConnection> getValidConnection(String connectionId) { | ||
| return tryGetConnection(connectionId).filter(ServerConnection::isValid); | ||
| } |
There was a problem hiding this comment.
This is used only in a single test, couldn't we get rid of it and use another method? I mean at least to not have it public and remove the annotation
|
|
||
| @VisibleForTesting | ||
| public Optional<ServerConnection> getValidConnection(String connectionId) { | ||
| return tryGetConnection(connectionId).filter(ServerConnection::isValid); |
There was a problem hiding this comment.
A last thing, should we log that the connection is invalid? Maybe too noisy?
SLCORE-1032