Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Update UTC time to LocalTime (EXPOSUREAPP-1863) #1041

Merged
merged 4 commits into from
Aug 31, 2020

Conversation

SamuraiKek
Copy link
Contributor

@SamuraiKek SamuraiKek commented Aug 19, 2020

Checklist

  • Test your changes as thoroughly as possible before you commit them. Preferably, automate your test by unit/integration tests.
  • If this PR comes from a fork, please Allow edits from maintainers
  • Set a speaking title. Format: {task_name} (closes #{issue_number}). For example: Use logger (closes # 41)
  • Link your Pull Request to an issue - Pull Requests that are not linked to an issue with you as an assignee will be closed, except for minor fixes for typos or grammar mistakes in documentation or code comments.
  • Create Work In Progress [WIP] pull requests only if you need clarification or an explicit review before you can continue your work item.
  • Make sure that your PR is not introducing unnecessary reformatting (e.g., introduced by on-save hooks in your IDE)
  • Make sure that your PR does not contain changes in text files, therefore the PR must not contain changes in values/strings/* and / or assets/* (see issue [HELP WANTED][README] Text / Translations  #332 for further information)
  • Make sure that your PR does not contain compiled sources (already set by the default .gitignore) and / or binary files

Description

Changed the way our API quota is calculated based on the updated requirements from Google from using UTC to using the phone's local time

@SamuraiKek SamuraiKek added maintainers Tag pull requests created by maintainers rc3-1.3.0 labels Aug 19, 2020
@SamuraiKek SamuraiKek requested review from jakobmoellerdev and a team August 19, 2020 11:46
@kbobrowski
Copy link
Contributor

This will introduce exceeded quota error on devices which still use GMS version which resets quota at midnight UTC. Implementation should support both mechanisms until all devices have GMS version which resets quota at midnight local time. This pull request could be one way of implementing it: #931

@kbobrowski
Copy link
Contributor

by the way, it would be great if you could also reference existing github issues in pull requests (in this case #912), not only internal jira, would be much easier for the community to keep track of actions around the issues (I found this pull request just by accident), thanks!

@SamuraiKek
Copy link
Contributor Author

Hey @kbobrowski, from my understanding, it seems that the quota has always been calculated with device local time. I think there was a communication error with the guys from google and it was assumed that UTC would be used. Looking in the ENF documentation right now, it looks like it's been updated to clearly state that local time is used for both versions of the API.
CounterReset

@kbobrowski
Copy link
Contributor

kbobrowski commented Aug 24, 2020

Hi @SamuraiKek , I'm quite sure that calls count was set to zero at midnight UTC, the implementation of CWA actually was assuming local time in the past, which was leading to 39508 and was fixed by #818 .

Looking at the source code of GMS, version 20.30.19, it seems that currently it still defines internally "day" as midnight to midnight UTC, when it comes to setting the count to zero. Relevant part of the code:

 120     invoke-static {}, Ljava/lang/System;->currentTimeMillis()J
 121 
 122     move-result-wide v11
 127 
 129     sget-object v14, Ljava/util/concurrent/TimeUnit;->HOURS:Ljava/util/concurrent/TimeUnit;
 133 
 134     const-wide/16 v7, 0x18
 135 
 137     invoke-virtual {v14, v7, v8}, Ljava/util/concurrent/TimeUnit;->toMillis(J)J
 138 
 139     move-result-wide v7
 140 
 141     div-long/2addr v11, v7

And loose translation of count reset mechanism to java:

public class Quota {

  private long day = 0;
  private int count = 0;

  public void updateCount() {
    long currentDay = System.currentTimeMillis() / TimeUnit.HOURS.toMillis(24);
    if (currentDay > day) {
      day = currentDay;
      count = 0;
    }
    if (count < 20) {
      count += 1;
    }
  }

  public boolean canCallProvideDiagnosisKeys() {
    return count < 20;
  }
}

I think that change from UTC to local time is upcoming, non-documented breaking change that will be introduced with future version of GMS, or there is a mistake in documentation and it should state that count is set to zero at midnight UTC.

If you have rooted android device you can have a look at the place where it keeps track of the number of calls, in:

/data/data/com.google.android.gms/files/nearby/shared

It should be easy to test it though, from #818:

Current design allows for initiating RetrieveDiagnosisKeysTransaction at 15:00 CEST and then next day again at 01:00 CEST. This will result in exceeded quota, as it corresponds to 13:00 UTC and 23:00 UTC on the same day.

@SamuraiKek
Copy link
Contributor Author

Thanks @kbobrowski! It looks like you're right. We will try to clarify this with the team at Google before we take further action.

@jakobmoellerdev jakobmoellerdev marked this pull request as draft August 25, 2020 11:21
@jakobmoellerdev jakobmoellerdev marked this pull request as ready for review August 31, 2020 11:16
@sonarcloud
Copy link

sonarcloud bot commented Aug 31, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@jakobmoellerdev jakobmoellerdev merged commit 2ba3ec0 into dev Aug 31, 2020
@jakobmoellerdev jakobmoellerdev deleted the fix/Use-LocalTime-instead-of-UTC branch August 31, 2020 11:36
jakobmoellerdev added a commit that referenced this pull request Aug 31, 2020
Co-authored-by: Jakob Möller <jakob.moeller@sap.com>
jakobmoellerdev added a commit that referenced this pull request Sep 2, 2020
This reverts commit 2ba3ec0

This is done as the UTC communication was done falsely by Google and will be corrected in the docs. No action is necessary on our side.

Signed-off-by: d067928 <jakob.moeller@sap.com>
jakobmoellerdev added a commit that referenced this pull request Sep 2, 2020
This reverts commit 2ba3ec0

This is done as the UTC communication was done falsely by Google and will be corrected in the docs. No action is necessary on our side.

Signed-off-by: d067928 <jakob.moeller@sap.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
maintainers Tag pull requests created by maintainers sprint4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants