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

add user reserved #302

Merged
merged 3 commits into from
Dec 15, 2022
Merged

add user reserved #302

merged 3 commits into from
Dec 15, 2022

Conversation

iiiidaaa
Copy link
Contributor

ユーザー予約機能を追加します
以下のgRPCエンドポイントを追加します

  • List

    • 条件のProjectID、UserIdpKeyに合致するリストの取得
  • Put

    • UserReservedの追加
    • ユーザーが既に存在する場合、登録するロールがプロジェクトに紐づかない場合にエラーとします
  • Delete

    • PrimaryKeyを元にUserReservedを削除します

    PutUser処理にて、新規にユーザーを作成した場合にUserReservedからロールをアタッチする処理を追加します

pkg/db/iam.go Outdated Show resolved Hide resolved
t.Fatalf("Unexpected error: %+v", err)
}
// 自動生成されるタイムスタンプをwantで指定できないのでそれ以外の値を比較
if c.want != nil && !((got.ReservedID == c.want.ReservedID) && (got.RoleID == c.want.RoleID) && (got.UserIdpKey == c.want.UserIdpKey)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

自動生成されるタイムスタンプをwantで指定できないのでそれ以外の値を比較

mockが何を返すかもWillReturnRowsなどで指定していると思うので、例えば now を返すようにしてやれば単純な比較にできませんか?

Copy link
Collaborator

Choose a reason for hiding this comment

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

sqlmock.NewRows(
	[]string{"reserved_id", "role_id", "user_idp_key", "created_at", "updated_at"}).
	AddRow(1, 1, "user", now, now).
	AddRow(2, 1, "user", now, now).
	...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

まず、現状のテストだとExpectExecを使用しているためWillReturnRowsが使えません。(WillReturnRowsが*sqlmock.ExpectedQueryのメソッドのため)

そのためExpectQueryを使用するように修正しようとしましたが、こちらだと呼び出しが期待されたものと異なってくるためにエラーが発生してテストを通すことができません。
(ExpectQueryを使用した際にQueryを期待しているが、ExecQueryが呼び出されるため)

そのため、現状の形にしています。
良い修正案などあればご教示いただければと思います。
(FirstOrCreateを使ってるとExpectQueryが使えないため、テスト元のコードまで修正が必要そうな気がしてます)

Copy link
Collaborator

@gassara-kys gassara-kys Dec 15, 2022

Choose a reason for hiding this comment

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

ありがとうございます。WillReturnRows使えない旨、理解しました。

mockで特に指定してないのに結果の比較を行っていることが少し違和感がありますね、、、
例えばINSERTの処理のテストでReservedID(自動インクリメント)のパラメータがない場合はうまく動きますか?
(ブラックボックス過ぎてテストコードから期待値と結果が同じになることが分からないという印象)

個人的にはエラーの有無の確認だけでも良いかもしれないなと思いました。

pkg/server/iam/user_reserved_test.go Outdated Show resolved Hide resolved
pkg/db/iam.go Outdated Show resolved Hide resolved
mockの戻り値を比較するだけのものとなっていたため、関数の戻り値のチェックを削除しました
Copy link
Collaborator

@gassara-kys gassara-kys left a comment

Choose a reason for hiding this comment

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

LGTM

@iiiidaaa iiiidaaa merged commit f4e1769 into master Dec 15, 2022
@iiiidaaa iiiidaaa deleted the add-user-reserved branch December 15, 2022 06:17
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.

2 participants