-
-
Notifications
You must be signed in to change notification settings - Fork 726
Add GET /api/v1/comments Endpoint #1361
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
base: main
Are you sure you want to change the base?
Conversation
Hey @darddan Thanks for this. Just on the limit - you're right to point out that it might be an issue for some instances. Not having a limit for the existing endpoint that lists comments for a post is I suppose OK - since you're unlikely to get that many for a single post. But if you're listing ALL comments from ALL posts, that's potentially a LOT of comments, so I would like to see a limit on there, with a default. Which means we'd need some sort of support for paging there too (I don't think any of the existing endpoints have this - listing posts has a limit with a default, but no "from" or similar) So - I think we're going to need to add support for that really... What do you think? |
@darddan I've added some paging logic - defaults to 50, you can go up to 100, so if you have more comments than 100, you'll need to check the pagination stuff - how does that look to you? |
@darddan I'm not sure about the usefulness of just returning "CommentRefs" - that's obviously useful for your scenario but for others I think it's more sensible / useful to return the same comment info that we return when you get comments for a post |
Hi @mattwoberts, appologies for the late response. Regarding the 100 comments limit / pagination: The Regarding the CommentRefs: I couldn't think of any other use case for this endpoint unless if you're using it as a |
@darddan Did you see I added paging to this PR - could you take a look at that and see what you think, and if you think it would work with your use case still with these changes? |
@mattwoberts looks great, thanks a lot. These changes would work for me |
Issue: #1360
I've built a bot that will poll my fider instance for new comments and in case there's an instruction for the but there it will do some processing and it will enrich the post. For that, in the current implementation I have to fetch all the comments from all the existing posts and filter what I've already processed.
This PR introduces a new endpoint
GET /api/v1/comments
with an optional query paramsince
where you can pass a date to filter out comments created/edited before that date.Data model:
In this endpoint I only return the
comment_id
,post_id
,user_id
,created_at
andchanged_at
. Because of that I created a new model calledCommentRef
and didn't use the currentComment
one.Indexing:
I'm assuming the
tenant_id
is used for the hosted service that you provide, so I added an index fortenant_id, COALESCE(edited_at, created_at)
to assure the performance stays decent.Limit:
I didn't add a limit parameter to limit the comments in the response. Not only because I didn't actually need them in my instance, but I also noticed that they're not present in other api requests. This might lead to too much memory use or similar issues for larger platforms, so it might important for your hosted instance