Add Discussions GraphQL query #173

Goder-0 · 2025-06-09T03:46:07Z

Related Issue

close Add Discussions GraphQL query #171

PR Description

GitHub로부터 Discussion 데이터를 가져와 저장하는 기능을 구현했습니다.
기존 코드의 구조를 참고하되, 역할에 따라 파일을 분리하여 가독성과 유지보수성을 높였습니다.
현재 GraphQL 쿼리는 Discussion의 모든 필드를 포함하고 있지 않습니다.

파일 역할

database/discussion.rs : DB에 저장될 스키마 구조체 정의
graphql/discussion.rs : GitHub Dashboard에서 제공하는 GraphQL API 응답 구조체 정의

codecov · 2025-06-09T03:47:17Z

Codecov Report

Attention: Patch coverage is 16.46341% with 137 lines in your changes missing coverage. Please review.

Project coverage is 26.04%. Comparing base (804552b) to head (4f263d5).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/database/discussion.rs	7.75%	107 Missing ⚠️
src/github.rs	0.00%	28 Missing ⚠️
src/graphql/discussion.rs	87.50%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #173      +/-   ##
==========================================
- Coverage   29.43%   26.04%   -3.39%     
==========================================
  Files          11       13       +2     
  Lines         530      691     +161     
==========================================
+ Hits          156      180      +24     
- Misses        374      511     +137

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

danbi2990

Could you add a changelog entry, as it introduces a new GraphQL API?

This PR includes the implementation of the discussion GraphQL API, which was not originally planned in #171. However, in my opinion, it's reasonable to include it here, as the PR primarily adds new code rather than modifying existing logic, and the changes are relatively straightforward.

That said, I'd like to ask that tasks be kept small and scoped according to their corresponding issue.

danbi2990 · 2025-06-10T01:59:14Z

src/database/discussion.rs

