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

新規Work作成時にWorksレコード作成完了後エラーが発生するとThumbnailレコードが生成されず関連ページを開けなくなる #155

Open
claustra01 opened this issue Jul 14, 2024 · 0 comments
Labels
bug Something isn't working

Comments

@claustra01
Copy link
Member

claustra01 commented Jul 14, 2024

ここで逐次Commitしているため、このCommit後にDBコネクションエラーが発生すると、Worksレコードは存在するがTagsレコードやThumbnailsレコードは存在しない状況が発生する可能性がある(発生した)

md = markdown.Markdown(extensions=["tables"])
work_orm = models.Work(
title=title,
description=description,
description_html=md.convert(description),
user_id=user_id,
visibility=visibility,
)
db.add(work_orm)
db.commit()
db.refresh(work_orm)
# assetのwork_idの更新
for asset_id in assets_id:
asset_orm = db.query(models.Asset).get(asset_id)
if asset_orm is None:
raise HTTPException(status_code=400, detail="This asset id is invalid.")
asset_orm.work_id = work_orm.id
# url_infoテーブルへのインスタンスの作成
for url in urls:
create_url_info(
db, url.get("url"), url.get("url_type", "other"), work_orm.id, user_id
)
# tagの中間テーブルへのインスタンスの作成
for tag_id in tags_id:
tagging_orm = models.Tagging(work_id=work_orm.id, tag_id=tag_id)
db.add(tagging_orm)
db.commit()
# Thumbnailの中間テーブルへのインスタンスの作成
if thumbnail_asset_id:
thumbnail = db.query(models.Asset).get(thumbnail_asset_id)
if thumbnail is None:
raise HTTPException(
status_code=400, detail="This thumbnail asset id is invalid."
)
thumbnail_orm = models.Thumbnail(work_id=work_orm.id, asset_id=thumbnail.id)
db.add(thumbnail_orm)
db.commit()

この実装のようにトランザクションを実装してあげれば解決しそう

def replace_blog(
db: Session,
blog_id: str,
user_id: str,
title: str,
body_text: str,
visibility: Visibility,
thumbnail_asset_id: str,
assets_id: list[str],
tags_id: list[str],
published_at: Optional[datetime],
) -> Blog:
blog_orm = db.query(blog_models.Blog).get(blog_id)
if blog_orm is None:
raise HTTPException(status_code=404, detail="blog is not found")
if blog_orm.user_id != user_id:
raise HTTPException(status_code=403, detail="this work's author isn't you")
if title == "":
raise HTTPException(status_code=400, detail="title is empty")
if thumbnail_asset_id == "":
raise HTTPException(status_code=400, detail="thumbnail is empty")
# 本来はwithを使うべきだが、ネストがかなり深くなってしまうのでこのような書き方をする
if db.transaction is None:
db.begin()
try:
# DB更新
blog_orm.title = title
blog_orm.body_text = body_text
blog_orm.visibility = visibility
blog_orm.published_at = published_at
# 使用していないassetsの紐付けを削除
old_assets_orm = (
db.query(blog_models.BlogAsset).filter_by(blog_id=blog_id).all()
)
for old_asset_orm in old_assets_orm:
if old_asset_orm.id not in assets_id:
old_asset_orm.blog_id = None
# 新しいassetsの紐付け
new_assets_orm = (
db.query(blog_models.BlogAsset)
.filter(blog_models.BlogAsset.id.in_(assets_id))
.all()
)
not_found_assets_id = set(assets_id) - set(
[new_asset_orm.id for new_asset_orm in new_assets_orm]
)
if not_found_assets_id:
raise HTTPException(
status_code=400,
detail=f'asset_id "{not_found_assets_id}" is invalid',
)
for new_asset_orm in new_assets_orm:
# ここでN+1問題が発生しているが、assetsはそんなに多くならないので許容する
if new_asset_orm.blog_id != blog_id:
new_asset_orm.blog_id = blog_id
# 使用していないtagsの紐付け中間テーブルを削除
db.query(blog_models.BlogTagging).filter(
blog_models.BlogTagging.blog_id == blog_id,
blog_models.BlogTagging.tag_id.notin_(tags_id),
).delete()
# 新しいtagsを中間テーブルで紐付け
new_tags_orm = (
db.query(blog_models.BlogTagging)
.filter(blog_models.BlogTagging.tag_id.in_(tags_id))
.all()
)
not_found_tags_id = set(tags_id) - set(
[new_tag_orm.tag_id for new_tag_orm in new_tags_orm]
)
if not_found_tags_id:
raise HTTPException(
status_code=400,
detail=f'tag_id "{not_found_tags_id}" is invalid',
)
for new_tag_orm in new_tags_orm:
# assetsと同様にここにもN+1問題がある
if (
not db.query(blog_models.BlogTagging)
.filter_by(blog_id=blog_id, tag_id=new_tag_orm.tag_id)
.all()
):
new_tagging_orm = blog_models.BlogTagging(
blog_id=blog_id, tag_id=new_tag_orm.tag_id
)
db.add(new_tagging_orm)
# Thumbnailを中間テーブルで紐付け
thumbnail = db.query(blog_models.BlogAsset).get(thumbnail_asset_id)
if thumbnail is None:
raise HTTPException(
status_code=400,
detail=f'thumbnail_asset_id "{thumbnail_asset_id}" is invalid',
)
thumbnail_orm = (
db.query(blog_models.BlogThumbnail).filter_by(blog_id=blog_id).first()
)
if thumbnail_orm.asset_id != thumbnail.id:
new_thumbnail_orm = blog_models.BlogThumbnail(
blog_id=blog_id, asset_id=thumbnail.id
)
db.add(new_thumbnail_orm)
db.commit()
except SQLAlchemyError as e:
db.rollback()
raise HTTPException(
status_code=500, detail=f"database transaction error: {str(e)}"
)
finally:
blog = Blog.from_orm(blog_orm)
return blog

Blog作成やWork更新などでも同じ実装になっているため、最終的には全部トランザクションを使ったCRUDsに置き換えないとまた同じ問題が発生する可能性が高い

@claustra01 claustra01 added the bug Something isn't working label Jul 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant