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

fix #1596 ブログ記事noが重複して保存される問題を改善 #1619

Merged
merged 10 commits into from
Jan 23, 2021

Conversation

ryuring
Copy link
Collaborator

@ryuring ryuring commented Jan 10, 2021

no, blog_content_id に複合キーを設定
- MySQL有効
- SQLiteはインストール時設定されない
- PostgreSQLは未検証

no と取得を保存を極力近づけできるだけ重複がないように調整

no, blog_content_id に複合キーを設定
- MySQL有効
- SQLiteはインストール時設定されない
- PostgreSQLは未検証

no と取得を保存を極力近づけできるだけ重複がないように調整
@ryuring ryuring requested a review from gondoh January 10, 2021 07:20
@ryuring ryuring added this to the 4.4.3 milestone Jan 10, 2021
@ryuring ryuring added the Bug バグ label Jan 10, 2021
@ryuring
Copy link
Collaborator Author

ryuring commented Jan 10, 2021

@gondoh 余計な空白除去が入ってしまって見にくいですが確認お願いします、、、

@ryuring
Copy link
Collaborator Author

ryuring commented Jan 10, 2021

Dockerで、PostgreSQLの環境を作ろうとしましたが、Debianにおける、libpq-dev に関連する依存関係の問題で無理っぽかったです。ディストリビューション変えた方がいいかも。

@ryuring
Copy link
Collaborator Author

ryuring commented Jan 11, 2021

むー、PostgreSQLが失敗する、、、やっぱ環境作らないとダメだな。。。
一旦、放置で大丈夫です。

複合キーへの対応が必要だったため
複合キーへの対応が必要だったため
@ryuring
Copy link
Collaborator Author

ryuring commented Jan 15, 2021

@gondoh やりました!PostgreSQLの環境も作って、テストも通して、動作も確認しました!

  • MySQL:有効
  • SQLite:インストール時設定されない
  • PostgreSQL:有効

4.4.3 でリリースしたいです。レビュー&マージよろしくお願いします。

@seto1
Copy link
Collaborator

seto1 commented Jan 19, 2021

@ryuring 動作を確認していたのですが、アイキャッチのファイル名が今までの形式と変わってしまうという問題があるようです。
今までは 2021/01/00032256_eye_catch.png のような形式でdbに登録されていましたが、変更後はアップロード時のファイル名のまま登録されます。

@seto1
Copy link
Collaborator

seto1 commented Jan 19, 2021

また、仕方ないような気がしますが同時操作を行った際に片方の画面で500エラーが表示されるのが気になりました。
以下のようにSQLのエラーが出ているので、tryで保存処理を挟んだりするとうまくbaser標準のフラッシュメッセージを出力できるかもしれません。

2021-01-19 11:22:50 Error: [PDOException] SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '2-32259' ..

@ryuring
Copy link
Collaborator Author

ryuring commented Jan 19, 2021

@seto1 確認ありがとうございます。

動作を確認していたのですが、アイキャッチのファイル名が今までの形式と変わってしまうという問題があるようです。
今までは 2021/01/00032256_eye_catch.png のような形式でdbに登録されていましたが、変更後はアップロード時のファイル名のまま登録されます。

これはまずいですね。考えます。

@yama
Copy link
Contributor

yama commented Jan 20, 2021

@ryuring

SQLiteはインストール時設定されない

SQLiteがインストール時に設定されないのって、何か理由があるのでしたっけ?

アイキャッチのファイル名が正常に取得できなかったため
@ryuring
Copy link
Collaborator Author

ryuring commented Jan 20, 2021

@seto1 アイキャッチファイル名の問題は解消しました
da6ea10

tryで保存処理を挟んだりするとうまくbaser標準のフラッシュメッセージを出力できるかもしれません。

これ横取りしたいんですが、Model::saveAssociated()throw しちゃってるので取れなかったです。

@yama CakeSchema での 複合インデックス生成に SQLiteが対応してなかっただけです

@fuchigam1
Copy link
Collaborator

@ryuring @yama アドバイスいただきました。

インデックス名は既存のカラム名とは重複しないようにしたほうがいいかな?と思いました。
no というカラム名がすでにあるので、テキトーですが index_id_no とか・・

@yama 既存のカラム名と重複しない方が良い理由ってなにか教えてもらって良いですか?

@yama
Copy link
Contributor

yama commented Jan 20, 2021

@materializing

@yama 既存のカラム名と重複しない方が良い理由ってなにか教えてもらって良いですか?

実際のデータアクセスには影響しないんですが、たとえばPHPのソースコードとして下記のように書いた場合、

$schema->tables['blog_posts']['indexes']['no']

ここだけを見ると、noカラムに対するインデックスだと読むのが普通だと思います。でも今回はそうではなく、2つのカラムに対する複合インデックスなので、それと分かるインデックス名にしたほうが間違いが起きにくいかな?と思いました。
・・というのもありますが、インデックス名を省略してインデックスを作成した場合はデフォルトで idx_name1_name2 みたいな名称がつくと思います。

@seto1
Copy link
Collaborator

seto1 commented Jan 20, 2021

@ryuring ありがとうございます。 アイキャッチが正しくアップロードされることを確認しました。
ただ、アイキャッチに関連してもう一つ見つけてしまいました..

アイキャッチ付きで同時に記事を追加しようとした場合、エラーが発生した方の画面でアップしようとしたアイキャッチ画像が削除されずに以下のパスに残ってしまうようです。

files/blog/ブログコンテンツID/blog_posts/ファイル名

これ横取りしたいんですが、Model::saveAssociated() が throw しちゃってるので取れなかったです。

ざっくりではあるんですが、BlogPostsController->admin_add を調節するとフラッシュメッセージの出力に成功しました。

// データを保存
try {
	if ($this->BlogPost->saveAll($this->request->data)) {
		// ...
	} else {
		$this->BcMessage->setError(__d('baser', 'エラーが発生しました。内容を確認してください。'));
	}
} catch (Exception $e) {
	$this->BcMessage->setError(__d('baser', 'エラーが発生しました。内容を確認してください。'));
}

@ryuring
Copy link
Collaborator Author

ryuring commented Jan 22, 2021

テストが失敗していたので調整しました。

@yama ありがとうございます。インデックス名は、 blog_content_id_no_index に変更しました。

@seto1 例外の件、モデル内でキャッチしようとしてうまくいかなかったんですが、提示頂いたとおりコントローラーだと成功しました。ありがとうございます。

また、アイキャッチの件は、afterSaveに行き着く前にコントローラーに処理が渡るのでちょっと難しいですね。
beforeSave で、files/blog_posts に保存し、afterSave で、該当日付のディレクトリに移動する仕様になっているみたいですが、 files/blog_posts に残ってしまうみたいです。
保存処理の仕組み自体を変える必要がありそうです。

こちらは別途 Issue を上げておきます。

@ryuring
Copy link
Collaborator Author

ryuring commented Jan 22, 2021

またもやテスト失敗。さっきの調整が影響してる。
もっかいやり直します。

@ryuring
Copy link
Collaborator Author

ryuring commented Jan 23, 2021

@gondoh テスト無事とおりました。よろしくお願いします。

@gondoh gondoh merged commit 7ab39f2 into baserproject:dev-4 Jan 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug バグ
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants