s2s/query: clarify profile behaviour and API responses#2326
s2s/query: clarify profile behaviour and API responses#2326famfo wants to merge 2 commits intomatrix-org:mainfrom
Conversation
data/api/server-server/query.yaml
Outdated
There was a problem hiding this comment.
While we are touching this endpoint, I believe that we should remove this enum definition, because it shows as:
One of: [displayname, avatar_url].
But with extended profile fields, more values are possible.
| The display name of the user. MUST either be omitted or set to `null` if | ||
| the user does not have a display name set. |
There was a problem hiding this comment.
Aside: I was a bit surprised by this, but then I found d914c40, and this seems to be just a clarification of that changes.
|
@richvdh it's not clear to me if this is ready to merge or not |
|
sorry, I thought it was, but then I saw the requests from @zecakeh for further changes |
Signed-off-by: famfo <famfo@famfo.xyz>
|
@famfo please don't force-push between rounds of review: it makes it very hard to see what has changed since the previous round. |
data/api/server-server/query.yaml
Outdated
| - allow users to set arbitrary key/value pairs in their profile in addition to the | ||
| specified pairs | ||
| - deny profile look-up over federation by responding with 403 and an error code of | ||
| `M_FORBIDDEN` | ||
| - omit certain key/value pairs in the response |
There was a problem hiding this comment.
Per https://github.com/matrix-org/matrix-spec/blob/main/meta/documentation_style.rst#general:
| - allow users to set arbitrary key/value pairs in their profile in addition to the | |
| specified pairs | |
| - deny profile look-up over federation by responding with 403 and an error code of | |
| `M_FORBIDDEN` | |
| - omit certain key/value pairs in the response | |
| - Allow users to set arbitrary key/value pairs in their profile in addition to the | |
| specified pairs. | |
| - Deny profile look-up over federation by responding with 403 and an error code of | |
| `M_FORBIDDEN`. | |
| - Omit certain key/value pairs in the response. |
There was a problem hiding this comment.
Why is this endpoint talking about setting data? Shouldn't that only be the CS docs?
There was a problem hiding this comment.
To set expectations for requesting servers. A server doesn't have to implement the c2s API.
There was a problem hiding this comment.
@clokep leaving this thread open for you to assess if you're satisfied with the answer. I agree it's not terribly clear as written here but I think it's an improvement over the current spec and I don't have any immediate suggestions for improvements.
data/api/server-server/query.yaml
Outdated
| m.tz: | ||
| type: string | ||
| description: The name of the user's time zone. The name MUST be registered in | ||
| the [IANA Time Zone Database](https://www.iana.org/time-zones). |
There was a problem hiding this comment.
It would be good to give this an x-addedInMatrixVersion tag. Since m.tz was added in spec v1.16 (it looks like this change got forgotten in #2206), we should probably set it to "1.16".
There was a problem hiding this comment.
should the other fields also get a x-addedInMatrixVersion: "1.16"?
There was a problem hiding this comment.
well no, because they haven't been newly added to the spec?
data/api/server-server/query.yaml
Outdated
| matching field MUST included in the response. If no `field` was specified, | ||
| the response MUST include the fields of the user's profile that can be made |
There was a problem hiding this comment.
I think there's some missing words:
| matching field MUST included in the response. If no `field` was specified, | |
| the response MUST include the fields of the user's profile that can be made | |
| matching field MUST be included in the response. If no `field` was specified, | |
| the response MUST include all of the fields of the user's profile that can be made |
I'm still not sure about the fist sentence. I think you're trying to say "the response MUST only include that field", but I can't make that change as a suggestion.
| m.tz: | ||
| type: string | ||
| description: The name of the user's time zone. The name SHOULD be registered in | ||
| the [IANA Time Zone Database](https://www.iana.org/time-zones), requesting |
There was a problem hiding this comment.
Comma splice
| the [IANA Time Zone Database](https://www.iana.org/time-zones), requesting | |
| the [IANA Time Zone Database](https://www.iana.org/time-zones). Requesting |
| the response should include the fields of the user's profile that can be made | ||
| public, such as the display name and avatar. | ||
| The profile for the user. If a `field` is specified in the request, the response | ||
| MUST only included the specified `field`. If no `field` was specified, the response |
There was a problem hiding this comment.
| MUST only included the specified `field`. If no `field` was specified, the response | |
| MUST only include the specified `field`. If no `field` was specified, the response |
| public, such as the display name and avatar. | ||
| The profile for the user. If a `field` is specified in the request, the response | ||
| MUST only included the specified `field`. If no `field` was specified, the response | ||
| SHOULD include all of the fields of the user's profile, which can be made public. |
There was a problem hiding this comment.
This doesn't mean what you want it to mean. In any case, why have you removed the examples of display name and avatar?
| SHOULD include all of the fields of the user's profile, which can be made public. | |
| SHOULD include all of the fields of the user's profile that can be made | |
| public, such as the display name and avatar. |
Since the existing specification of the behavior is relatively fuzzy, I have chosen the stricter interpretations from multiple places saying something similar.
Pull Request Checklist
Preview: https://pr2326--matrix-spec-previews.netlify.app