+    }
+}
+
+impl fmt::Display for ReactionContent {


Could you consider replacing this impl with scalar!(ReactionContent);? This change eliminates the need for exhaustive conversions.

~~It seems difficult because ReactionContent is an enum~~
Done!

danbi2990 · 2025-06-10T04:21:17Z

src/database/discussion.rs

+
+impl From<DiscussionsRepositoryDiscussionsNodesReactions> for Reactions {
+    fn from(reactions: DiscussionsRepositoryDiscussionsNodesReactions) -> Self {
+        let nodes = if let Some(reactions) = reactions.nodes {


How about using unwrap_or_default instead of if let Some(...) {...} else { vec![] }? The behavior appears to be identical.

Goder-0 · 2025-06-12T08:33:47Z

Could you add a changelog entry, as it introduces a new GraphQL API?

I Updated the CHANGELOG.md

danbi2990 · 2025-06-13T01:12:41Z

src/database/discussion.rs

+// impl fmt::Display for ReactionContent {
+//     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+//         let s = match self {
+//             ReactionContent::CONFUSED => "CONFUSED",
+//             ReactionContent::EYES => "EYES",
+//             ReactionContent::HEART => "HEART",
+//             ReactionContent::HOORAY => "HOORAY",
+//             ReactionContent::LAUGH => "LAUGH",
+//             ReactionContent::ROCKET => "ROCKET",
+//             ReactionContent::THUMBS_DOWN => "THUMBS_DOWN",
+//             ReactionContent::THUMBS_UP => "THUMBS_UP",
+//             ReactionContent::Other(s) => s,
+//         };
+//         write!(f, "{s}")
+//     }
+// }


Could you remove this commented-out code?

Removed it. I forgot to remove the comment, sorry about that.

danbi2990 · 2025-06-13T01:16:51Z

src/database/discussion.rs

+        }
+    }
+}
+scalar!(ReactionContent);


~~In my opinion, this declaration should be placed in src/github.rs file, as the Enum is defined there via a macro. Could you share your thoughts on this?~~

On second thought, I'd argue that the declaration should be placed in src/graphql/discussion.rs, as the impls generated by the macro are used for the GraphQL API.

danbi2990 · 2025-06-13T01:22:32Z

src/github.rs

+    name: &str,
+    token: &str,
+) -> Result<Vec<Vec<DiscussionDbSchema>>> {
+    use crate::database::discussion::DiscussionDbSchema;


This import seems unnecessary.

Removed it.

danbi2990 · 2025-06-13T01:36:33Z

CHANGELOG.md

+- Added support for fetching discussions from the GitHub GraphQL API and storing
+  them in the local database.


In my opinion, this entry seems unnecessary, as users don't need to be concerned with the fetching and storing functionality.

Removed it.

danbi2990 · 2025-06-13T01:50:34Z

CHANGELOG.md

  environment variable.
+- Added support for fetching discussions from the GitHub GraphQL API and storing
+  them in the local database.
+- Exposed a new discussions field in the server’s GraphQL API to query the


How about writing 'discussions query' instead of 'discussions field'? It might be more straightforward for users, as 'field' typically refers to the members within a query or mutation.

Also, how about wrapping discussions in backticks as discussions? That would make it clearer that it's a programmatic term.

danbi2990 · 2025-06-16T01:17:18Z

src/database/discussion.rs

+        let author = match &discussion.author {
+            Some(DiscussionsRepositoryDiscussionsNodesAuthor::User(author)) => author.login.clone(),


The clone() call can be removed by using discussion.query instead of &discussion.author.

It's not done yet.

danbi2990 · 2025-06-16T01:19:24Z

src/database/discussion.rs

+impl From<DiscussionsRepositoryDiscussionsNodesCategory> for Category {
+    fn from(category: DiscussionsRepositoryDiscussionsNodesCategory) -> Self {
+        Self {
+            name: (category.name),


Could you remove unnecessary parentheses?

danbi2990 · 2025-06-16T01:21:35Z

src/database/discussion.rs

+        let author = match &answer.author {
+            Some(DiscussionsRepositoryDiscussionsNodesAnswerAuthor::User(author)) => {
+                author.login.clone()


The clone() call can be removed by using answer.author instead of &answer.author.

danbi2990 · 2025-06-16T01:37:20Z

src/database/discussion.rs

+        let author = match &comment.author {
+            Some(DiscussionsRepositoryDiscussionsNodesCommentsNodesAuthor::User(author)) => {
+                author.login.clone()


The clone() call can be removed by using comment.author instead of &comment.author.

danbi2990 · 2025-06-16T01:39:17Z

src/database/discussion.rs

+                let author = match &reply.author {
+                    Some(DiscussionsRepositoryDiscussionsNodesAnswerRepliesNodesAuthor::User(
+                        author,
+                    )) => author.login.clone(),


The clone() call can be removed by using reply.author instead of &reply.author.

danbi2990 · 2025-06-16T01:40:46Z

src/database/discussion.rs

+                let author = match &reply.author {
+                    Some(
+                        DiscussionsRepositoryDiscussionsNodesCommentsNodesRepliesNodesAuthor::User(
+                            author,
+                        ),
+                    ) => author.login.clone(),


The clone() call can be removed by using reply.author instead of &reply.author.

sophie-cluml · 2025-06-16T09:22:56Z

src/github/discussions.graphql

+          createdAt
+          updatedAt
+          url
+          replies(last: 10) {


reply가 10개 보다 많은 경우는 문제가 될 것 같습니다. 모든것을 다 가져오는 것이 가장 완전한 형태일 것 같은데, pageInfo 같은 것을 활용해서 가져올 방법은 없을까요? 추가적인 GraphQL query가 발생할 수 있다고 생각합니다. reply 말고도 last 갯수를 특정 갯수로 제한한 모든 것들이 유사하게 핸들링 되어야 할 것 같습니다. 다만, 이 부분을 TODO로 두고, 별개 enhancement 이슈로 진행할 수도 있을 것 같습니다.

https://docs.github.com/en/graphql/overview/rate-limits-and-node-limits-for-the-graphql-api

위와 같은 제한사항이 존재합니다.
제 생각에는 100개로 조정하게되면 괜찮을것 같다고 생각이 드는데, 혹시 어떻게 생각하시나요?

100 개로 조정한다고 하더라도 마찬가지 이슈가 있다고 생각합니다. 값을 특정하는 것 자체에서 이슈가 있다고 생각이 됩니다.

다른 PR도 유사한 것이 있어서 참고차 코멘트 논의되고 있는 사항 공유드립니다. https://github.com/aicers/github-dashboard-server/pull/175/files#r2151559559

#175 (comment) 에서 논의가 정리된 것으로 보이는데요, 이에 따라 TODO 주석을 추가해두면 괜찮을지 확인 부탁드립니다.

넵 동일하게 처리해주세요.

여기 TODO 주석이 추가 안 된 것 같습니다.

추가했습니다

danbi2990 · 2025-06-18T06:09:31Z

src/database/discussion.rs

+        let author = match &discussion.author {
+            Some(DiscussionsRepositoryDiscussionsNodesAuthor::User(author)) => author.login.clone(),


It's not done yet.

danbi2990 · 2025-06-18T06:56:04Z

src/graphql/discussion.rs

+pub struct Discussion {
+    owner: String,
+    repo: String,
+    number: i64,


This field should be i32 since GraphQL Int maps to i32. Reference
Could you update the type accordingly and add the necessary conversion?

github-dashboard-server/src/database.rs

Line 167 in 8b38549

pub(crate) fn parse_key(key: &[u8]) -> Result<(String, String, i64)> {

github-dashboard-server/src/graphql/discussion.rs

Lines 61 to 69 in 8b38549

impl TryFromKeyValue for Discussion {

fn try_from_key_value(key: &[u8], value: &[u8]) -> anyhow::Result<Self> {

let (owner, repo, number) = database::parse_key(key)

.with_context(|| format!("invalid key in database: {key:02x?}"))?;

let discussion_schema = bincode::deserialize::<DiscussionDbSchema>(value)?;

let discussion = Discussion::new(owner, repo, number, discussion_schema);

Ok(discussion)

}

}

I used i64 because of this code. In that case, should we also update the return type of the parse_key method in database.rs accordingly?

Yes, that's correct.

Goder-0 · 2025-06-20T00:44:12Z

#173 (comment) Done. I apologize, I accidentally missed that.

danbi2990 · 2025-06-24T05:24:30Z

src/database/discussion.rs

+
+#[derive(Debug, Serialize, Deserialize)]
+pub struct DiscussionDbSchema {
+    pub(crate) number: i64,


이 파일에 있는 모든 i64를 i32로 바꿔야할 것 같습니다.
graphql-client 패키지의 매크로가 깃헙 쿼리의 Int 필드를 i64로 변환하는데 이는 잘못된 변환입니다.
저희가 DB에 저장하는 단계에서 오류를 수정하고 저장하는 것이 바람직해 보입니다.

이해했습니다. 수정하겠습니다.

변환작업이 필요하며, 변환시에 범위 체크가 불필요하다고 생각되는데 unwrap을 이용해도 무관할까요?

number: i32::try_from(discussion.number).unwrap(),

해당 코드를 감싸고 있는 impl From 구현을 impl TryFrom 으로 변경하고 unwrap 대신 ?를 쓰는게 바람직해 보입니다. 필요한 경우 호출부에서 예외 처리롤 해주고요.

우선은 호출부에서 변환이 불가능한 경우, 제외하고 처리하도록 구현했습니다. 별도 처리 필요하다면 수정하겠습니다.

sophie-cluml · 2025-07-03T10:06:41Z

Could you rebase please?

Goder-0 · 2025-07-04T06:02:59Z

Could you rebase please?

Rebased

sophie-cluml · 2025-07-04T14:29:03Z

src/database.rs

 }

-pub(crate) fn parse_key(key: &[u8]) -> Result<(String, String, i64)> {
+pub(crate) fn parse_key(key: &[u8]) -> Result<(String, String, i32)> {


이것을 i32로 변경한다면, Issue, Pull Request 등의 db schema 상의 number 항목도 i32 로 변경되는 것이 일관적일 것 같습니다.

수정했습니다.

sophie-cluml · 2025-07-04T15:00:06Z

src/github.rs

+                    if !repository.discussions.page_info.has_next_page {
+                        total_discussions.push(discussions);
+                        break;


has_next_page가 없는 경우에만 DB에 insert하는 로직으로 작성된 것 같습니다. has_next_page 가 있어도 데이터가 insert 되도록 보장해주어야 할 것 같습니다.

해당 로직은 기존에 존재하던 구조를 참고하여 작성된 것입니다.
다음페이지가 존재한다면 loop를 돌며 end_cur을 갱신해가며 페이지네이션을 수행하고,
최종적으로 마지막 페이지에 도달한 경우의 응답을 total_discussions에 추가하는 방식입니다.

discussions 가 loop 돌 때마다 초기화되지 않나요?

맞습니다. 그러나, GraphQL 요청의 Variables에 after: end_cur 값을 설정하여 커서를 이동시키고 있으며, 마지막 페이지인 경우에만 데이터를 추가하고 있습니다.
즉, 마지막 페이지에서 보낸 요청은 처음부터 끝까지의 모든 discussion을 포함하게 됩니다.

마지막 페이지에서 보낸 요청은 처음부터 끝까지의 모든 discussion을 포함하게 됩니다.

이 말이 잘 이해가 안 됩니다. end_cur 이후 GITHUB_FETCH_SIZE 만큼을 요청하게 되는데, 어떻게 처음부터 끝까지의 모든 discussion을 포함하게 되게 되나요?

제가 잘못 이해했었습니다. 수정하겠습니다!

수정했습니다.

sophie-cluml · 2025-07-04T15:05:03Z

src/graphql/issue.rs

+            number,
+            title,
+            author,


이왕 정렬하기로 마음먹었다면 무순서보다는 field 선언 순서와 정렬을 맞춰주시면 좋을 것 같습니다.

수정했습니다.

sophie-cluml · 2025-07-04T15:05:58Z

src/graphql/discussion.rs

+    database::{self, Database, DiscussionDbSchema, TryFromKeyValue},
+    github::discussions::ReactionContent,
+};
+scalar!(ReactionContent);


다른 파일들을 참고하여 적절히 blank line이 들어가면 좋겠습니다.

수정했습니다.

sophie-cluml · 2025-07-15T02:47:05Z

src/github.rs

+) -> Result<Vec<Vec<DiscussionDbSchema>>> {
+    let mut total_discussions = Vec::new();
+    let mut end_cur: Option<String> = None;
+    let mut discussions = Vec::new();


혹시 total_discussions 를 제거하고 discussions 만 활용해도 이 send_github_discussion_query가 현재 역할을 수행해내는데 문제없는지 봐주실 수 있나요? mut vec 으로 선언하는 경우 vec 은 대개 1개로 충분하기 때문에 확인 요청드립니다.

만약 위를 vec 1개로 수정하게 되면

github-dashboard-server/src/github.rs

Lines 234 to 252 in d0007cb

loop {

re_itv.tick().await;

match send_github_discussion_query(&repoinfo.owner, &repoinfo.name, &token).await {

Ok(resps) => {

for resp in resps {

if let Err(error) =

db.insert_discussions(resp, &repoinfo.owner, &repoinfo.name)

{

error!("Problem while insert Sled Database. {}", error);

}

}

break;

}

Err(error) => {

error!("Problem while sending github discussion query. Query retransmission is done after 5 minutes. {}", error);

}

}

itv.reset();

}

위의 로직에서 for문을 제거해야할 것 같은데요,
해당 for문이 의도된바가 아니라면 그렇게 해도 될까요?

total_discussions.push(discussions); 이 코드 제거해도 현재 코드와 동일한 목적 달성이 가능하지 않는가가 저의 질문입니다. L234-252 는 전혀 관계가 없어보입니다.

수정했습니다. 말씀해주신 내용을 제가 제대로 이해한 것이 맞을지 확인 부탁드립니다.

혹시 #173 (comment) 에서 for loop 를 제거하신다는 말씀이 send_github_discussion_query의 리턴타입을 Result<Vec<Vec<DiscussionDbSchema>>> 에서 Result<Vec<DiscussionDbSchema>> 로 변경하고자 하신다는 뜻이었던 것인가요? 그렇다면 표현이 서로 달랐을 뿐 서로 같은 방향으로 변경하자는 이야기를 하고 있었던 것 같습니다.

네 그렇게 변경하겠습니다!

sophie-cluml · 2025-07-15T11:52:27Z

src/github.rs

+        }
+        bail!("Failed to parse response data");
+    }
+    Ok(vec![discussions])


여기를 Ok(discussions) 로 변경하면서 리턴 타입의 2겹 vec을 1겹 vec으로 축소하면 좋을 것 같습니다.

수정했습니다!

Close #171

Goder-0 linked an issue Jun 9, 2025 that may be closed by this pull request

Add Discussions GraphQL query #171

Closed

danbi2990 self-requested a review June 9, 2025 08:12

danbi2990 reviewed Jun 10, 2025

View reviewed changes

Goder-0 force-pushed the kai/discussions-graphql-query branch 3 times, most recently from 2bc59ca to b3dda06 Compare June 12, 2025 08:31

Goder-0 force-pushed the kai/discussions-graphql-query branch 2 times, most recently from 84c60b8 to db2f1ad Compare June 12, 2025 08:41

danbi2990 reviewed Jun 13, 2025

View reviewed changes

Goder-0 force-pushed the kai/discussions-graphql-query branch from db2f1ad to e584b65 Compare June 13, 2025 07:02

danbi2990 reviewed Jun 16, 2025

View reviewed changes

Goder-0 force-pushed the kai/discussions-graphql-query branch from e584b65 to 8b38549 Compare June 16, 2025 05:44

sophie-cluml reviewed Jun 16, 2025

View reviewed changes

sophie-cluml mentioned this pull request Jun 17, 2025

Add issueStat query and openIssueCount field #175

Merged

danbi2990 reviewed Jun 18, 2025

View reviewed changes

Goder-0 force-pushed the kai/discussions-graphql-query branch from 8b38549 to d395a46 Compare June 20, 2025 00:43

Goder-0 force-pushed the kai/discussions-graphql-query branch 2 times, most recently from 0e48b32 to 4cc3e81 Compare June 20, 2025 01:41

danbi2990 reviewed Jun 24, 2025

View reviewed changes

Goder-0 force-pushed the kai/discussions-graphql-query branch 3 times, most recently from 5881a70 to 1df790c Compare June 26, 2025 01:47

danbi2990 approved these changes Jun 26, 2025

View reviewed changes

danbi2990 requested a review from sophie-cluml June 26, 2025 02:13

kiwon94 mentioned this pull request Jul 3, 2025

Add new fields to PullRequests query #184

Merged

Goder-0 force-pushed the kai/discussions-graphql-query branch from 1df790c to 73f3e65 Compare July 4, 2025 06:02

sophie-cluml reviewed Jul 4, 2025

View reviewed changes

Goder-0 force-pushed the kai/discussions-graphql-query branch 4 times, most recently from b0f5424 to d0007cb Compare July 15, 2025 01:36

sophie-cluml reviewed Jul 15, 2025

View reviewed changes

Goder-0 force-pushed the kai/discussions-graphql-query branch from d0007cb to 1572985 Compare July 15, 2025 11:43

sophie-cluml reviewed Jul 15, 2025

View reviewed changes

Add Discussions GraphQL query

4f263d5

Close #171

Goder-0 force-pushed the kai/discussions-graphql-query branch from 1572985 to 4f263d5 Compare July 15, 2025 11:57

sophie-cluml approved these changes Jul 15, 2025

View reviewed changes

danbi2990 merged commit 94c437f into main Jul 15, 2025
10 of 12 checks passed

danbi2990 deleted the kai/discussions-graphql-query branch July 15, 2025 23:08

Goder-0 mentioned this pull request Jul 25, 2025

send_github 메서드 반환 타입 통일 #195

Closed

		- Added support for fetching discussions from the GitHub GraphQL API and storing
		them in the local database.

		let author = match &discussion.author {
		Some(DiscussionsRepositoryDiscussionsNodesAuthor::User(author)) => author.login.clone(),

	impl TryFromKeyValue for Discussion {
	fn try_from_key_value(key: &[u8], value: &[u8]) -> anyhow::Result<Self> {
	let (owner, repo, number) = database::parse_key(key)
	.with_context(\|\| format!("invalid key in database: {key:02x?}"))?;
	let discussion_schema = bincode::deserialize::<DiscussionDbSchema>(value)?;
	let discussion = Discussion::new(owner, repo, number, discussion_schema);
	Ok(discussion)
	}
	}

	loop {
	re_itv.tick().await;
	match send_github_discussion_query(&repoinfo.owner, &repoinfo.name, &token).await {
	Ok(resps) => {
	for resp in resps {
	if let Err(error) =
	db.insert_discussions(resp, &repoinfo.owner, &repoinfo.name)
	{
	error!("Problem while insert Sled Database. {}", error);
	}
	}
	break;
	}
	Err(error) => {
	error!("Problem while sending github discussion query. Query retransmission is done after 5 minutes. {}", error);
	}
	}
	itv.reset();
	}

Add Discussions GraphQL query #173

Add Discussions GraphQL query #173

Uh oh!

Conversation

Goder-0 commented Jun 9, 2025

Related Issue

PR Description

파일 역할

Uh oh!

codecov bot commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

danbi2990 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Goder-0 Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Goder-0 commented Jun 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbi2990 Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sophie-cluml Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 9, 2025 •

edited

Loading

Goder-0 Jun 12, 2025 •

edited

Loading

danbi2990 Jun 13, 2025 •

edited

Loading

sophie-cluml Jun 16, 2025 •

edited

Loading