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

[feature] Conversations API #3013

Merged

Conversation

VyrCossont
Copy link
Contributor

@VyrCossont VyrCossont commented Jun 16, 2024

Description

Implements a Mastodon-compatible DM conversations API.

Closes #1312

Checklist

  • I/we have read the GoToSocial contribution guidelines.
  • I/we have discussed the proposed changes already, either in an issue on the repository, or in the Matrix chat.
  • I/we have not leveraged AI to create the proposed changes.
  • I/we have performed a self-review of added code.
  • I/we have written code that is legible and maintainable by others.
  • I/we have commented the added code, particularly in hard-to-understand areas.
  • I/we have made any necessary changes to documentation.
  • I/we have added tests that cover new code.
  • I/we have run tests and they pass locally with the changes.
  • I/we have run go fmt ./... and golangci-lint run.

TODO

  • Deleting conversations when the owning account is deleted
  • Updating conversations when the last status is deleted
  • Duplication of code related to filtering & muting (best handled in a separate PR since it'd involve extending the visibility cache)
  • Testing conversation updates
  • Interaction with updated statuses (should editing a status in the middle of a conversation mark it as unread?) (no, other implementations don't do this)
  • Test that it generates the correct notification and expected notification payload for a DM
  • Test that it doesn't do those things for non-DMs

@VyrCossont VyrCossont force-pushed the conversations-api branch from 548b244 to bf42ce8 Compare July 1, 2024 20:17
@VyrCossont VyrCossont force-pushed the conversations-api branch from 87f5497 to 31b260d Compare July 2, 2024 18:14
@VyrCossont
Copy link
Contributor Author

VyrCossont commented Jul 12, 2024

There are some experimental bits in this one:

  • Advanced migrations: a table that stores a pre-assigned ID, finished flag, and state JSON blob, and a point in server startup that runs them after regular migrations and most of the app setup. (I note this because it's not triggered by CLI tools.) Right now we have one advanced migration, which stores the progress of creating conversations for DMs that predate this feature, in case the task is interrupted before it finishes. That process requires substantial amounts of app logic so we can't run it as part of bundb migrations. We might want to run advanced migrations in a background goroutine?
  • db/test contains a reusable factory struct mixin to create conversations and statuses for them, with ULIDs and timestamps based on a fake clock that we control. I'd like to expand this to reduce test copypasta in the future: our fixtures are fragile, and we should generate more things as needed rather than having a shared set of test objects.
  • Deleting a conversation's last status runs some mildly complex SQL to revert to the most recent status in the conversation that still exists. This uses temp tables to avoid repeating queries, since we need to self-join a derived table, and also use the results for both updates and deletes. It took some iteration but it uses them in a way that works in PG and SQLite.

@VyrCossont VyrCossont marked this pull request as ready for review July 12, 2024 20:59
@VyrCossont VyrCossont changed the title Conversations API [feature] Conversations API Jul 12, 2024
internal/cache/size.go Outdated Show resolved Hide resolved
// in each conversation for which the deleted status is the last status
// (if there are such statuses).
conversationStatusesTempTable := bun.Ident("conversation_statuses_" + id.NewULID())
if _, err := tx.NewRaw(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm really not very keen on these raw transactions tbh. Is there no way with bun to do this jiggery pokery? If there is, I'd much rather we use that, since it gives us all the dialect-specific escaping, sanitization, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do. This iteration ended up not using anything that we definitely can't do in Bun.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not quite true, turns out Bun's NewCreateTable can't do CREATE TABLE … AS SELECT … because it assumes that there's always a parenthesized column list, so we still have to use raw queries to create these tables, or do even more work to rewrite them as the more portable CREATE TABLE followed by INSERT … SELECT. But we can use a very short raw query parameterized with a Bun subquery for the SELECT part, and I think that's a decent tradeoff.

I also think we're not getting anything in terms of escaping or type checking by using Bun's query builders that pure raw queries can't also do: raw queries can still use placeholders and escaping, and Bun's query builders can't tell me if I spelled a column name wrong at compile time, so the only benefit is that a subset of SQL keywords get checked at compile time, and the cost is that it's a lot less readable, IMO, especially with our frequently-unnecessary house style use of bun.Ident.

Take a look, and let me know if I'm missing any tricks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we end up using this pattern again, we can add a small CreateTableAsQuery/NewCreateTableAs helper along the lines of UpsertQuery/NewUpsert, but it doesn't seem worth it for two usages in the same part of the same file.

- Add context info for errors
- Extract some common processor code into shared methods
- Move conversation eligibility check ahead of populating conversation
@tsmethurst
Copy link
Contributor

I think as soon as you've resolved Kim's comments and tests pass, click the squerge button! 😍

@NyaaaWhatsUpDoc
Copy link
Member

fantastic work on this PR 👌

@NyaaaWhatsUpDoc NyaaaWhatsUpDoc merged commit 8fdd358 into superseriousbusiness:main Jul 23, 2024
2 checks passed
@VyrCossont VyrCossont deleted the conversations-api branch July 26, 2024 23:31
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.

[feature] Implement /api/v1/conversations masto API endpoints for viewing direct message (dm) conversations
3 participants