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

重複したイベント/部屋の作成禁止 #376

Merged
merged 13 commits into from
Jun 19, 2023

Conversation

ras0q
Copy link
Member

@ras0q ras0q commented May 2, 2023

close #369

@ras0q
Copy link
Member Author

ras0q commented May 2, 2023

マイグレーションがまだ

@ras0q ras0q marked this pull request as ready for review May 14, 2023 01:20
@ras0q
Copy link
Member Author

ras0q commented May 14, 2023

image

image

@ras0q
Copy link
Member Author

ras0q commented May 14, 2023

これ進捗部屋じゃなくてイベントだ

@ras0q ras0q changed the title name,time_start,time_endの複合ユニークキーを貼る 重複したイベント/部屋の作成禁止 May 14, 2023
@hijiki51
Copy link
Member

hijiki51 commented May 14, 2023

いま見かけたから聞くんですがこれってZoom/Discordの時部屋重複したりしますよね

イベントはいいけど部屋はいまの設計だとDBじゃなくてアプリケーション側で重複管理しないといけないと思います

@ras0q
Copy link
Member Author

ras0q commented May 14, 2023

確かに全く同じ時間に違う名前のイベントが同じ部屋(zoom等含む)で開催されてたらそうなるか

@hijiki51
Copy link
Member

hijiki51 commented May 14, 2023

image

@hijiki51
Copy link
Member

あとこういう風に「未定」があります

@ras0q
Copy link
Member Author

ras0q commented May 14, 2023

type Room struct {
	ID             uuid.UUID `gorm:"type:char(36);primaryKey"`
	Place          string    `gorm:"type:varchar(32); uniqueIndex:idx_place_time_start_time_end"`
	Verified       bool
	TimeStart      time.Time `gorm:"type:DATETIME; index; uniqueIndex:idx_place_time_start_time_end"`
	TimeEnd        time.Time `gorm:"type:DATETIME; index; uniqueIndex:idx_place_time_start_time_end"`
	Events         []Event   `gorm:"->; constraint:-"` // readOnly
	Admins         []RoomAdmin
	CreatedByRefer uuid.UUID `gorm:"type:char(36);" cvt:"CreatedBy, <-"`
	CreatedBy      User      `gorm:"->; foreignKey:CreatedByRefer; constraint:OnDelete:CASCADE;" cvt:"->"`
	Model          `cvt:"->"`
}

Eventsあるけど同じ時間のイベントはここに入って部屋自体が重複することはないみたいなロジックがあればよさそう
まだ見てないけど今は多分進捗部屋のときしかここ複数になりえない気がする

@hijiki51
Copy link
Member

hijiki51 commented May 14, 2023

Roomに対してイベントの重複がないってロジックならよいと思います。
ただし、述べてもらっている通り、進捗部屋以外という条件が必要です。

いま進捗部屋以外の部屋って登録してましたっけ? OR 手動で(CSVではなく)簡単に登録できるようになってましたっけ
(ないなら今登録されてるのは進捗部屋だけなので重複管理いらないかなと)

@ras0q
Copy link
Member Author

ras0q commented May 14, 2023

進捗部屋以外の部屋もDBには登録されてます
フロントエンドからは部屋名だけ渡されるのでその都度部屋が作成されてるっぽい
image

@ras0q
Copy link
Member Author

ras0q commented May 14, 2023

手元で動かしてたけどこれm.InitSchema動かなくてインデックス貼れてないな
なぜさっき貼れてたかわからないけど

→ 手元で繰り返しv14実行してたからでした、migrationテーブルから14消したらできた

→ 動いてるけどv14()する前にインデックス貼ろうとして失敗してそう

@ras0q
Copy link
Member Author

ras0q commented May 14, 2023

v13() v14()の中にインデックス追加する処理書くのって許されますか書いた

@ras0q
Copy link
Member Author

