Add new fields to `PullRequests` query #184

jadenow · 2025-06-19T10:10:25Z

Closes #170

Added new fields (specified in Add GraphQL fields to PullRequests query #170) to the PullRequests GraphQL query.
Extended GitHubPullRequests to store the additional data.
Implemented support for nested types like labels, comments, reviews, and commits.
Derived Serialize/Deserialize for new types to enable database storage.

codecov · 2025-06-20T02:34:35Z

Codecov Report

❌ Patch coverage is 25.12077% with 155 lines in your changes missing coverage. Please review.
✅ Project coverage is 28.22%. Comparing base (2f62f55) to head (73644d8).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/outbound.rs	0.00%	126 Missing ⚠️
src/api/pull_request.rs	63.75%	29 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #184      +/-   ##
==========================================
- Coverage   29.20%   28.22%   -0.99%     
==========================================
  Files          14       14              
  Lines         767      946     +179     
==========================================
+ Hits          224      267      +43     
- Misses        543      679     +136

☔ 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 · 2025-06-23T01:00:22Z

@kiwon94 Could you review this PR?

src/github.rs

kiwon94 · 2025-06-25T10:40:48Z

Could you update the return type from i64 to i32 in database::parse_key?

src/github.rs

src/graphql/pull_request.rs

src/github.rs

kiwon94 · 2025-07-02T06:48:55Z

We need to refactor our code using Jiff according to this comment.

I think you can not only refer to the suggested changes in the comment, but also issue-related changes, while implementing the refactoring.

Cargo.toml

src/github.rs

src/graphql/pull_request.rs

src/github.rs

src/github/pull_requests.graphql

kiwon94 · 2025-07-03T08:09:24Z

I suggest updating the test code in pull_request.rs to match the approach used in issue_stat.rs for consistency. I'd appreciate your thoughts on this. @danbi2990

danbi2990 · 2025-07-03T08:12:58Z

I suggest updating the test code in pull_request.rs to match the approach used in issue_stat.rs for consistency. I'd appreciate your thoughts on this. @danbi2990

I believe this can be addressed in a separate issue if there are no code conflicts.

kiwon94 · 2025-07-08T05:40:35Z

Cargo.toml

 [dependencies]
 anyhow = "1"
-async-graphql = "7"
+async-graphql = { version = "7" }


Can you revert to the original code?

Cargo.toml 및 Cargo.lock 파일 변화가 없도록 다시 체크해주세요

src/github.rs

src/graphql.rs

src/graphql/pull_request.rs

sophie-cluml · 2025-07-24T01:37:25Z

@danbi2990 리뷰 가능하실까요?

danbi2990 · 2025-07-24T01:38:31Z

@danbi2990 리뷰 가능하실까요?

넵

kiwon94 · 2025-07-24T02:30:00Z

src/graphql/issue.rs

                    .into_iter()
                    .map(|comment| Comment {
-                        id: comment.id,
+                        id: comment.id.unwrap_or_default(),


이렇게 변경하신 이유에 대해 여쭙고 싶습니다.

제가 이해하기로는 issue comment와 PR comment의 경우 GraphQL 스키마 정의가 서로 다르기 때문에, 하나의 구조체로 통합하여 처리하는 과정에서 추가적인 코드 수정이 발생한 것으로 보입니다.

이러한 경우 PR comment의 schema 정보를 issue와 동일하게 맞추거나, 아니면 별도의 구조체를 선언하여 명확히 구분하는 방식이 더 적합하다고 생각하는데, 이에 대해 어떻게 생각하시는지 의견을 듣고 싶습니다. (@danbi2990 께서도 의견을 주시면 감사합니다.)

또한 이 변경사항은 현재 진행 중인 이슈의 범위를 벗어나는 수정 사항으로 보이기 때문에, 원래대로 되돌리는 것이 더 적절하다고 생각합니다.

참고로, #169 에 따른 issue에서 수집 중인 comment 정보는 다음과 같습니다.

comments(last: 100) { totalCount nodes { author { __typename ... on User { login } } body createdAt id repository { name } updatedAt url } }

#170 에 따른 PR에서 수집 중인 comment 정보는 아래와 같습니다.

comments(last: 100) { totalCount nodes { body createdAt updatedAt author { __typename ... on User { login } } } }

넵 키언 말씀대로 Issue와 PR의 구조체를 독립적으로 선언하는 게 관리하기 쉬워 보입니다.
현재 github.rs에 Issue와 PR에 대응하는 구조체가 섞여있어서 추후 분리해야될 것 같네요.

kiwon94 · 2025-07-24T02:36:06Z

src/github.rs

-
-                        prs.push(GitHubPullRequests {
+                        prs.push(GitHubPullRequestNode {
+                            id: pr.id.clone(),


이 변경사항 및 비슷한 사항과 관련하여 의견을 드리고자 합니다.

GraphQL 응답을 단순히 DB에 저장하는 용도로만 사용하기 때문에, 가능하면 clone()의 사용을 피하고 코드를 작성하는 것이 더 효율적일 것 같습니다. 이 부분에 대한 수정 검토를 부탁드립니다.

감사합니다.

말씀해주신 대로 Issue와 PR의 Comment 구조를 명확히 분리하는 방향이 더 맞는 것 같습니다.
해당 변경은 되돌렸습니다.

src/graphql.rs

kiwon94 · 2025-07-24T02:51:44Z

src/graphql/pull_request.rs

 pub(crate) struct PullRequest {
-    pub(crate) owner: String,
-    pub(crate) repo: String,
+    pub(crate) id: ID,


ID 타입 사용 여부에 관해 논의가 필요해 보입니다. Github Docs - Comment에 따르면 Comment의 id 필드는 ID scalar를 사용합니다. Github Docs - Scalar 에 따르면 ID 타입은 String으로 간주한다고 명시되어 있습니다.

제 생각엔 현재 코드 베이스 상에선 issue와 관련해선 id 정보를 String 타입으로 일관되게 저장하고 있기 때문에, 통일성을 유지하기 위해 해당 타입도 String 타입으로 변경하는 방안을 고려하면 좋겠습니다.

이에 대한 의견이 궁금합니다. @jadenow @danbi2990

제 생각엔 일단은 그냥 두는게 어떨까 싶습니다.

이 PR은 Issue, PullRequest, Discussion 버전이 동시에 진행되었다는 특이사항이 있습니다. Issue와 Discussion이 먼저 머지되었기는 하지만 거기에 맞춰서 이 PR을 수정하다보니 작업이 다소 늘어지는 경향이 있어 보입니다.

우선 '정상적으로 동작하는 코드'를 먼저 머지하고 리팩토링 이슈를 추가로 작업하는게 어떨까 싶습니다. String vs. ID 에 대한 판단도 이 때 하면 될 것 같고요.

'정상적으로 동작하는 코드'의 기준은 아래와 같습니다.

기능적으로 PullRequest 의 새로운 필드들이 잘 수집되는 상태

기존의 기능들이 정상적으로 동작하는 상태

Issue, Discussion 코드와 구조적, 방법론적으로 다른 부분은 허용합니다.

이 기준에 따라 기존 코드의 동작을 해치는 코드, 일반적으로 개선되어야 할 코드는 수정 대상입니다.

id값을 String으로 되돌렸습니다.

src/graphql/pull_request.rs

Cargo.lock

danbi2990 · 2025-07-25T02:54:48Z

src/github.rs

 #[derive(Debug, Deserialize, Serialize)]
 pub(crate) struct GitHubComment {
-    pub(crate) id: String,
+    pub(crate) id: Option<String>,


GitHubComment 구조체를 이슈와 PR에서 재사용하려다보니 String을 Option<String>으로 바꾼 것으로 보입니다.

GitHubComment 구조체를 재사용하지 않고 이슈와 PR 각각에 대한 코멘트 구조체를 따로 선언하는 것이 좋을 것 같습니다. 예를 들면 IssueGitHubComment, PullRequestGitHubComment 등으로 따로 선언하는 방식입니다.

이유는, GraphQL의 셀렉션셋을 서로 다른 쿼리가 공유했을 때 유지 보수가 어렵기 때문입니다:

위의 사례처럼 PR의 기능을 작업하는데 Issue가 영향 받는 경우가 발생합니다.

이슈와 PR의 관점에서 필요한 코멘트 필드가 다를 수 있습니다.

danbi2990 · 2025-07-25T05:25:38Z

src/graphql/pull_request.rs

+            #[allow(clippy::cast_possible_truncation)]
+            number: gh.number as i32,


number와 gh.number의 타입이 같아서 변환이 필요 없어 보입니다.

src/graphql/issue.rs

src/github.rs

src/graphql/pull_request.rs

danbi2990 · 2025-08-12T04:24:17Z

src/api/pull_request.rs

+
+        Ok(PullRequest {
+            id: gh.id,
+            #[allow(clippy::cast_possible_truncation)]


How about removing this allow?

danbi2990 · 2025-08-12T04:34:58Z

src/api/pull_request.rs

-            owner,
-            repo,


Could you revert these fields? These are necessary, as mentioned in this command #184 (comment).

danbi2990 · 2025-08-12T04:39:48Z

src/api/pull_request.rs

+                body: r.body,
+                url: r.url,
+                created_at: DateTimeUtc(r.created_at),
+                published_at: DateTimeUtc(r.published_at.unwrap_or_default()),


How about declaring Review.published_at as Option<DateTimeUtc> to avoid using unwrap_or_default?

danbi2990 · 2025-08-12T04:52:11Z

src/api/pull_request.rs

 impl fmt::Display for PullRequest {
    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
-        write!(f, "{}/{}#{}", self.owner, self.repo, self.number)
+        write!(f, "{}", self.number)


fmt 함수는 기존 코드로 되돌려야할 것으로 보입니다.

fmt 함수는 connect_cursor 함수에서 Connection 의 cursor를 인코딩할 때 사용됩니다.
cursor는 pagination에 사용되므로 유니크해야 합니다. 변경된 코드에서는 서로 다른 저장소의 PR이 저장되면 페이징이 안 될 것으로 보입니다.

danbi2990 · 2025-08-12T05:01:10Z

src/api/pull_request.rs

+            updated_at: DateTimeUtc(gh.updated_at),
+            closed_at: gh.closed_at.map(DateTimeUtc),
+            merged_at: gh.merged_at.map(DateTimeUtc),
+            author: Some(gh.author),


How about declaring PullRequest.author as String instead of Option<string> to avoid using Some?

danbi2990 · 2025-08-12T06:11:03Z

src/api/pull_request.rs

+            repository_owner: gh.repository.owner,
+            repository_name: gh.repository.name,


How about removing these fields? They are duplicates of owner and repo.

danbi2990 · 2025-08-12T06:12:34Z

src/api/pull_request.rs

+    #[allow(clippy::too_many_lines)]
    fn try_from_key_value(key: &[u8], value: &[u8]) -> anyhow::Result<Self> {
-        let (owner, repo, number) = database::parse_key(key)
+        let _number = database::parse_key(key)


How about removing this line? It seems unnecessary.

danbi2990 · 2025-08-12T06:13:08Z

src/api/pull_request.rs

-        };
-        Ok(pr)
+        let gh: GitHubPullRequestNode = bincode::deserialize(value)
+            .with_context(|| format!("Deserialization failed for key: {key:?}"))?;


The error message should be "Deserialization failed for value: {value:?}".

danbi2990 · 2025-08-12T06:14:21Z

src/api/pull_request.rs


 impl TryFromKeyValue for PullRequest {
+    #[allow(clippy::too_many_lines)]
    fn try_from_key_value(key: &[u8], value: &[u8]) -> anyhow::Result<Self> {


key might need to be renamed to _key after changing lines 89 and 92.

danbi2990 · 2025-08-12T06:50:39Z

src/api/pull_request.rs

+            owner: gh.repository.owner.clone(),
+            repo: gh.repository.name.clone(),


Could you remove clone for these lines?

danbi2990 · 2025-08-12T06:52:39Z

CHANGELOG.md

 - Exposed a new `discussions` query in the server’s GraphQL API to query the
  stored discussion data.
+- Added new fields to the `PullRequests` GraphQL query and corresponding fields to
+  the `graphql::pull_request::PullRequest` struct.


graphql::pull_request::PullRequest should be api::pull_request::PullRequest.

Closes #170 - Added new fields (specified in #170) to the `PullRequests` GraphQL query. - Extended `GitHubPullRequests` to store the additional data. - Implemented support for nested types like labels, comments, reviews, and commits. - Derived `Serialize`/`Deserialize` for new types to enable database storage.

danbi2990 self-requested a review June 20, 2025 00:21

jadenow force-pushed the jade/add-graphql-field branch from a4bc1cc to 502e9fc Compare June 20, 2025 02:31

danbi2990 requested review from kiwon94 and removed request for danbi2990 June 23, 2025 00:58

kiwon94 reviewed Jun 24, 2025

View reviewed changes

jadenow force-pushed the jade/add-graphql-field branch from 502e9fc to d211055 Compare June 25, 2025 08:13

kiwon94 reviewed Jun 26, 2025

View reviewed changes

src/github.rs Outdated Show resolved Hide resolved

src/github.rs Outdated Show resolved Hide resolved

src/graphql/pull_request.rs Outdated Show resolved Hide resolved

jadenow force-pushed the jade/add-graphql-field branch from d211055 to a4402a7 Compare June 26, 2025 09:01

kiwon94 reviewed Jun 26, 2025

View reviewed changes

src/github.rs Outdated Show resolved Hide resolved

src/github.rs Outdated Show resolved Hide resolved

jadenow force-pushed the jade/add-graphql-field branch 2 times, most recently from 16f2b8c to 92c5395 Compare July 2, 2025 06:41

jadenow force-pushed the jade/add-graphql-field branch from 92c5395 to 3e6a679 Compare July 2, 2025 08:32

kiwon94 reviewed Jul 3, 2025

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

src/github.rs Outdated Show resolved Hide resolved

src/graphql/pull_request.rs Outdated Show resolved Hide resolved

kiwon94 reviewed Jul 3, 2025

View reviewed changes

src/github.rs Outdated Show resolved Hide resolved

src/github.rs Outdated Show resolved Hide resolved

src/github.rs Outdated Show resolved Hide resolved

src/github/pull_requests.graphql Show resolved Hide resolved

jadenow force-pushed the jade/add-graphql-field branch from 3e6a679 to 1cf9b22 Compare July 3, 2025 07:05

jadenow force-pushed the jade/add-graphql-field branch 2 times, most recently from 024fa78 to 41179f9 Compare July 7, 2025 11:11

kiwon94 reviewed Jul 8, 2025

View reviewed changes

kiwon94 reviewed Jul 9, 2025

View reviewed changes

src/graphql/pull_request.rs Outdated Show resolved Hide resolved

jadenow force-pushed the jade/add-graphql-field branch 2 times, most recently from f788c02 to fa654cb Compare July 18, 2025 02:37

danbi2990 mentioned this pull request Jul 24, 2025

LLM이 Semantic Parsing, RAG 의 답변을 조합할 수 있는지 확인 #180

Closed

kiwon94 reviewed Jul 24, 2025

View reviewed changes

jadenow force-pushed the jade/add-graphql-field branch from fa654cb to b3a75c2 Compare July 24, 2025 06:47

kiwon94 approved these changes Jul 24, 2025

View reviewed changes

danbi2990 reviewed Jul 25, 2025

View reviewed changes

jadenow force-pushed the jade/add-graphql-field branch from b3a75c2 to 82917ae Compare July 28, 2025 09:07

kiwon94 reviewed Jul 29, 2025

View reviewed changes

src/graphql/issue.rs Outdated Show resolved Hide resolved

src/github.rs Outdated Show resolved Hide resolved

jadenow force-pushed the jade/add-graphql-field branch from 82917ae to 1d941f5 Compare July 29, 2025 06:33

kiwon94 approved these changes Jul 29, 2025

View reviewed changes

sophie-cluml requested a review from kimhanbeom July 30, 2025 01:46

kimhanbeom reviewed Jul 30, 2025

View reviewed changes

src/github.rs Outdated Show resolved Hide resolved

kimhanbeom reviewed Jul 30, 2025

View reviewed changes

src/github.rs Show resolved Hide resolved

kimhanbeom reviewed Jul 30, 2025

View reviewed changes

src/graphql/pull_request.rs Outdated Show resolved Hide resolved

jadenow force-pushed the jade/add-graphql-field branch from 1d941f5 to 9686485 Compare August 12, 2025 04:02

danbi2990 reviewed Aug 12, 2025

View reviewed changes

jadenow force-pushed the jade/add-graphql-field branch from 9686485 to fdc0339 Compare August 12, 2025 05:44

danbi2990 reviewed Aug 12, 2025

View reviewed changes

jadenow force-pushed the jade/add-graphql-field branch 3 times, most recently from 7cc788e to c354b53 Compare August 12, 2025 06:50

danbi2990 reviewed Aug 12, 2025

View reviewed changes

jadenow force-pushed the jade/add-graphql-field branch from c354b53 to c066260 Compare August 12, 2025 07:08

danbi2990 approved these changes Aug 12, 2025

View reviewed changes

danbi2990 requested a review from kimhanbeom August 12, 2025 07:18

jadenow force-pushed the jade/add-graphql-field branch from c066260 to 73644d8 Compare August 13, 2025 05:55

kimhanbeom approved these changes Aug 13, 2025

View reviewed changes

danbi2990 merged commit c601fd1 into main Aug 13, 2025
10 of 12 checks passed

danbi2990 deleted the jade/add-graphql-field branch August 13, 2025 06:03

		#[allow(clippy::cast_possible_truncation)]
		number: gh.number as i32,

		repository_owner: gh.repository.owner,
		repository_name: gh.repository.name,

		owner: gh.repository.owner.clone(),
		repo: gh.repository.name.clone(),

Add new fields to PullRequests query #184

Add new fields to PullRequests query #184

Conversation

jadenow commented Jun 19, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

danbi2990 commented Jun 23, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kiwon94 commented Jun 25, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kiwon94 commented Jul 2, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kiwon94 commented Jul 3, 2025

Uh oh!

danbi2990 commented Jul 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiwon94 Jul 24, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sophie-cluml commented Jul 24, 2025

Uh oh!

danbi2990 commented Jul 24, 2025

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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Add new fields to `PullRequests` query #184

Add new fields to `PullRequests` query #184

codecov bot commented Jun 20, 2025 •

edited

Loading

kiwon94 Jul 24, 2025 •

edited

Loading

danbi2990 Aug 12, 2025 •

edited

Loading