Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use post request and correct content type #1

Merged
merged 2 commits into from
Sep 18, 2023
Merged

Conversation

art0007i
Copy link
Contributor

without these changes this library failed to log in every time for me...

@ljoonal ljoonal added the bug Something isn't working label Aug 26, 2023
@ljoonal ljoonal self-requested a review August 26, 2023 14:51
@@ -173,7 +173,7 @@ impl racal::reqwest::ApiClient<NoAuthentication> for UnauthenticatedCVR {
&self, req: RequestBuilder,
) -> Result<RequestBuilder, racal::reqwest::ApiError> {
self.http_rate_limiter.until_ready().await;
Ok(req)
Ok(req.header("Content-Type", "application/json"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be added to ApiConfiguration::to_headers instead, as some requests may want to overwrite this, and setting it explicitly always just before making the requests would stop that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I've looked into this a bit more and I'd say it should be included in the racal package anyway since the body function inside of Queryable returns an Option with a serde_json::Result and I think it's safe to assume that if something is there then it's json data.
but correct me if anything I've said here is wrong since I'm not fully confident about this

I also found that neos_rs had the same header issue, so fixing it only in this package wouldn't actually fix everything

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eeeh, could provide it as a default there, but other types of data can be posted too, for example I'd expect that image uploads might be a thing that would be neat to support, so it would need to at least be something that can be overridden in the end...

tests/common/mod.rs Outdated Show resolved Hide resolved
tests/login.rs Outdated Show resolved Hide resolved
@ljoonal ljoonal merged commit c2956e7 into onlivfe:main Sep 18, 2023
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants