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

ETag による通信量削減 #199 の解決 #233

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
Draft

ETag による通信量削減 #199 の解決 #233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
32c1c57
show case for issue https://github.com/cocoa-mhlw/cocoa/issues/199
i-maruyama May 28, 2021
d21e8cf
avoid error-log with the code: return "[]"
i-maruyama May 29, 2021
d5c0b2f
Merge branch 'cocoa-mhlw:develop' into issue#199
i-maruyama Jun 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Covid19Radar/Covid19Radar/Common/PreferenceKey.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ public static class PreferenceKey
// for preferences
public static string StartDateTime = "StartDateTime";
public static string LastProcessTekTimestamp = "LastProcessTekTimestamp";
public static string ETag = "Etag";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

なんの通信での Etag なのか分からなくなりますね。他のところで使いたくなると名前が衝突してしまいますし。
長いですが、 LastProcessTekEtag 等の方が良いのではないかと思います。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

取り込みます。

public static string ExposureNotificationConfiguration = "ExposureNotificationConfiguration";
public static string TermsOfServiceLastUpdateDateTime = "TermsOfServiceLastUpdateDateTime";
public static string PrivacyPolicyLastUpdateDateTime = "PrivacyPolicyLastUpdateDateTime";
Expand Down
5 changes: 4 additions & 1 deletion Covid19Radar/Covid19Radar/Services/ExposureNotificationHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,9 @@ public async Task FetchExposureKeyBatchFilesFromServerAsync(Func<IEnumerable<str
await submitBatches(downloadedFiles);

exposureNotificationService.SetLastProcessTekTimestamp(serverRegion, newCreated);
loggerService.Info($"region: {serverRegion}, lastCreated: {newCreated}");
var etag = Xamarin.Essentials.Preferences.Get("ETag", "");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

個人的にはここでHTTP通信である依存を入れたくないので他のクラスでEtagの値を引き回さない方が良いと思います。

個人的な案としては、HttpDataService のところでコメントします。

exposureNotificationService.SetETag(serverRegion, etag);
loggerService.Info($"region: {serverRegion}, lastCreated: {newCreated}, etag: {etag}");

// delete all temporary files
foreach (var file in downloadedFiles)
Expand Down Expand Up @@ -224,6 +226,7 @@ public async Task FetchExposureKeyBatchFilesFromServerAsync(Func<IEnumerable<str

var httpDataService = HttpDataService;

Xamarin.Essentials.Preferences.Set("ETag", ExposureNotificationService.GetETag(region));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExposureNotificationService （の中の PreferenceService） と Xamarin.Essentioals.Preferences の両方に Etag を持つ意味は何かありますか？

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. ETagによって、TekList をダウンロードするか決めます。
  2. LastProcessTektimestamp によって、各 TeK zip file をダウンロードするか決めます。
  3. すべての処理が終われば、両方更新します。

上に述べたように、　ETag の更新は最後に行うため、情報を最後まで保持する必要があります。そのための、Xamarin.Essentioals.Preferences です。

本当は、関数の引数の中と局所変数の中に ETag 情報を持ちたいのですが、変更する部分が大きくなって、うまい案が思いつかなかったというところで、ここはもう少し良案があればXamarin.Essentioals.Preferences から脱却したいところです。

List<TemporaryExposureKeyExportFileModel> tekList = await httpDataService.GetTemporaryExposureKeyList(region, cancellationToken);
if (tekList.Count == 0)
{
Expand Down
39 changes: 31 additions & 8 deletions Covid19Radar/Covid19Radar/Services/ExposureNotificationService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ public interface IExposureNotificationService

long GetLastProcessTekTimestamp(string region);
void SetLastProcessTekTimestamp(string region, long created);
string GetETag(string region);
void SetETag(string region, string etag);
void RemoveLastProcessTekTimestamp();

Task FetchExposureKeyAsync();
Expand Down Expand Up @@ -170,13 +172,23 @@ public async Task FetchExposureKeyAsync()
}

public long GetLastProcessTekTimestamp(string region)
{
return GetKey<long>(region, PreferenceKey.LastProcessTekTimestamp, 0L);
}

public string GetETag(string region)
{
return GetKey<string>(region, PreferenceKey.ETag, "");
}

public T GetKey<T>(string region, string key, T def)
{
loggerService.StartMethod();
var result = 0L;
var jsonString = preferencesService.GetValue<string>(PreferenceKey.LastProcessTekTimestamp, null);
var result = def;
var jsonString = preferencesService.GetValue<string>(key, null);
if (!string.IsNullOrEmpty(jsonString))
{
var dict = JsonConvert.DeserializeObject<Dictionary<string, long>>(jsonString);
var dict = JsonConvert.DeserializeObject<Dictionary<string, T>>(jsonString);
if (dict.ContainsKey(region))
{
result = dict[region];
Expand All @@ -187,27 +199,38 @@ public long GetLastProcessTekTimestamp(string region)
}

public void SetLastProcessTekTimestamp(string region, long created)
{
SetKey<long>(region, PreferenceKey.LastProcessTekTimestamp, created);
}

public void SetETag(string region, string etag)
{
SetKey<string>(region, PreferenceKey.ETag, etag);
}

public void SetKey<T>(string region, string key, T created)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Etag の値を Dictionary で保存するのは少々大げさな気がしますね。
メソッドを共通化するほどでは無いかなと。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

プログラム的な重要さは、 LastProcessTektimestamp と同じと考えますので、同じ処理にしたいと考えています。

  1. ETagによって、TekList をダウンロードするか決めます。
  2. LastProcessTektimestamp によって、各 TeK zip file をダウンロードするか決めます。
  3. すべての処理が終われば、両方更新します。

{
loggerService.StartMethod();
var jsonString = preferencesService.GetValue<string>(PreferenceKey.LastProcessTekTimestamp, null);
Dictionary<string, long> newDict;
var jsonString = preferencesService.GetValue<string>(key, null);
Dictionary<string, T> newDict;
if (!string.IsNullOrEmpty(jsonString))
{
newDict = JsonConvert.DeserializeObject<Dictionary<string, long>>(jsonString);
newDict = JsonConvert.DeserializeObject<Dictionary<string, T>>(jsonString);
}
else
{
newDict = new Dictionary<string, long>();
newDict = new Dictionary<string, T>();
}
newDict[region] = created;
preferencesService.SetValue(PreferenceKey.LastProcessTekTimestamp, JsonConvert.SerializeObject(newDict));
preferencesService.SetValue(key, JsonConvert.SerializeObject(newDict));
loggerService.EndMethod();
}

public void RemoveLastProcessTekTimestamp()
{
loggerService.StartMethod();
preferencesService.RemoveValue(PreferenceKey.LastProcessTekTimestamp);
preferencesService.RemoveValue(PreferenceKey.ETag);
loggerService.EndMethod();
}

Expand Down
24 changes: 20 additions & 4 deletions Covid19Radar/Covid19Radar/Services/HttpDataService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
using System.IO;
using System.Threading;
using System.Net;
using System.Linq;
using Newtonsoft.Json;

namespace Covid19Radar.Services
Expand Down Expand Up @@ -169,14 +170,29 @@ private async Task<string> GetAsync(string url, CancellationToken cancellationTo
}
private async Task<string> GetCdnAsync(string url)
{
Task<HttpResponseMessage> response = downloadClient.GetAsync(url);
HttpResponseMessage result = await response;
await result.Content.ReadAsStringAsync();
var etag = Xamarin.Essentials.Preferences.Get("ETag", "");
var request = new HttpRequestMessage(HttpMethod.Get, url);
if (! string.IsNullOrEmpty(etag))
{
request.Headers.TryAddWithoutValidation("If-None-Match", etag);
}
var result = await downloadClient.SendAsync(request);
var status = result.StatusCode;
var headers = result.Headers;
var etags = headers.GetValues("ETag");
etag = etags.First();
Comment on lines +182 to +183
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

一応 responce に Etag Header が存在しないケースもカバーして置いた方がよりよいかと。

Xamarin.Essentials.Preferences.Set("ETag", etag);

if (result.StatusCode == System.Net.HttpStatusCode.OK)
await result.Content.ReadAsStringAsync(); // Is this line important?

if (status == System.Net.HttpStatusCode.OK)
{
return await result.Content.ReadAsStringAsync();
}
else if (status == System.Net.HttpStatusCode.NotModified)
{
return "[]";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ごく個人的な意見ですが、ここで前回の Content と同じデータを返してしまえば HTTP のレイヤーで変更範囲を封じ込めることが出来そうです。
Content を保存する場所が必要ですが、数kbyte 程度なので影響は少ないかなと。
流石に preferences に保存するのは気が引けますが。

Copy link
Contributor Author

@i-maruyama i-maruyama Jun 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

この後の関数の部分で、中身がなければ、次の処理に進まずに終了するルーチンがあります。処理を軽くするためには、この方が良いです。

cocoa/Covid19Radar/Covid19Radar/Services/ExposureNotificationHandler.cs

Lines 227 to 233 in c29d7a7

List<TemporaryExposureKeyExportFileModel> tekList = await httpDataService.GetTemporaryExposureKeyList(region, cancellationToken);
if (tekList.Count == 0)
{
loggerService.EndMethod();
return (-1, downloadedFiles);
}
Debug.WriteLine("C19R Fetch Exposure Key");

}
return null;
}
private async Task<string> GetCdnAsync(string url, CancellationToken cancellationToken)
Expand Down