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 side-article filter condition from created_at to id #213

Closed

Conversation

jessyoon14
Copy link
Contributor

@jessyoon14 jessyoon14 commented Jul 18, 2021

이슈 [#212 ]에 적은 내용입니다:

정확히 같은 시간에 작성된 글들 사이에 side_articles에서 무한 루프 버그가 발생합니다.

문제

같은 시간에 작성된 A, B 게시글에 대해, A의 before & after가 B가 되며, B의 before & after가 A가 됩니다.
예시: https://newara.sparcs.org/post/231012?from_view=board

자세한 상황은 노션 카드 참고: https://www.notion.so/sparcs/side_articles-1ec72349d9be4734ae601a929211f510

기존 코드 (serializers/article.py , get_side_articles)

before = articles.filter(created_at__lte=obj.created_at).first()
after = articles.filter(created_at__gte=obj.created_at).last()

기존 코드의 문제점:

  • created_at__lte, created_at__gte 둘다 equality가 포함됨. 그래서 정확하게 같은 시간에 2개의 글 A & B가 올라올 경우, A의 before & after article이 둘다 B가 됨. (B의 before & after article도 둘다 A가 됨)
  • lte, gte를 lte, gt로 바꿔준다고 해도, 순서가 게시판 순서와 달라짐.
    • ex) A = B < C 순서로 글이 올라왔을때,
      • (A의 이전글: B, 이후글: C) , (B의 이전글: A, 이후글: C )가 됨.
      • 이렇게 되면 안되고, A의 이전글: null, 이후글: C가 되어야함.

해결 방법

시간 순으로 정렬할 경우, 2개의 글이 동시에 올라운 경우 순서를 정의할 수 없음. 게시판 순서를 그대로 표현하기 위해 id 순으로 정렬후, id_lt, id_gt로 수정했습니다.

@jessyoon14 jessyoon14 self-assigned this Jul 18, 2021
@jessyoon14 jessyoon14 changed the base branch from master to develop July 18, 2021 13:45
Copy link
Member

@jungnoh jungnoh left a comment

Choose a reason for hiding this comment

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

정렬성에도 문제 없을것?같고?
id로 하니까 인덱스도 더 잘타겠네요.
👍

@iv-y
Copy link
Contributor

iv-y commented Jul 18, 2021

정렬성에 문제가 없...? 나요? 🤔
es 구현할 때 기억을 더듬어 보면, 이전 ara에서 migration된 글들과 (3년치) 크롤링한 포탈 공지들이 시간 순과 id 순이 달랐던 것 같아요. 이외에도 포탈공지가 크롤링이 안돼서 몰아서 크롤링하는 경우 시간 순과 id 순이 달라질 수 있을 것 같습니다.

Copy link
Member

@victory-jooyon victory-jooyon left a comment

Choose a reason for hiding this comment

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

@iv-y 님 말이 맞습니다. 생성시간 더 나중이라고 해서 id가 더 크다고 볼수없어요. (대부분은 그렇겠지만 항상 그렇다고 볼 수 없습니다.)
공동코딩때 더 나은 방법을 생각해보면 좋을 것 같습니다.

@jungnoh
Copy link
Member

jungnoh commented Jul 19, 2021

아 포탈공지 글이 있군요..! 인정합니다.

@victory-jooyon victory-jooyon requested review from jungnoh and removed request for jungnoh July 19, 2021 02:19
@victory-jooyon
Copy link
Member

created_at 이 정확히 일치하는 경우는, 거의 99% 포탈공지글에서 정확히 같은 글이 두개 이상 올라왔을때 일 것 같은데요,
image
포탈공지글에서 제목+내용이 같은 글들을 한번 정리해주고, 그리고 포탈크롤링 코드에서 같은 제목+내용이면 하나만 생성되도록 수정해주면 좋을 것 같습니다.

@jessyoon14
Copy link
Contributor Author

jessyoon14 commented Jul 19, 2021

포탈 크롤링에서 중복 글을 막는 방법이 좋을 것 같네요. 이렇게 수정해볼게요. 감사합니다!
그런데 이미 존재하는 포탈 공지글에서 제목 + 내용이 같은 글들을 정리하는건 어렵지 않을까요? 중복된 글 중 하나를 빼고 다 지울때, 사용자들이 지워진 글을 읽은 기록 + 담은 기록 + 좋아요/싫어요 누른 기록 등을 한 게시글로 합치는게 가능할까요..?

@victory-jooyon
Copy link
Member

victory-jooyon commented Jul 19, 2021

그런데 이미 존재하는 포탈 공지글에서 제목 + 내용이 같은 글들을 정리하는건 어렵지 않을까요? 중복된 글 중 하나를 빼고 다 지울때, 사용자들이 지워진 글을 읽은 기록 + 담은 기록 + 좋아요/싫어요 누른 기록 등을 한 게시글로 합치는게 가능할까요..?

이전 글 정리같은 경우는 django management command 로 직접 코드 짜보셔도 좋을 것 같구요 (이 위치에 추가하시면 되요)

  • 제목 + 내용 똑같으면 가장 id가 낮은것 하나를 지정: target
  • readlog의 article_id 나 vote 의 parent_article_id 등을 target 으로 변경 (물론 중복 불가 등의 조건은 다 따져야함) ...

@jessyoon14
Copy link
Contributor Author

수정 사항이 많으니, 이 PR은 닫고, 말씀해주신 내용들은 두 개로 나눠서 작업하겠습니다.

DB에서 중복되는 포탈 게시물 정리: [#224 ]
포탈 크롤링 시 같은 글이 여러번 크롤링 되는 것 방지: [#225 ]

@jessyoon14 jessyoon14 deleted the fix/issue-212/side-articles-infinite-loop branch November 4, 2021 17:05
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.

4 participants