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

Fix endoint slash #231

Merged
merged 7 commits into from
Dec 2, 2024
Merged

Fix endoint slash #231

merged 7 commits into from
Dec 2, 2024

Conversation

Alkarex
Copy link
Contributor

@Alkarex Alkarex commented Nov 27, 2024

The URLs sent to the API contained a slash too much:

Example for FreshRSS:

"POST //api/greader.php/reader/api/0/edit-tag HTTP/1.1" 200 2 "-" "okhttp/4.12.0"

"GET //api/greader.php/reader/api/0/tag/list?output=json HTTP/1.1" 200 654 "-" "okhttp/4.12.0"

The URLs sent to the API contained a slash too much:

Example for FreshRSS:

```
"POST //api/greader.php/reader/api/0/edit-tag HTTP/1.1" 200 2 "-" "okhttp/4.12.0"

"GET //api/greader.php/reader/api/0/tag/list?output=json HTTP/1.1" 200 654 "-" "okhttp/4.12.0"
```
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 21.43%. Comparing base (90875af) to head (e32758a).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@            Coverage Diff             @@
##             develop     #231   +/-   ##
==========================================
  Coverage      21.43%   21.43%           
  Complexity       441      441           
==========================================
  Files            168      168           
  Lines           8935     8935           
  Branches        1397     1397           
==========================================
  Hits            1915     1915           
  Misses          6905     6905           
  Partials         115      115           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Shinokuni
Copy link
Member

Thanks for pointing this! However I think we should fix account url slash duplication earlier than in Credentials. As we get account url from AccountCredentialsScreen, url is always normalized before being inserted in database. That means we can just remove the end point first slash. Nextcloud News is also affected by this.

Could you replace your solution in Credentials by simply removing the first slash from FreshRSS API end point in FreshRSSService?

@Alkarex
Copy link
Contributor Author

Alkarex commented Nov 30, 2024

Done

@Alkarex
Copy link
Contributor Author

Alkarex commented Nov 30, 2024

The broken CI test does not seem to be due to my fix

@Shinokuni
Copy link
Member

Oh okay so thank you for the fix but I must have explained it wrong 😅. I was talking about modifying directly the endpoint string in FreshRSSService instead of removing its first slash in Credentials as we shouldn't modify anything there.

We can ignore the CI as it seems to be a packaging error which appears sometimes and could be related to not enough memory for Gradle.

@Alkarex
Copy link
Contributor Author

Alkarex commented Dec 2, 2024

OK. Something like that?

@Shinokuni Shinokuni merged commit 6fdb360 into readrops:develop Dec 2, 2024
1 check passed
@Shinokuni
Copy link
Member

Yes perfect, thanks for that!

@Alkarex Alkarex deleted the fix-endpoint-slash branch December 2, 2024 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants