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

fix: upgraded tokens not being used #2166

Merged
merged 3 commits into from
Oct 24, 2023

Conversation

vitorhugods
Copy link
Member


PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

Tokens that are result of token-upgrading (i.e. associating a client ID with the tokens), are not being used right after they are acquired.

Causes

In the previous implementation, the SessionManagerImpl was not properly utilizing updated tokens causing session issues.

It was keeping a session in memory and using it. So when other entities would modify the session directly in storage, it would not be updated.

Solutions

There was no reason to keep the session in memory, as it's something that is rarely used. And as long as the SessionManager is atomic in its operations, it won't race with itself.

The code has been updated to not retain the first session instance but fetch a new one each time, hence always using the newest token. Tests have been added to verify the proper functioning of this feature.

Testing

Test Coverage

  • I have added automated test to this contribution

PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

In the previous implementation, the `SessionManagerImpl` was not properly utilizing updated tokens causing session issues. The code has been updated to not retain the first session instance but construct a new one each time, hence always using the newest token. Tests have been added to verify the proper functioning of this feature.
@github-actions
Copy link
Contributor

github-actions bot commented Oct 24, 2023

Unit Test Results

   459 files  ±0     459 suites  ±0   2m 15s ⏱️ - 1m 11s
2 556 tests ±0  2 452 ✔️ ±0  104 💤 ±0  0 ±0 

Results for commit 55c0278. ± Comparison against base commit 872981a.

♻️ This comment has been updated with latest results.

@datadog-wireapp
Copy link

Datadog Report

Branch report: fix/upgraded-token-not-being-used
Commit report: 3113c7e

kalium-ios: 2 Failed (0 Known Flaky), 0 New Flaky, 1772 Passed, 30 Skipped, 13.09s Wall Time

❌ Failed Tests (2)

  • givenInitialSessionIsUpdated_whenFetchingSession_thenSessionShouldBeUpdatedProperly[iosX64] - com.wire.kalium.logic.network.SessionManagerTest - Details

    Expand for error
     kotlin.AssertionError: Expected <41d2b365-f4a9-4ba1-bddf-5afb8aca6786>, actual <potato>.
     
     kotlin.AssertionError: Expected <41d2b365-f4a9-4ba1-bddf-5afb8aca6786>, actual <potato>.
     	at kotlin.Error#<init>(/opt/buildAgent/work/f43969c6214a19e7/kotlin/kotlin-native/runtime/src/main/kotlin/kotlin/Exceptions.kt:14)
     	at kotlin.AssertionError#<init>(/opt/buildAgent/work/f43969c6214a19e7/kotlin/kotlin-native/runtime/src/main/kotlin/kotlin/Exceptions.kt:132)
     	at kotlin.test.DefaultAsserter#fail(/opt/buildAgent/work/f43969c6214a19e7/kotlin/libraries/kotlin.test/common/src/main/kotlin/kotlin/test/DefaultAsserter.kt:16)
     	at kotlin.test.Asserter#assertTrue(/opt/buildAgent/work/f43969c6214a19e7/kotlin/libraries/kotlin.test/common/src/main/kotlin/kotlin/test/Assertions.kt:652)
     	at kotlin.test.Asserter#assertEquals(/opt/buildAgent/work/f43969c6214a19e7/kotlin/libraries/kotlin.test/common/src/main/kotlin/kotlin/test/Assertions.kt:671)
     	at kotlin.test#assertEquals(/opt/buildAgent/work/f43969c6214a19e7/kotlin/libraries/kotlin.test/common/src/main/kotlin/kotlin/test/Assertions.kt:63)
     	at kotlin.test#assertEquals$default(/opt/buildAgent/work/f43969c6214a19e7/kotlin/libraries/kotlin.test/common/src/main/kotlin/kotlin/test/Assertions.kt:62)
     ...
    
  • givenTokenWasUpdated_whenGettingSession_thenItShouldBeUpdatedAsWell[iosX64] - com.wire.kalium.logic.network.SessionManagerTest - Details

    Expand for error
     io.mockative.MissingExpectationError: A function was called without a matching expectation.
     
         An expectation was not given on the function:
             AuthTokenStorageMock.getToken(41d2b365-f4a9-4ba1-bddf-5afb8aca6786@domain)
     
         Set up an expectation using:
             given(instance).invocation { getToken(41d2b365-f4a9-4ba1-bddf-5afb8aca6786@domain) }
                 .then { ... }
         
         The following expectations were configured on the mock:
     ...
    

@vitorhugods vitorhugods enabled auto-merge October 24, 2023 11:02
@vitorhugods vitorhugods added this pull request to the merge queue Oct 24, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #2166 (55c0278) into develop (872981a) will decrease coverage by 0.01%.
The diff coverage is 71.42%.

@@              Coverage Diff              @@
##             develop    #2166      +/-   ##
=============================================
- Coverage      57.88%   57.88%   -0.01%     
  Complexity        21       21              
=============================================
  Files           1059     1059              
  Lines          40190    40185       -5     
  Branches        3722     3721       -1     
=============================================
- Hits           23265    23261       -4     
+ Misses         15314    15313       -1     
  Partials        1611     1611              
Files Coverage Δ
...om/wire/kalium/logic/network/SessionManagerImpl.kt 50.79% <71.42%> (-2.15%) ⬇️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 872981a...55c0278. Read the comment docs.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 24, 2023
@vitorhugods vitorhugods added this pull request to the merge queue Oct 24, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 24, 2023
@vitorhugods vitorhugods merged commit b6cfb79 into develop Oct 24, 2023
15 checks passed
@vitorhugods vitorhugods deleted the fix/upgraded-token-not-being-used branch October 24, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants