[barodeur]

Part of: #19414

When MSC4133 (custom profile fields) was implemented, the returned value for an unset display name changed from 200 {} to 200 { displayname: null }. This happened first on the unstable uk.tcpip.msc4133 path in #17488 (1.123.0), then on the stable path when #18635 (1.135.0) unified the displayname, avatar_url and custom field servlets. Neither PR discussed the change in review, so it looks like an unintended side effect of the refactor rather than a deliberate decision.

The v1.16 spec mandated to change from returning 200 {} to 404 but change was not identified as breaking and was eventually not implemented in other clients and server. This PR has a sister MSC that proposes to return to the pre-1.16 error codes: MSC4537.

Before:

GET /_matrix/client/v3/profile/@alice:test/displayname
  200 {"displayname": null}

After:

GET /_matrix/client/v3/profile/@alice:test/displayname
  200 {}

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Code style is correct (run the linters)

[devonh]

Should the generic profile field case also have the same fix applied?

[barodeur]

I think for now, the ecosystem, is going to accept a bit of inconsistencies with HomeServers either returning 200 {} or 404.

Here get_profile_field would raise StoreError(404, "No row found") and the HS would return 404.

I'm open to also making it return 200 {} for consistency, but the spec might change in the future back to 404 for all fields, because that is probably the correct REST semantic response

The next final change I would like to make on this matter is rather changing definitely to 404 for all fields once the spec settles and clients all accepts 404 as response for unset fields.

[devonh]

Fair enough. I agree with your assessment.

It's probably worth adding a comment in with this information so future developers don't have to go digging for why the different cases treat response handling in different ways.

[devonh]

Other than the one question, the changes here look complete to me.

[barodeur]

👋 thanks for your review, I replied to your question