ras0q commented May 17, 2023

  • 名前、開始時刻、終了時刻が重複するイベントは作れない
  • 名前、開始時刻、終了時刻が重複するイベントは作れない
  • 名前(場所)、開始時刻、終了時刻 が重複する即時部屋は新規作成する代わりに既にある部屋を使う
  • ↑のときに部屋のadminをどうにかする

@ras0q
Copy link
Member Author

ras0q commented May 17, 2023

進捗部屋はIDを指定するからadminの変更等は考えなくていい

@ras0q
Copy link
Member Author

ras0q commented May 17, 2023

部屋重複させてadmin増やしたらさすがに脆弱性生みそうだけどどうするのがいいんだろ
最初に作った人固定(今のまま)でいい?

@ras0q
Copy link
Member Author

ras0q commented May 17, 2023

これは多分作成者が未定って指定しただけ(デフォルトではない)ので他の部屋と同じ扱いでよさそう

あとこういう風に「未定」があります

@oribe1115 oribe1115 self-requested a review May 19, 2023 02:14
Copy link

@oribe1115 oribe1115 left a comment

Choose a reason for hiding this comment

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

migrationの方はまだ見れてないのですが、現状で気になったところを書きました

infra/db/hooks.go Outdated Show resolved Hide resolved
infra/db/event_test.go Show resolved Hide resolved
infra/db/room_test.go Show resolved Hide resolved
infra/db/room_test.go Show resolved Hide resolved
Copy link

@oribe1115 oribe1115 left a comment

Choose a reason for hiding this comment

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

これ、重複イベント/部屋が作成された際に最終的に返るstatus codeで500になってそうです?
アプリケーションのロジックで弾いているものなので、Internal Server ErrorではなくBad Requestを返すようにするのが適切かと思います

@ras0q
Copy link
Member Author

ras0q commented May 19, 2023

レビューありがとうございます:bow:
イベント重複のときは400が返るようになっているはずですね:thiking:

knoq_development_app         | 2023-05-19T15:27:13.220+0900     INFO    router/middleware.go:63 client error    {"field": {"requestMethod": "POST", "requestUrl": "/api/events", "requestSize": "260", "status": 400, "responseSize": "54", "userAgent": "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/115.0.0.0 Safari/537.36 Edg/115.0.0.0", "remoteIp": "172.26.0.1", "serverIp": "", "referer": "http://localhost:6006/events/new", "latency": "0.220820805s", "protocol": "HTTP/1.1", "error": "/srv/knoq/router/errors.go:117: bad request: duplicate entry"}}

migration/v13.go Outdated Show resolved Hide resolved
Copy link

@oribe1115 oribe1115 left a comment

Choose a reason for hiding this comment

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

遅くなりましたがmigrationの実装読みました
実行は試していませんが、記述とテスト結果的にはロジックよさそうです
v13の方のみにコメントしましたが、v14も同じコメントになりそうです

migration/v13.go Show resolved Hide resolved
migration/v13.go Show resolved Hide resolved
@ras0q ras0q force-pushed the feat/unique-event-on-same-time branch from 7a972b2 to f47d40f Compare June 3, 2023 04:08
@ras0q
Copy link
Member Author

ras0q commented Jun 11, 2023

@oribe1115 修正しました:pray:

Copy link

@oribe1115 oribe1115 left a comment

Choose a reason for hiding this comment

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

よさそうです!

@ras0q ras0q merged commit 01e2de3 into main Jun 19, 2023
@ras0q ras0q deleted the feat/unique-event-on-same-time branch June 19, 2023 02:21
ras0q added a commit that referenced this pull request Jun 24, 2023
…same-time"

This reverts commit 01e2de3, reversing
changes made to 80f4a6e.
ras0q added a commit that referenced this pull request Jun 24, 2023
Revert "Merge pull request #376 from traPtitech/feat/unique-event-on-same-time"
@ras0q
Copy link
Member Author

ras0q commented Jul 17, 2023

(n)を付けたせいで文字数オーバーになるイベントがある

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重に登録してしまう
3 participants