Handle empty author object in pr_commits (#656)

src/api/pulls.rs

@@ -9,7 +9,7 @@ use crate::error::HttpSnafu;
 use crate::models::pulls::ReviewComment;
 use crate::models::CommentId;
 use crate::pulls::specific_pr::pr_reviews::specific_review::SpecificReviewBuilder;
-use crate::pulls::specific_pr::SpecificPullRequestBuilder;
+use crate::pulls::specific_pr::{SpecificPullRequestBuilder, SpecificPullRequestCommitBuilder};
 use crate::{Octocrab, Page};
 
 pub use self::{
@@ -425,14 +425,26 @@ impl<'octo> PullRequestHandler<'octo> {
         SpecificReviewBuilder::new(self, pull_nr, review_id)
     }
 
-    /// /repos/{owner}/{repo}/pulls/{pull_number}/commits
-    pub async fn pr_commits(
-        &self,
-        pull_nr: u64,
-    ) -> crate::Result<crate::Page<crate::models::repos::RepoCommit>> {
-        SpecificPullRequestBuilder::new(self, pull_nr)
-            .commits()
-            .await
+    /// Lists a maximum of 250 commits for a pull request.
+    ///
+    /// To receive a complete commit list for pull requests with more than 250 commits,
+    /// use the [List commits](https://docs.github.com/rest/commits/commits#list-commits) endpoint.
+    ///
+    /// ```no_run
+    /// # async fn run() -> octocrab::Result<()> {
+    /// let commits = octocrab::instance()
+    ///     .pulls("owner", "repo")
+    ///     .pr_commits(21u64)
+    ///     .per_page(100)
+    ///     .page(2u32)
+    ///     .send()
+    ///     .await?;
+    /// # Ok(())
+    /// # }
+    /// ```
+    // /repos/{owner}/{repo}/pulls/{pull_number}/commits
+    pub fn pr_commits(&self, pr_number: u64) -> SpecificPullRequestCommitBuilder<'_, '_> {
+        SpecificPullRequestCommitBuilder::new(self, pr_number)
     }
 
     // /repos/{owner}/{repo}/pulls/{pull_number}/comments/{comment_id}/replies

src/api/pulls/specific_pr.rs

@@ -1,3 +1,4 @@
+pub use self::pr_commit::SpecificPullRequestCommitBuilder;
 use crate::models::repos::RepoCommit;
 use crate::models::CommentId;
 use crate::pulls::specific_pr::pr_comment::SpecificPullRequestCommentBuilder;
@@ -6,6 +7,7 @@ use crate::pulls::PullRequestHandler;
 use crate::Page;
 
 mod pr_comment;
+mod pr_commit;
 pub(crate) mod pr_reviews;
 /// A builder pattern struct for working with a specific pull request data,
 /// e.g. reviews, commits, comments, etc.

src/api/pulls/specific_pr/pr_commit.rs

@@ -0,0 +1,51 @@
+use crate::pulls::PullRequestHandler;
+use serde::Serialize;
+
+use crate::models::repos::RepoCommit;
+
+#[derive(Serialize)]
+pub struct SpecificPullRequestCommitBuilder<'octo, 'r> {
+    #[serde(skip)]
+    handler: &'r PullRequestHandler<'octo>,
+    #[serde(skip)]
+    pr_number: u64,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    per_page: Option<u8>,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    page: Option<u32>,
+}
+
+impl<'octo, 'r> SpecificPullRequestCommitBuilder<'octo, 'r> {
+    pub(crate) fn new(handler: &'r PullRequestHandler<'octo>, pr_number: u64) -> Self {
+        Self {
+            handler,
+            pr_number,
+            per_page: None,
+            page: None,
+        }
+    }
+
+    /// Results per page.
+    pub fn per_page(mut self, per_page: impl Into<u8>) -> Self {
+        self.per_page = Some(per_page.into());
+        self
+    }
+
+    /// Page number of the results to fetch.
+    pub fn page(mut self, page: impl Into<u32>) -> Self {
+        self.page = Some(page.into());
+        self
+    }
+
+    /// Send the actual request.
+    pub async fn send(self) -> crate::Result<crate::Page<RepoCommit>> {
+        let route = format!(
+            "/repos/{owner}/{repo}/pulls/{pr}/commits",
+            owner = self.handler.owner,
+            repo = self.handler.repo,
+            pr = self.pr_number,
+        );
+
+        self.handler.crab.get(route, Some(&self)).await
+    }
+}

src/models/repos.rs

@@ -38,7 +38,9 @@ pub struct RepoCommit {
     pub html_url: String,
     pub comments_url: String,
     pub commit: RepoCommitPage,
+    #[serde(deserialize_with = "maybe_empty::deserialize")]
     pub author: Option<Author>,
+    #[serde(deserialize_with = "maybe_empty::deserialize")]
     pub committer: Option<Author>,
     pub parents: Vec<Commit>,
 
@@ -392,3 +394,82 @@ pub struct MergeCommit {
 
 /// A HashMap of languages and the number of bytes of code written in that language.
 pub type Languages = std::collections::HashMap<String, i64>;
+
+mod maybe_empty {
+    use serde::{Deserialize, Deserializer};
+
+    #[derive(Deserialize)]
+    #[serde(deny_unknown_fields)]
+    struct Empty {}
+
+    #[derive(Deserialize)]
+    #[serde(untagged)]
+    enum MaybeEmpty<T> {
+        Empty(Empty),
+        Something(T),
+    }
+
+    impl<T> From<MaybeEmpty<T>> for Option<T> {
+        fn from(value: MaybeEmpty<T>) -> Self {
+            match value {
+                MaybeEmpty::Something(t) => Some(t),
+                _ => None,
+            }
+        }
+    }
+
+    pub fn deserialize<'de, D, T>(d: D) -> Result<Option<T>, D::Error>
+    where
+        T: Deserialize<'de>,
+        D: Deserializer<'de>,
+    {
+        Ok(Option::<MaybeEmpty<T>>::deserialize(d)?.and_then(Into::into))
+    }
+
+    #[cfg(test)]
+    mod tests {
+        use super::*;
+
+        use serde_json::json;
+
+        #[derive(Deserialize, Debug)]
+        struct Struct {
+            #[serde(deserialize_with = "deserialize")]
+            value: Option<u32>,
+        }
+
+        #[test]
+        fn deserialize_null_to_none() {
+            let actual: Struct = serde_json::from_value(json! {
+                {
+                    "value": null,
+                }
+            })
+            .unwrap();
+
+            assert!(actual.value.is_none());
+        }
+
+        #[test]
+        fn deserialize_empty_to_none() {
+            let actual: Struct = serde_json::from_value(json! {
+                {
+                    "value": {},
+                }
+            })
+            .unwrap();
+
+            assert!(actual.value.is_none());
+        }
+
+        #[test]
+        fn deserialize_invalid() {
+            serde_json::from_value::<Struct>(json! {
+                {
+                    "value": "hello world",
+                }
+            })
+            .unwrap_err();
+        }
+    }
+}

tests/generate_release_notes_test.rs

@@ -57,5 +57,5 @@ async fn should_return_page_with_check_runs() {
 
     let response = result.unwrap();
     assert_eq!(response.name, tag_name);
-    assert_eq!(response.body.is_empty(), false);
+    assert!(!response.body.is_empty());
 }

tests/pull_request_commits_test.rs

@@ -46,9 +46,9 @@ async fn should_return_pull_request_commits() {
 
     let result = client
         .pulls(OWNER, REPO)
-        .pull_number(PULL_NUMBER)
+        .pr_commits(PULL_NUMBER)
         .page(0u32)
-        .commits()
+        .send()
         .await;
 
     assert!(
@@ -69,3 +69,33 @@ async fn should_return_pull_request_commits() {
         assert_eq!(author.clone().unwrap().login, "octocat");
     }
 }
+
+#[tokio::test]
+async fn should_return_pull_request_commits_empty_author_object() {
+    let pull_request_commits_response: Vec<RepoCommit> = serde_json::from_str(include_str!(
+        "resources/pull_request_commits_empty_author_object.json"
+    ))
+    .unwrap();
+    let template = ResponseTemplate::new(200).set_body_json(&pull_request_commits_response);
+    let mock_server = setup_api(template).await;
+    let client = setup_octocrab(&mock_server.uri());
+
+    let result = client
+        .pulls(OWNER, REPO)
+        .pr_commits(PULL_NUMBER)
+        .page(0u32)
+        .send()
+        .await;
+
+    assert!(
+        result.is_ok(),
+        "expected successful result, got error: {:#?}",
+        result
+    );
+
+    let mut commits = result.unwrap();
+    let items = commits.take_items();
+
+    assert!(items[11].author.is_some());
+    assert!(items[12].author.is_none());
+}