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

MSC4133: Extending User Profile API with Key:Value Pairs #4133

Open
wants to merge 52 commits into
base: main
Choose a base branch
from

Conversation

tcpipuk
Copy link

@tcpipuk tcpipuk commented Apr 19, 2024

@tcpipuk tcpipuk changed the title MSC0000: Extending User Profile API with Key:Value Pairs MSC4133: Extending User Profile API with Key:Value Pairs Apr 19, 2024
@turt2live turt2live added proposal A matrix spec change proposal client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Apr 19, 2024
proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
@turt2live
Copy link
Member

@tcpipuk when you get a chance, please sign off on your changes to allow the MSC to eventually be eligible for acceptance.

@tcpipuk

This comment was marked as resolved.

@turt2live
Copy link
Member

Looks great, thanks!

@tcpipuk tcpipuk marked this pull request as ready for review April 19, 2024 19:46
@andrewzhurov
Copy link

Seems we discuss here a key-value CRDT, as keys would need to be updated over time. Personally, I'm all in favor.
With no regard be those events a part of a (messaging) Room or be in its own Profile Room (#1769) (which would be neat),
we need a mechanism to get the latest key-value pairs.

Matrix Event Graph is isomorphic to Merkle-CRDT that powers OrbitDB (key-value DB is one of the supported CRDT).
Taking an inspiration from how key-value CRDT works may be of help to spec it for those Servers wishing to opt-in.

proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
@turt2live
Copy link
Member

@mscbot concern [High] Checklist is incomplete
@mscbot concern [Medium] Clarity on how servers can deny a value being set, regardless of capability

@mscbot mscbot added the unresolved-concerns This proposal has at least one outstanding concern label Jan 4, 2025
@tcpipuk
Copy link
Author

tcpipuk commented Jan 5, 2025

This is an attempt to answer the authentication/rate-limiting/guest access requirements in the FCP checklist: f686090

Please post a review if any further clarification is required, thanks!

@tcpipuk tcpipuk requested a review from turt2live January 5, 2025 18:54
@turt2live
Copy link
Member

@mscbot resolve [Medium] Clarity on how servers can deny a value being set, regardless of capability
@mscbot resolve [High] Checklist is incomplete

@mscbot mscbot removed the unresolved-concerns This proposal has at least one outstanding concern label Jan 7, 2025
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

A couple of minor comments, but overall, this looks great. I particularly like that you're keeping the initial proposal small and encouraging future extensions via future proposals.

I know a lot of time and effort has gone into putting this proposal together, and it shows. Thank you.

proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

Very clear and easy to read proposal. Well done!

Only two minor comments below - the rest looks good to me!

unstable features. Proposal [MSC4175](https://github.com/matrix-org/matrix-spec-proposals/pull/4175)
demonstrates the process of defining new fields in the `m.*` namespace.

## Error Handling
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a case for M_MISSING_PARAM for if the client tries to set the profile field "foobar", yet neglects to include a "foobar" field in the PUT request body JSON?

I noticed this case was handled in the Synapse implementation, but clients will not be expecting it.

Copy link
Member

Choose a reason for hiding this comment

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

I copied that from the avatar URL profile endpoint, so probably is a spec bug that it was never documented?

Copy link
Author

Choose a reason for hiding this comment

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

I'm happy to take this opportunity to document it now? 🙂

Copy link
Member

@clokep clokep Jan 11, 2025

Choose a reason for hiding this comment

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

Sounds good! Was mostly implying it shouldn't be controversial!

Copy link
Author

@tcpipuk tcpipuk Jan 13, 2025

Choose a reason for hiding this comment

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

How's this? a11286a (I've since fixed the line-wrapping, as I forgot it the first time, oops...)

unstable features. Proposal [MSC4175](https://github.com/matrix-org/matrix-spec-proposals/pull/4175)
demonstrates the process of defining new fields in the `m.*` namespace.

## Error Handling
Copy link
Member

Choose a reason for hiding this comment

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

When it comes time to write the spec PR, the author will be trying to match up error codes to each individual endpoint.

Is it expected that all status code/error code combinations here can be used for all Client-Server and Server-Server endpoints introduced in this MSC? If so, could you explicitly say so at the beginning of this section?

Copy link
Author

Choose a reason for hiding this comment

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

My assumption was that the Server-Server endpoints would have identical errors to the current ones, as federation access is still read-only in this MSC.

I'm happy to try to make the Client-Server endpoints a bit clearer on which errors apply to each of them though!

I might not have time to work on this over the weekend as my schedule is pretty full, but I'll try to get time ASAP 🙂

Copy link
Member

Choose a reason for hiding this comment

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

My assumption was that the Server-Server endpoints would have identical errors to the current ones, as federation access is still read-only in this MSC.

That's fine! It's just not clear with the current copy. I'd explicitly state what you just said in the MSC to make it clear.

I'm happy to try to make the Client-Server endpoints a bit clearer on which errors apply to each of them though!

That would be appreciated!

Copy link
Author

@tcpipuk tcpipuk Jan 13, 2025

Choose a reason for hiding this comment

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

I've written this: 2a86235

Would you prefer I literally write which errors apply to each endpoint, or is a section like this ok? I'm not sure what works best for spec-writers, so let me know and I'll do my best to help!

@richvdh richvdh self-requested a review January 10, 2025 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API disposition-merge kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period.
Projects
Status: Ready for FCP ticks
Development

Successfully merging this pull request may close these issues.