PersonalAccessTokenに対応しました #679

seraphr · 2024-09-09T21:30:56Z

インスタンス生成のためのbuild関数は互換性を維持しています
- class Resourceのコンストラクタ__init__は、互換性が壊れています

以下の事項について確認しています。

PersonalAccessTokenが有効になっている環境を対向としてcreate_test_project.pyによるテスト用プロジェクトの生成が成功すること
↑で作ったプロジェクトに対して、make testの大部分が成功すること
- 作ったばかりのプロジェクトには統計情報が無いので、そのあたりのテストがMissingResourceで失敗しました
- AnnotationZip関係も失敗してたけど、そちらは手動で作ったら通りました

* Unauthorize時のlogin&retry処理を、IdPassが渡されていた時のみにした * Patを渡された場合のlogin時、self.tokensにPatを転写するようにした * logoutの処理と対象になるように

yuji38kwmt · 2024-09-10T03:00:30Z

annofabapi/api.py

        self.endpoint_url = endpoint_url
        self.input_mfa_code_via_stdin = input_mfa_code_via_stdin
        self.url_prefix = f"{endpoint_url}/api/v1"
        self.session = requests.Session()

-        self.token_dict: Optional[Dict[str, Any]] = None
+        self.tokens: Union[Tokens, Pat, None] = None


興味による質問です。
self.tokensの型はUnion[HasAuthToken, None]でもよいように思ったのですが、Union[Tokens, Pat, None]の方がよいのでしょうか？

その定義だと、以下の部分で型チェックが通りません。
class HasAuthTokenは、別の場所で継承が可能であり、PatでもNoneでも無いことがわかっていても、Tokensであるとは限らないからです。
class HasAuthTokenは、PatとTokensに定義されているauth_tokenという関数が、同じ意味であることを示すためのマーカーとして定義していて、self.tokensの型として使うことを想定していません。

annofab-api-python-client/annofabapi/api.py

Lines 732 to 736 in 8f044b8

if isinstance(self.tokens, Pat):

self.tokens = None

return

request_body = self.tokens.to_dict()

annofab-api-python-client/annofabapi/api.py

Lines 753 to 756 in 8f044b8

if isinstance(self.tokens, Pat):

return

request_body = {"refresh_token": self.tokens.refresh_token}

Union[HasAuthToken, None]みたいな定義はオブジェクト指向的なサブタイプポリモフィズムを想定していて、Union[Tokens, Pat, None]みたいな定義は代数的データ型を想定しています。
それぞれの利点・欠点は、そのままオブジェクト指向的なデータ定義と代数的データ型の利点・欠点を引き継ぎます（cf. Expression Problem）

オブジェクト指向的なサブタイプポリモフィズムで十全に使えるように、PatとTokens（及び IdPass）を定義するのは、難しいとは言いませんが、その構造が今のapi.pyの構造から乖離するので面倒です。
loginやlogout、refreshの責務自体を（そうなったら名前は変わりますが）HasAuthTokenに持たせなければいけなくなるでしょう（PatとTokensの処理の大きな違いはその辺りであり、その差を吸収する必要があるため）。
また、その場合self.credentialsを含めて抽象化する必要があるでしょう。

今後近いうちに、パーソナルアクセストークンとパスワード認証に加えて新しい認証方法が追加されることが想定されている場合は、オブジェクト指向的に定義しておくと後の手間が省けます。
が、現状そういうことは想定していないので、実装の手間が低い方を選んでいます。

ありがとうございます。理解できました！

yuji38kwmt · 2024-09-10T03:26:13Z

annofabapi/util/type_util.py

+
+def assert_noreturn(x: NoReturn) -> NoReturn:
+    """python 3.11 以降に追加されたassert_neverの代わり"""
+    raise AssertionError(f"Invalid value: {x!r}")  # x!rは str(x)と等価


"x!rは str(x)"はrepr(x)の間違い？
https://docs.python.org/ja/3/library/string.html#format-string-syntax

おや、ホントだ

yuji38kwmt · 2024-09-10T07:17:17Z

annofabapi/resource.py

@@ -19,7 +20,7 @@ class Resource:
        login_user_id: AnnofabにログインするときのユーザID
        login_password: Annofabにログインするときのパスワード
        endpoint_url: Annofab APIのエンドポイント。
