.teams().repos().remove() always fails with EOF error

[manchicken]

There are several APIs which result in the GitHub API which return 204 and have no response body. DELETE /orgs/{org}/teams/{team_slug}/repos/{owner}/{repo} is one of those.

At the moment, the FromResponse::from_response() function does not support responses with no body. It looks like this:

/// A trait for mapping from a `http::Response` to an another type.
#[async_trait::async_trait]
pub trait FromResponse: Sized {
    async fn from_response(response: http::Response<hyper::Body>) -> crate::Result<Self>;
}

#[async_trait::async_trait]
impl<T: serde::de::DeserializeOwned> FromResponse for T {
    async fn from_response(response: http::Response<hyper::Body>) -> crate::Result<Self> {
        let (_, body) = response.into_parts();
        let body = hyper::body::to_bytes(body)
            .await
            .context(crate::error::HyperSnafu)?;
        let de = &mut serde_json::Deserializer::from_slice(&body);

        // THIS COMMENT ADDED FOR DEMONSTRATION PURPOSES, IGNORE UNTIL LATER

        return serde_path_to_error::deserialize(de).context(crate::error::JsonSnafu);
    }
}

I have added a test in this draft PR which demonstrates the error.

I think the fix is in that self-same FromResponse::from_response() function, right where I added the comment. It seems like you could just check whether body.is_empty() and then return something that conforms to the crate::Result<Self>.

I've been trying to put together a fix for it, but my Rust is perhaps too weak and I am having a difficult time figuring out exactly what I can return from this function to make it play nice.

If anybody has any thoughts, I'm happy to do the work on a fix here. I already have a test, and I'm going to expand the test once I navigate this issue.

Collaboration is appreciated, thanks.

Solidarity.

[manchicken]

I could also see changing the Octocrab::delete() function, as I don't think any of the DELETE APIs return a body in the response. A number of the responses from these APIs return a 204 but no body. A bunch of the APIs which have side-effects fit into that category.

Maybe body.is_empty() is the wrong test, but instead we should do a check for that return status. Maybe both?

[manchicken]

So, I just pushed commits to my PR #503. Those changes do kinda fix this issue, but I don't like the fix since it relies on using what seems to conventionally be a non-public function.

I'm trying to keep the change as light-touch as possible so that it won't break anybody's code. I'm still pretty new to the language, though, so I'm probably missing something. I would appreciate feedback.

[manchicken]

It looks like .teams().repos().add_or_update() may also be failing with errors, but the code in the current version of the crate suppresses the error. In testing it I have found that it has no effect, but still returns Ok(()).

[XAMPPRocky]

Thank you for your issue! I think you just need to use the _delete method instead of delete.

[manchicken]

Are there any delete methods for which .delete() functions as expected? I wasn't able to find any.

[XAMPPRocky]

I don't think so, but I don't have an complete reference of GitHub APIs in my head 😄

[manchicken]

I added a list of endpoints which return a 204 as an attached CSV file above. I have a CSV file which has all of the endpoints which return 204, but GitHub is being goofy about attaching it.

I think the bigger issue here is that we don't have tests or good support for those endpoints.

I'm willing to commit to adding tests and troubleshooting if we can hammer out an agreement on what the best approach to a more general fix.

[manchicken]

Here's the CSV file: #506

[XAMPPRocky]

I think we can just make delete equivalent to _delete. That seems like the solution to me.

[manchicken]

Groovy. I’ll add some tests and see how that goes. This seems like a breaking change, somehow?

[XAMPPRocky]

Groovy. I’ll add some tests and see how that goes. This seems like a breaking change, somehow?

Yes but that's fine.

[manchicken]

Groovy. I'll go in that direction. I should have an updated PR in a week or two.