SLCORE-1032 Wrong token error during sync #1157
Conversation
|
backend/core/src/main/java/org/sonarsource/sonarlint/core/ServerApiProvider.java
Outdated
Show resolved
Hide resolved
| RuleType.VULNERABILITY))) | ||
| .start(); | ||
|
|
||
| server.getMockServer().stubFor(get("/api/system/status").willReturn(aResponse().withStatus(status))); |
There was a problem hiding this comment.
I think it would make sense to improve the server fixture instead of accessing the internals. It might be useful for other cases in the future, and it hides how we implement the fixture
| @@ -69,7 +77,7 @@ public OrganizationApi organization() { | |||
| } | |||
|
|
|||
| public IssueApi issue() { | |||
| return new IssueApi(helper); | |||
| return new IssueApi(helper, wrongTokenCallbackWithConnectionIdParam); | |||
There was a problem hiding this comment.
Why is the IssueApi the only one that accepts this parameter, and not other Api classes?
| try (var failedResponse = toBeClosed) { | ||
| if (failedResponse.code() == HttpURLConnection.HTTP_UNAUTHORIZED) { | ||
| invalidTokenCallback.run(); |
There was a problem hiding this comment.
I feel propagating this callback in many classes is a bit odd. As discussed maybe we could rely on exceptions and have a "middle -man" catching and acting on them
backend/core/src/main/java/org/sonarsource/sonarlint/core/usertoken/UserTokenService.java
Outdated
Show resolved
Hide resolved
kirill-knize-sonarsource
force-pushed
the
feature/kk/SLCORE-1032-Wrong-token-error-during-sync
branch
from
4ede711 to
bef1626
Compare
backend/core/src/main/java/org/sonarsource/sonarlint/core/ConnectionManager.java
Outdated
Show resolved
Hide resolved
backend/core/src/main/java/org/sonarsource/sonarlint/core/ConnectionManager.java
Show resolved
Hide resolved
kirill-knize-sonarsource
force-pushed
the
feature/kk/SLCORE-1032-Wrong-token-error-during-sync
branch
2 times, most recently
from
6032253 to
66d6373
Compare
kirill-knize-sonarsource
force-pushed
the
feature/kk/SLCORE-1032-Wrong-token-error-during-sync
branch
from
2d21c06 to
bf53de4
Compare
|
There was a problem hiding this comment.
If we are to merge this, I'd prefer to have it merged after the release. It probably need some dogfooding time.
I'm also unsure about the design, is it alright to apply the same behavior on all methods (i.e. sending a notification)? We already have an handler in ServerApiHelper that is checking for error codes. Sometimes we receive the exception on client side, and sometimes we don't due to catch. Maybe we should adapt method per method the behavior?
|
|
||
| public Optional<IssueApi.ServerIssueDetails> tryFetchIssue(String issueKey, String projectKey, String branch, @Nullable String pullRequest, | ||
| SonarLintCancelMonitor cancelMonitor) { | ||
| return callWithErrorHandleAndRethrow(() -> serverApi.issue().fetchServerIssue(issueKey, projectKey, branch, pullRequest, cancelMonitor)); |
There was a problem hiding this comment.
I'm not sure this is working, for example fetchServerIssue is already catching any exception from the HTTP request, it's not going to be handled by your handler
There was a problem hiding this comment.
It works in the sense that it is a transparent wrapper that will notify the client in case a wrapped call throws a corresponding exception. If the wrapped call doesn't throw - the wrapper won't do anything.
| @@ -5,6 +5,8 @@ | |||
| * Signature of `org.sonarsource.sonarlint.core.rpc.protocol.backend.file.DidUpdateFileSystemParams#DidUpdateFileSystemParams` was changed | |||
| * Parameter `addedOrChangedFiles` was split into `addedFiles` and `changedFiles` | |||
| * Removed parameter `branch` and `pullRequest` from `org.sonarsource.sonarlint.core.rpc.protocol.client.issue.IssueDetailsDto` as it should not be used anymore by the client. | |||
| * Add new client method `org.sonarsource.sonarlint.core.rpc.client.SonarLintRpcClientDelegate#invalidToken` | |||
| * It is called when the token is invalid and the client should notify the user about it | |||
There was a problem hiding this comment.
I'm not sure I like this way of doing. SLCORE is not aware in which context the method was called. For instance, on IntelliJ, we might be calling the server from a wizard on UI thread, we are not going to display a notification during the process. Any error should be done within the scope of the wizard (so as response from the call to the server IMO).
There was a problem hiding this comment.
We haven't discussed in depth the scenarios you described. This feature is more about mitigating the sync errors that lead to an inconsistent state of the app in a way that is not obvious to the user.
Situations like wrong token or network error during the wizard manipulations are not in the scope, and I think notifications to clients should be disregarded or even not sent in the first place. Unfortunately, on the CORE side, we don't track if the Web API call is performed by the client request or because of the CORE internal need. So, not sending notifications is quite tricky.
Let's discus it.
| import org.sonarsource.sonarlint.core.serverapi.system.ServerStatusInfo; | ||
| import org.sonarsource.sonarlint.core.serverapi.system.ValidationResult; | ||
|
|
||
| public class ServerApiErrorHandlingWrapper { |
There was a problem hiding this comment.
Looks complex to maintain and likely to be error prone 🤷🏻
There was a problem hiding this comment.
I came up with this because it's an approach where we centralize all the API calls and handle certain situations like wrong tokens or network errors in a single place. And introducing new API calls will naturally follow the same pattern. Even if not - if someone forgets to put the new call into the wrapper - in the worst-case scenario, we will get an unhandled error for this call as it exists today.
The suggestion above to change API behaviors method per method is pretty much as error-prone as this - you can easily forget to do it. But method per method changes will be a less visible pattern than a centralized wrapper, and I think it's actually easier to forget to follow this idea in the future.
Let's discuss our options. I created a meeting.
SLCORE-1032