Add conversion for GitHub API response to `GitHubIssue` #192

JonghoKim-jj · 2025-07-23T07:35:55Z

OBJECTIVE: Convert GitHub API Response to our own struct GitHubIssue using From<T> or TryFrom<T>
Define struct GitHubIssueResponse and implement TryFrom<T> to parse GitHub API response.
- From: GitHub API response
- To: struct GitHubIssueResponse defined by us
- If request is wrong, try_from returns Err. If empty response, try_from returns Ok(GitHubIssueResponse).
Implement From<T> to convert types
- From: structs which is generated by async-graphql's codegen based on GitHub GraphQL schema (ex) crate::github::issues::ResponseData, crate::github::issues::IssuesRepositoryIssuesNodesComments, ...
- To: Our own structs (ex) GitHubIssue, GitHubCommentConnection, ...
- Since wrong request or bad response will be handled by TryFrom<T> trait for GitHubIssueResponse, these conversions have no need to handle exceptions
- This implementation enables to split one function into functions usingfrom for conversion, to avoid clippy::too_many_lines
Replace unwrap_or_default to expect for issue number, pr number, total count, etc. (for Option<Vector<T>>, still using unwrap_or_default because it returns empty vector vec![])
Unit tests use data of this repository, which is public

Close #185

codecov · 2025-07-23T07:39:14Z

Codecov Report

❌ Patch coverage is 6.61765% with 127 lines in your changes missing coverage. Please review.
✅ Project coverage is 30.99%. Comparing base (64ead41) to head (7eaca6b).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/database/issue.rs	7.43%	112 Missing ⚠️
src/outbound.rs	0.00%	15 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #192      +/-   ##
==========================================
+ Coverage   30.64%   30.99%   +0.34%     
==========================================
  Files          15       16       +1     
  Lines         979      968      -11     
==========================================
  Hits          300      300              
+ Misses        679      668      -11

☔ 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-08-04T08:29:30Z

src/github.rs

+const GRAPHQL_ISSUE_NUMBER_ASSERTION: &str = r"
+GraphQL field Issue.number is Int! type, thus always exist.
+And it will not exceed 2^32.";
+const GRAPHQL_PULL_REQUEST_NUMBER_ASSERTION: &str = r"
+GraphQL field PullRequest.number is Int! type, thus always exist.
+And it will not exceed 2^32.";
+const GRAPHQL_ISSUE_CONNECTION_TOTAL_COUNT_ASSERTION: &str = r"
+GraphQL field IssueConnection.totalCount is Int! type, thus always exist.
+And it will not exceed 2^32.";


How about removing the assertions and using TryFrom instead, as we've already adopted that approach in discussion.rs?

BTW, the maximum value of i32 is 2^31 - 1, considering negative values and zero.

I implemented:

TryFrom for a conversion

From: GitHub response (GraphQlResponse<issues::ResponseData>)

To: Our own data structureGitHubIssueResponse

I thought the conversion is fallible due to incorrect request, etc.

From for few conversions

From: Structs generated by graphql-client's codegen (ex) IssuesRepositoryIssuesNodesComments

To: Our own data structures. (ex) GitHubCommentConnection

I thought those conversions are infallible, because the content of codegen structs and our own data structures are identical.

The reason I implement From for those conversions is that those conversions are infallible. Rust standard document guides to implement TryFrom for fallible conversions and From for infallible conversions.

Note: This trait must not fail. The From trait is intended for perfect conversions. If the conversion can fail or is not perfect, use TryFrom.

I think replacing From implementation for infallible conversion to TryFrom makes one kind of misunderstanding - that the conversion is fallible while it is actually infallible.
And those assertions are for expect() in infallible conversions, not for exception handling.

If you think the code is too verbose or longer than needed, I will simplify the code.

Thanks for pointing out that max value of i32 is 2^31-1. I should fix it.

By the way, how about changing types of number, total_count from i32 to unsinged integer such as u32?
The numbers are never be negative number.

I think replacing From implementation for infallible conversion to TryFrom makes one kind of misunderstanding - that the conversion is fallible while it is actually infallible.

That said, the documentation also states:

But From cannot be used to convert from u32 to u16, since that cannot succeed in a lossless way.

I believe the concept of fallibility here is from a technical standpoint.

By the way, how about changing types of number, total_count from i32 to unsigned integer such as u32?

