Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: main
Are you sure you want to change the base?
MSC4133: Extending User Profile API with Key:Value Pairs #4133
Changes from 33 commits
0e85b3b
0f373db
c5a3015
c8a5a1a
3157982
b8ed87a
a81f21f
e688eb1
39d5fa2
e160c71
a9546aa
0c43f50
613411a
4a9557f
fbb4e44
591999f
15a0bd1
26c59f7
30e82aa
2966c85
d97189e
ae19725
23a3a62
f32932c
4afb8b8
bb4fc76
09318e8
9b4741e
068d44e
fa381da
b373a55
3349123
9fa0096
da0a791
a948f5d
c82dab6
21ad83f
1726ef3
30d203e
43e1e1a
af87bbe
f686090
17cc30b
d620259
c1b419a
4b3a8ed
0f41c82
1b98d40
9b2918e
e00d2e9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like homeservers SHOULD enforce at least some of the requirements of the CNIG (key length, character set, starts with
[a-z]
)?I notice that an error is defined for
M_KEY_TOO_LARGE
which implies that there is some expectation here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, the wording of this should be clearer... homeservers should not be checking whether the key uses a known namespace, but they must still enforce the grammar. I'll try to make this a bit more explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had a go at this - what do you think? 9b2918e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I screwed up the line wrapping, so this should fix that: e00d2e9
There was a problem hiding this comment.
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 thePUT
request body JSON?I noticed this case was handled in the Synapse implementation, but clients will not be expecting it.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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? 🙂
There was a problem hiding this comment.
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 housing be controversial!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
That would be appreciated!