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(posts): return error on invalid community name #3418

Merged

Conversation

lgerard-pass
Copy link
Contributor

This should fix #3383

Before when we get an error while listing posts on a community, because the community does not exists, we transform the Result we get into an Option, after that there is no way to recognize whether a community name was given and not found, or if no name was given at all.

To fix this, I'm simply returning the error if there is one, if not I simply wrap the value we get in an Option.

This gives a 404 error when requesting http://localhost:8536/api/v3/post/list?community_name=InvalidCommunityName with the Record not found message which from my testing is the same behavior we get if we try to follow a community that does not exists.

I'm just starting out Rust so please don't hesitate to let me know if that's not a good way to implement it.

@Nutomic
Copy link
Member

Nutomic commented Jun 30, 2023

Makes sense. You should also make the same change in list_comments.rs and search.rs.

@lgerard-pass lgerard-pass force-pushed the fix/list-post-community-not-found branch 2 times, most recently from a81598a to 76a69b9 Compare June 30, 2023 11:53
@lgerard-pass
Copy link
Contributor Author

Yep,
I've added these changes and I get the same error querying :

http://localhost:8536/api/v3/search?q=test&community_name=InvalidCommunity
http://localhost:8536/api/v3/comment/list?community_name=InvalidCommunity

Running these calls with a valid community returns the expected results.

@Nutomic Nutomic enabled auto-merge (squash) June 30, 2023 13:19
@lgerard-pass lgerard-pass force-pushed the fix/list-post-community-not-found branch from 76a69b9 to 1f5d336 Compare June 30, 2023 14:26
@Nutomic
Copy link
Member

Nutomic commented Jul 3, 2023

Ups I just merged another PR which causes a conflict with this one, can you fix the conflicts?

auto-merge was automatically disabled July 3, 2023 17:14

Head branch was pushed to by a user without write access

@lgerard-pass lgerard-pass force-pushed the fix/list-post-community-not-found branch from 1f5d336 to f85f9e2 Compare July 3, 2023 17:14
@dessalines dessalines enabled auto-merge (squash) July 3, 2023 17:25
auto-merge was automatically disabled July 3, 2023 17:26

Head branch was pushed to by a user without write access

@lgerard-pass lgerard-pass force-pushed the fix/list-post-community-not-found branch 3 times, most recently from cdd25b8 to 6eac707 Compare July 3, 2023 18:32
@lgerard-pass
Copy link
Contributor Author

I've rebased as you asked, I also add to update the branch a few times to keep up with main but unless another commit is merged, it should be ready for merging.

@Nutomic Nutomic force-pushed the fix/list-post-community-not-found branch from 6eac707 to f31e52e Compare July 4, 2023 10:14
@Nutomic Nutomic enabled auto-merge (squash) July 4, 2023 10:14
@Nutomic Nutomic disabled auto-merge July 4, 2023 11:04
@Nutomic
Copy link
Member

Nutomic commented Jul 4, 2023

Federation tests are failing in CI but passing locally, so Im merging this manually.

@Nutomic Nutomic merged commit 85dab14 into LemmyNet:main Jul 4, 2023
1 check failed
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.

[Bug]: GetPosts returns all Posts if community_name is not found
3 participants