These values are used for GraphQL Int type, which is equivalent to i32.

Ok. I'll implement TryFrom and use pattern such as:

let number: i32 = issue.number.try_into()?;

inside try_from(), and remove those string literals for assertions.

danbi2990 · 2025-08-04T08:37:55Z

src/github.rs

+    }
+
+    #[test]
+    fn convert_response_to_issue_() {


How about removing the trailing underscore?

danbi2990 · 2025-08-04T08:44:50Z

src/github.rs

+}
+
+#[cfg(test)]
+mod tests {


Could you verify that your VSCode is configured as described in the Notion page? There are several warnings in the test module.

"rust-analyzer.check.extraArgs": [ "--all-features", "--tests", "--", "-W", "clippy::pedantic" ],

I did not noticed it was set to --features default. Thanks for the check.

danbi2990 · 2025-08-04T09:06:53Z

src/github.rs

+        let graphql_response: GraphQlResponse<issues::ResponseData> =
+            serde_json::from_str(response_str).expect("Valid JSON");


How about removing response_str and implementing Default for GraphQlResponse<issues::ResponseData> instead?

There are several reason for this suggestion:

serde_json::from_str is outside the scope of this test.

response_str takes up too much space in the file.

If the GraphQL fields change, response_str must be exhaustively updated, which is quite burdensome.

I believe we can adopt a similar approach to what's used in issue_stat::test.

I agree that the string response_str in unit test is too long. How about separate it to a file tests/issue_response_from_github.json?

As you pointed out, if GitHub GraphQL API changes, we should update the sample json data. However, I think core API of GitHub GraphQL API will not change that frequently.

I think fetching raw data from GitHub GraphQL API is our core/essential feature. If we can not parse response from GitHub, our dashboard server can not serve to users.

Implementing a trait/method for GraphQlResponse<issues::ResponseData> is impossible because it is foreign type. I suggest (1) not implementing them or (2) implementing issues::ResponseData::new() instead, which gets issue numbers as a parameter.

issues::ResponseData::new() can avoid duplicated number fields, which will be used as composite key (primary key) of database records.

I think creating object with default values might not simplify our test code much in this test case. For example, our default value for IssueState is IssueState::OPEN. For testing resolved issue statistics, we need to create object with default values and modify fields of each object. Also, for another example, we need to modify each closed_by_pull_requests's state to be merged, because we deteremine an issue to be resolved if and only if all of its closed_by_pull_requests is merged.

As you pointed out, if GitHub GraphQL API changes, we should update the sample json data. However, I think core API of GitHub GraphQL API will not change that frequently.

Sorry for the confusion. I wasn't referring to the GitHub GraphQL API, but to our own issue.graphql query. Whenever we add a new field for a statistics, this test case will fail unless it's updated accordingly. If we can find a way to prevent the test from failing when new fields are added, I believe that would be deal.

If we can not parse response from GitHub, our dashboard server can not serve to users.

Parsing the JSON is handled by the serde_json crate, so I think that falls outside our direct responsibility.

I suggest (1) not implementing them or (2) implementing issues::ResponseData::new() instead

I'd like to suggest option '(1) not implementing them', following the approach used in discussion.rs.

Overview of implementations in this PR

The figure below shows overview of implementations in this pull request.

response_str * Response JSON from GitHub GraphQL API, || which may contain issues, paging informations, || graphql_client crate has integration with error messages || serde_json crate. The conversion is done by || serde_json crate, while result type is || graphql_client::Response<Data>, which is of || graphql_client crate. \/ graphql_client::Response<issues::ResponseData> * Data structure defined by graphql_client crate. || || Conversion by using TryFrom. If success, || (1) Issue Data and (2) Paging Information || are obtained. || || || if success |----------------> (IssuesRepositoryIssuesNodes, paging inforamtions) || || || || Conversion by using From. || || || \/ \/ +-- Vec<GitHubIssue> GitHubIssueResponse----+ * Data structure defined by us. +-- Paging Informations (has_next_page, end_cursor)

Explanation on conversion implementation

Let me explain my implementation.
I decided to define a struct GitHubIssueResponse to store issue data (issues) and paging informations (has_next_page, end_cursor) and implement conversion from GitHub response, whose type is GraphQlResponse<issues::ResponseData>>, to GitHubIssueResponse by implementing TryFrom.

use graphql_client::{GraphQLQuery, QueryBody, Response as GraphQlResponse}; ... struct GitHubIssueResponse { issues: Vec<GitHubIssue>, has_next_page: bool, end_cursor: Option<String>, } ... impl TryFrom<GraphQlResponse<issues::ResponseData>> for GitHubIssueResponse { ... } ...

At first, I thought about implementing a function named parse(), such as:

impl GraphQlResponse<issues::ResponseData>> { fn parse(&self) -> (Vec<GitHubIssue>, bool, Option<String>) { ... } }

but it was impossible to define function/method to GraphQlResponse<issues::ResponseData> because it was foreign type. So I change my mind to implement TryFrom. To make the conversion lossless, I decided to define struct GitHubIssueResponse in order to make conversion to work without dropping any information from GitHub response. (Rust standard says that From and TryFrom should be lossless)

If conversion is success, we can get issue data as a IssuesRepositoryIssuesNodes object and paging informations.

I also implemented few conversions using From. They convert struct generated by graphql_client crate's codegen, based on GraphQL schema defined by GitHub. All of them is called if and only if the GitHubIssueResponse::try_from() is success. The reason I chose From is that those conversions are almost 1:1 trivial mapping of fields, so they are infallible and lossless.

impl From<IssuesRepositoryIssuesNodes> for GitHubIssue { ... }

Reason for using JSON in unit tests

The reason I gave JSON as a test input is not to test serde_json crate.
I do not intend to test serde_json, but I'm just calling them in the unit tests.

If I separate JSON to a file named tests/issue_response_from_github.json then the test code looks like:

#[test] fn convert_response_to_issue_() { let response_str = include_str!("../tests/issue_response_from_github.json"); let graphql_response: GraphQlResponse<issues::ResponseData> = serde_json::from_str(response_str).expect("Valid JSON"); let resp = GitHubIssueResponse::try_from(graphql_response) .expect("Correct data, so parsing should success"); assert!(resp.has_next_page); assert_eq!( resp.end_cursor, Some(String::from( "Y3Vyc29yOnYyOpK5MjAyMi0wNy0xMlQxODozMzo0MiswOTowMM5Nl-UC" )) ); // ... More assertions on GitHubIssue objects, // which can be obtained by converting // GraphQlResponse<issues::ResponseData> // to GitHubIssueResponse

I want to note that:

I wanted to test that conversion from GitHub response GraphQlResponse<issues::ResponseData> to GitHubIssueResponse works correctly and GitHubIssueResponse contains GitHubIssues whose fields are correct.

I assume serde_json::from_str() will always success even when the request is wrong or response is error. So I used expect() to pass exception handling.

If I do not use JSON string,

2-3 lines of testing code such as:

let response_str = include_str!("../tests/issue_response_from_github.json"); let graphql_response: GraphQlResponse<issues::ResponseData> = serde_json::from_str(response_str).expect("Valid JSON");

will turn into:

use graphql_client::Response as GraphQlResponse; ... let graphql_response = GraphQlResponse { data: Some(issues::ResponseData { repository: Some(IssuesRepository { issues: IssuesRepositoryIssues { page_info: IssuesRepositoryIssuesPageInfo { has_next_page: false, end_cursor: Some(String::from( "Y3Vyc29yOnYyOpK5MjAyMi0wNy0xMlQxODozMzo0MiswOTowMM5Nl-UC", )), }, nodes: Some(vec![ ... ]), }, }), }), errors: None, extensions: None, };

I think this is more burdensome than just storing actual JSON response from GitHub.

About From vs. TryFrom

I don't see any practical benefit of choosing From over TryFrom in this context. With TryFrom, we avoid the need to define verbose error messages, as internal error messages are automatically propagated automatically by the compiler. It's also a widely adopted pattern in Rust. Could you elaborate on the advantages of your implementation?

About using JSON

I also seriously explored JSON fixtures in #153 (you can find the relevant code in the commits). The reason I moved away from that approach is because, whenever we add a new field for a statistics, this test case will fail unless it's updated accordingly.

In my view, test cases should be additive, not retrospective, meaning it's fine to add a new test case when introducing new functionality, but it's problematic if you have to modify existing test case. In this case, adding a field to a query should be covered by a new test, and shouldn't break previous ones. From a practical standpoint, breaking existing tests would slow down the development pace. Could you share your idea on this?

Thanks for sharing your ideas, experiences, and histories.
I think I understand feedbacks, and I would summarize the feedbacks before modifying the code.

Use TryFrom instead of From

For consistency with codebase such as discussion.rs

Use of TryFrom is general and reasonable

Remove JSON fixture

It is burdensome with API changes - Most API changes (ex issue.graphql edited by us) will be breaking changes to tests

It is too verbose

I also want to share my idea/reasoning.

I prefer From over TryFrom where the conversion never fails

Mainly for ergonomics - It avoids misunderstanding that the conversion can fail while it is actually always success.

It removes need for handling exceptions to simplify code. No need to think about error messages because it will always success.

As @danbi2990 pointed out, I might overthink the benefit.

I prefer adding JSON fixture to test conversion from GitHub response to our own data structure

I think there will be almost no breaking changes to GitHub API and our request to GitHub

I think we need strong test cases before implementing paging logic mentioned in Pagination 구현: GitHub GraphQL API 에 요청 및 응답을 파싱할 때 pagination 이용 #181

JSON is human-readable and we can separate JSON to fixture file, while codegened structs are too verbose to create/construct

Thank you for the summary. In addition, I'm not sure we even need a test case for the conversion, since the conversion is already guaranteed by strong typing.

In contrast, issue_stat.rs includes actual calculation logic (such as filtering and statistics), which justifies having tests. Or if we are implementing a public API, then adding test cases may be required by corporate policy.

If testing code-generated structs becomes too verbose, I believe it's reasonable to skip those tests.

I agree that strong test cases are essential for the pagination logic.

It sounds reasonable.
I think it is good to skip writing test for the conversion.
I will write strong tests for pagination logic in other issue, not in this issue.

danbi2990 · 2025-08-18T05:56:22Z

src/database/issue.rs

+        let author = String::from(
+            issue
+                .author
+                .ok_or(anyhow!("Author of GitHub issue always exist."))?,


How about using context instead of or_or(anyhow!(...))? I believe that would be more idiomatic.

I think the error message is misleading. If the value should always exist, we should use expect. If the goal is to show an error message when it fails, the message should be something like "Author of GitHub issue is missing".

I think author of issue should always exist, but GraphQL schema is defined as:

type Issue implements ... { ... author: Actor ... }

I will use context because GraphQL schema defines author as an optional field.

danbi2990 · 2025-08-18T05:58:19Z

src/outbound.rs

+            .data
+            .expect("Response data should exist, although when it is empty or error.")
+            .repository
+            .ok_or(anyhow!("Wrong repository."))?;


This can be simplified by using context.

I'll replace both expect and ok_or to context to simplify.

danbi2990 · 2025-08-18T06:09:30Z

src/database/issue.rs

+                .author
+                .ok_or(anyhow!("Author of GitHub issue always exist."))?,
+        );
+        let comments = GitHubIssueCommentConnection::try_from(issue.comments)?;


How about using try_into instead of try_from as shown below?

let comments = issue.comments.try_into()?;

This makes the code simpler by allowing the compiler to infer the target type, and it's also more idiomatic.

The same change can be applied to all try_from usages below.

danbi2990 · 2025-08-18T06:16:19Z

src/database/issue.rs

+            author,
+            body: issue.body,
+            state: issue.state,
+            assignees: Vec::<String>::from(issue.assignees),


Similar to the above comments, this can be simplified to assignees: issues.assignees.into().

danbi2990 · 2025-08-18T07:02:04Z

src/outbound.rs

+    fn try_from(value: GraphQlResponse<issues::ResponseData>) -> Result<Self> {
+        let repo = value
+            .data
+            .expect("Response data should exist, although when it is empty or error.")


Is it correct to assume that data should always exist? According to this link, it can be None if an error was raised during the execution.

I suggest using context("error message")? instead of leaving it uncertain.

I will accept your suggestion.

It seems that the link you gave is official GraphQL spec.
I think GitHub does not obey the rule in the link.

The reason I assumed data should always exist is that GitHub always returns data even if wrong owner and repo are given.
The screenshot below is a response from GitHub GraphQL API:

I think although GitHub does not obey the rule, it is better to obey the official GraphQL spec.

danbi2990 · 2025-08-18T07:04:57Z

src/outbound.rs

+        let nodes = repo
+            .issues
+            .nodes
+            .expect("This field will be always returned even if no issue exist");


Could you explain why this is always true? Otherwise, consider using context("error message")?.

I will change message for expect to "If repo exists, the issue field will be always returned.".

This is always true because if repo exists, the issue field is always returned.

The screenshot below is response from GitHub for a request for a repo which has 0 issues (my personal repository):

In my opinion, using expect on the assumption that the external API is always safe seems risky. This approach means our software could panic depending on the external API's behavior.

We don't know the details of the GitHub API source code, their infrastructure, or internal guarantees. The only thing we can clearly rely on is their public API contract (schema.graphql), which specifies IssueConnection.nodes: [Issue]. Since the type is nullable, it can be null at any time.

I believe gathering evidence to justify treating IssueConnection.nodes as always safe would be time-consuming, because the public schema explicitly declares it as nullable.

I will remove all unnecessary assumptions and rely exclusively on schema.graphql in making my judgment.

danbi2990 · 2025-08-18T07:05:59Z

src/outbound.rs

+            .issues
+            .nodes
+            .expect("This field will be always returned even if no issue exist");
+        let issues: Vec<_> = nodes


The Vec<_> type declaration seems redundant.

Yes, it is redundant. I'll remove it.
Thanks for your careful review.

JonghoKim-jj · 2025-08-18T08:27:07Z

@danbi2990 Thanks for your feedbacks. Especially, I recognized that I need to read README of anyhow crate, so I read it. It was great help.

- Define struct `GitHubIssueResponse` and implement `TryFrom<T>` trait to parse/convert GitHub API response - Add few implementations of `From<T>` trait to convert contents of struct `GitHubIssueResponse` to struct `GitHubIssue`

danbi2990 self-requested a review July 23, 2025 07:39

danbi2990 mentioned this pull request Aug 4, 2025

Avoid using non-intuitive names ("github", "graphql") as a module name #201

Closed

danbi2990 reviewed Aug 4, 2025

View reviewed changes

JonghoKim-jj force-pushed the leo/#185-github_issue-converter branch 3 times, most recently from fc7f1f5 to 9fb25cd Compare August 18, 2025 05:52

danbi2990 reviewed Aug 18, 2025

View reviewed changes

JonghoKim-jj force-pushed the leo/#185-github_issue-converter branch 2 times, most recently from 85f9bb9 to 2f9ac53 Compare August 19, 2025 00:53

Add conversion for response to GitHubIssue

7eaca6b

- Define struct `GitHubIssueResponse` and implement `TryFrom<T>` trait to parse/convert GitHub API response - Add few implementations of `From<T>` trait to convert contents of struct `GitHubIssueResponse` to struct `GitHubIssue`

JonghoKim-jj force-pushed the leo/#185-github_issue-converter branch from 2f9ac53 to 7eaca6b Compare August 19, 2025 09:14

danbi2990 approved these changes Aug 20, 2025

View reviewed changes

danbi2990 requested a review from kimhanbeom August 20, 2025 05:57

kimhanbeom approved these changes Aug 20, 2025

View reviewed changes

danbi2990 merged commit 35f7ef6 into main Aug 20, 2025
11 of 12 checks passed

danbi2990 deleted the leo/#185-github_issue-converter branch August 20, 2025 08:00

		let graphql_response: GraphQlResponse<issues::ResponseData> =
		serde_json::from_str(response_str).expect("Valid JSON");

Add conversion for GitHub API response to GitHubIssue #192

Add conversion for GitHub API response to GitHubIssue #192

Conversation

JonghoKim-jj commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

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!

JonghoKim-jj Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Overview of implementations in this PR

Explanation on conversion implementation

Reason for using JSON in unit tests

Uh oh!

Choose a reason for hiding this comment

About From vs. TryFrom

About using JSON

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!

JonghoKim-jj Aug 18, 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

Add conversion for GitHub API response to `GitHubIssue` #192

Add conversion for GitHub API response to `GitHubIssue` #192

JonghoKim-jj commented Jul 23, 2025 •

edited

Loading

codecov bot commented Jul 23, 2025 •

edited

Loading

JonghoKim-jj Aug 5, 2025 •

edited

Loading

About `From` vs. `TryFrom`

JonghoKim-jj Aug 18, 2025 •

edited

Loading