-
Notifications
You must be signed in to change notification settings - Fork 2
PersonalAccessTokenに対応しました #679
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
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
4bc7af8
api.pyをPATに対応して既存のテストが通るところまで
seraphr 46268b2
環境変数・引数にPATを指定してインスタンスを生成できるようにした
seraphr 2e99912
AnnofabApiをいくつか修正
seraphr 471e243
create_test_project.pyがendpointを引数に取れるようにした
seraphr 8f044b8
Patをcredentialsに利用する場合のtokensへの転写をコンストラクタではなくリクエストを実行する直前に行うようにした
seraphr d44fc73
README.mdを更新
seraphr c19f62e
正しくないコメントを修正
seraphr 06eff37
input_mfa_code_via_stdin のコメントを修正
seraphr c0db97a
rename test_build.py to test_local_build.py
seraphr File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
from dataclasses import dataclass | ||
from typing import Dict, Protocol | ||
|
||
|
||
class HasAuthToken(Protocol): | ||
@property | ||
def auth_token(self) -> str: ... | ||
|
||
|
||
@dataclass(frozen=True) | ||
class IdPass: | ||
user_id: str | ||
password: str | ||
|
||
|
||
@dataclass(frozen=True) | ||
class Pat(HasAuthToken): | ||
"""Personal Access Token""" | ||
|
||
token: str | ||
|
||
@property | ||
def auth_token(self) -> str: | ||
return f"Bearer {self.token}" | ||
|
||
|
||
@dataclass(frozen=True) | ||
class Tokens(HasAuthToken): | ||
"""IdPassを元にログインしたあとに取得されるトークン情報""" | ||
|
||
id_token: str | ||
access_token: str | ||
refresh_token: str | ||
|
||
@property | ||
def auth_token(self) -> str: | ||
return self.id_token | ||
|
||
def to_dict(self) -> Dict[str, str]: | ||
return { | ||
"id_token": self.id_token, | ||
"access_token": self.access_token, | ||
"refresh_token": self.refresh_token, | ||
} | ||
|
||
@staticmethod | ||
def from_dict(d: Dict[str, str]) -> "Tokens": | ||
return Tokens(id_token=d["id_token"], access_token=d["access_token"], refresh_token=d["refresh_token"]) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
興味による質問です。
self.tokens
の型はUnion[HasAuthToken, None]
でもよいように思ったのですが、Union[Tokens, Pat, None]
の方がよいのでしょうか?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
その定義だと、以下の部分で型チェックが通りません。
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
annofab-api-python-client/annofabapi/api.py
Lines 753 to 756 in 8f044b8
Union[HasAuthToken, None]
みたいな定義はオブジェクト指向的なサブタイプポリモフィズムを想定していて、Union[Tokens, Pat, None]
みたいな定義は代数的データ型を想定しています。それぞれの利点・欠点は、そのままオブジェクト指向的なデータ定義と代数的データ型の利点・欠点を引き継ぎます(cf.
Expression Problem
)オブジェクト指向的なサブタイプポリモフィズムで十全に使えるように、
Pat
とTokens
(及びIdPass
)を定義するのは、難しいとは言いませんが、その構造が今のapi.pyの構造から乖離するので面倒です。login
やlogout
、refresh
の責務自体を(そうなったら名前は変わりますが)HasAuthToken
に持たせなければいけなくなるでしょう(Pat
とTokens
の処理の大きな違いはその辺りであり、その差を吸収する必要があるため)。また、その場合
self.credentials
を含めて抽象化する必要があるでしょう。今後近いうちに、パーソナルアクセストークンとパスワード認証に加えて新しい認証方法が追加されることが想定されている場合は、オブジェクト指向的に定義しておくと後の手間が省けます。
が、現状そういうことは想定していないので、実装の手間が低い方を選んでいます。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとうございます。理解できました!