-        input_mfa_code_via_stdin: MFAコードを標準入力から入力するかどうか
+        input_mfa_code_via_stdin: MFAコードを標準入力から入力するかどうか Falseの時にMFAコードの入力を求められた場合は例外を送出する


Falseの時にMFAコードの入力を求められた場合は例外を送出する

これは、どの部分でどのような例外が送出されましたか？認識していなかったので質問しました。

雑にコード読んで判断したんですが、読み直してみると、ここの記述は正確ではないですね。
06eff37

yuji38kwmt · 2024-09-10T09:02:21Z

tests/test_build.py

@@ -0,0 +1,31 @@
+import os


補足情報です。
ファイル名にtest_local_というプレフィックスが付かないと、CIでテストが実行されません。

#680

最終的にはPytestのmarkers機能を使って、CIで実行しないテストを指定できるようにしたいです。

おっと、なるほど
renameしました

c0db97a

CIでのテスト対象とするため

seraphr added 5 commits September 10, 2024 03:48

api.pyをPATに対応して既存のテストが通るところまで

4bc7af8

環境変数・引数にPATを指定してインスタンスを生成できるようにした

46268b2

AnnofabApiをいくつか修正

2e99912

* Unauthorize時のlogin&retry処理を、IdPassが渡されていた時のみにした * Patを渡された場合のlogin時、self.tokensにPatを転写するようにした * logoutの処理と対象になるように

create_test_project.pyがendpointを引数に取れるようにした

471e243

Patをcredentialsに利用する場合のtokensへの転写をコンストラクタではなくリクエストを実行する直前に行うようにした

8f044b8

seraphr requested a review from yuji38kwmt September 9, 2024 21:30

README.mdを更新

d44fc73

seraphr marked this pull request as ready for review September 9, 2024 21:42

yuji38kwmt requested changes Sep 11, 2024

View reviewed changes

seraphr added 3 commits September 11, 2024 21:22

正しくないコメントを修正

c19f62e

input_mfa_code_via_stdin のコメントを修正

06eff37

rename test_build.py to test_local_build.py

c0db97a

CIでのテスト対象とするため

seraphr force-pushed the feature/678_pat branch from 453534d to c0db97a Compare September 11, 2024 12:35

yuji38kwmt merged commit e118b8f into main Sep 11, 2024
8 checks passed

yuji38kwmt deleted the feature/678_pat branch September 11, 2024 14:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PersonalAccessTokenに対応しました #679

PersonalAccessTokenに対応しました #679

seraphr commented Sep 9, 2024 •

edited

Loading

yuji38kwmt Sep 10, 2024 •

edited

Loading

seraphr Sep 11, 2024 •

edited

Loading

yuji38kwmt Sep 11, 2024

yuji38kwmt Sep 10, 2024

seraphr Sep 11, 2024

seraphr Sep 11, 2024

yuji38kwmt Sep 10, 2024

seraphr Sep 11, 2024 •

edited

Loading

yuji38kwmt Sep 10, 2024

seraphr Sep 11, 2024 •

edited

Loading

	if isinstance(self.tokens, Pat):
	self.tokens = None
	return

	request_body = self.tokens.to_dict()

	if isinstance(self.tokens, Pat):
	return

	request_body = {"refresh_token": self.tokens.refresh_token}

PersonalAccessTokenに対応しました #679

PersonalAccessTokenに対応しました #679

Conversation

seraphr commented Sep 9, 2024 • edited Loading

yuji38kwmt Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

seraphr Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

yuji38kwmt Sep 11, 2024

Choose a reason for hiding this comment

yuji38kwmt Sep 10, 2024

Choose a reason for hiding this comment

seraphr Sep 11, 2024

Choose a reason for hiding this comment

seraphr Sep 11, 2024

Choose a reason for hiding this comment

yuji38kwmt Sep 10, 2024

Choose a reason for hiding this comment

seraphr Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

yuji38kwmt Sep 10, 2024

Choose a reason for hiding this comment

seraphr Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

seraphr commented Sep 9, 2024 •

edited

Loading

yuji38kwmt Sep 10, 2024 •

edited

Loading

seraphr Sep 11, 2024 •

edited

Loading

seraphr Sep 11, 2024 •

edited

Loading

seraphr Sep 11, 2024 •

edited

Loading