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

Migrate to shared keychain #664

Merged
merged 27 commits into from
Aug 20, 2022
Merged

Migrate to shared keychain #664

merged 27 commits into from
Aug 20, 2022

Conversation

bannzai
Copy link
Owner

@bannzai bannzai commented Aug 14, 2022

Abstract

Firebase AuthのKeychainでuserAccessGroupを使用する。これによりWidgetExtensionなどのApp ExtensionとKeychainを共有できるメリットが挙げられる。iOSのWidget Extension対応に関連して行う

Why

Links

Checklist

正常系

  • 既存からの移行
    • 2度目の起動
    • method channelの順番を変更したので確認
  • 新規登録
    • 2度目の起動
    • method channelの順番を変更したので確認
  • Androidで起動ができる
    • 2度目の起動
  • 古いバージョンからのアプデ後にクイックレコードができる

異常系

  • method channel から移行結果がfalseの場合にクラッシュする

途中から失敗

  • updateCurrentUser でエラーが起きた場合に一度アプリがクラッシュする。その後の起動で正常に移行が完了するかを確認 意図的にエラーを起こすのが難しいのでパス

Checked

  • Analyticsのログを入れたか
  • Navigator.of(context).pop() の後にContextを使用したメソッドを実行していない
  • 境界値に対してのUnitTestを書いた
  • パターン分岐が発生するWidgetに対してWidgetTestを書いた
  • リリースノートを追加した

@bannzai
Copy link
Owner Author

bannzai commented Aug 19, 2022

起動時にやると起動処理が都度走って微妙に遅く感じるのでappGroupが必要なユーザーに対してオプトインが良いかも。と思い始めた。起動処理が遅い理由は

  • MethodChannel経由の呼び出しだと少し時間がかかるかも
  • MethodChannelの確率が実は非同期で、Migration済みかどうかのチェックが返ってくるのが Channelの確立 -> メソッドの実行。という順序だとすると前者のChannelの確立で時間がかかっている可能性がある

2つ目の理由が今の所濃厚

@bannzai
Copy link
Owner Author

bannzai commented Aug 20, 2022

一回このPRはマージしてRevertする。必要はなくなった。ただ、必要になる可能性があるのでその時にはこのPRを利用する

不要な理由

UserDefaultsのAppGroupsを使用してデータのやり取りをする。で十分な気がしてきた。メリットは一番依存関係が少なく単純だからである

UserDefaultsを使わない場合のデメリット

  • FirestoreをSwift側で使うのでFirebase AuthとFirestoreを依存関係に含める必要がある
  • Entityの定義とかがSwiftにはないので再定義をする必要がある。常にFlutter側と合わせる必要もある
    • とはいえ一度定義しちゃえばそんなに気にならないが、UserDetaultsの方がお手軽に感じた
    • Entityが無い場合、とかもあるので気をつける必要がある
    • PilllSheet -> PillSheetGroupになった時のようにEntity自体が変わる可能性もある
  • 通信処理なのでエラーのことも考慮する必要がある
  • Authのデータを共有するためにどこかのタイミングでKeychain AccessをWidgetKitにも共有できるようにする必要がある
    • 正直これが一番ネック。Pilllの場合Anonymousユーザーが多いので移行に失敗した時の回復策が無い

UserDetaulsを使う場合のデメリット

  • Single Source of Truth が保てなくなる。FirestoreとUserDetaultsの情報を合わせる必要がある。なのでデータ不整合のバグが起きる可能性はある
    • とはいえ今回の例で言うとUserDetaulsに対してデータを同期する処理になるのでそんなに難しいかと言われるとそうでもない

@bannzai bannzai marked this pull request as ready for review August 20, 2022 08:01
@bannzai bannzai merged commit 328c32c into main Aug 20, 2022
@bannzai bannzai deleted the add/function/migrate-to-keychain branch August 20, 2022 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant