[schema] Correct null value handling for the AS Registration’s url property

[V02460]

Fixes the url property’s nullability of the Application Service Registration schema.

The Spec states that the Registration’s url property can be “optionally set to null if no traffic is required”, but the corresponding schema does not allow this:

matrix-spec/data/api/application-service/definitions/registration.yaml

Lines 21 to 23 in 23ff7f1

	url:
	type: string
	description: The URL for the application service. May include a path after the domain name. Optionally set to null if no traffic is required.

This PR fixes the problem by making the url property nullable, i.e., give it type: ["null", "string"].

I find this change to be between a clarification and a backwards-compatible change. Still I want to note the possibility of a Registration consumer breaking for new setups, because they didn’t expect null values for the url property to be present and will now have to handle them.

In a previous iteration of this PR I additionally removed url from the list of required properties and made null its default value. I did this to get consistent meaning for the url property being unset and the property being set to null; but I removed it, because the Synapse source code seems to consider it not a bug, but a feature:

'url' must either be a string or explicitly null, not missing to avoid accidentally turning off push for ASes.

From synapse/config/appservice.py#L115-L116

Pull Request Checklist

• Pull request includes a changelog file
• Pull request includes a sign off
• Pull request is classified as 'other changes'

Preview: https://pr2130--matrix-spec-previews.netlify.app

[anoadragon453]

I double-checked ruma, which is used by Conduit, just in case.

They appear to also treat url as an optional type, specifically calling out that null = don't send traffic.

So this indeed looks like a case of just fixing the schema. Various tools will thank you!

[V02460]

@anoadragon453: Ruma’s implementation does differ from what Synapse implements and the Spec now says: While the former two do not allow deserialization with a missing url field, Ruma does allow that. This is worth considering, because the mentioned Synapse comment especially calls out that case.

To unify the behavior, there are two ways forward:

1. The Spec is right: Change Ruma to no longer accept registrations without an url field.
2. Change the Spec: Remove url from the list of required attributes (and adapt Synapse accordingly), by that allowing a missing url field.

I’m not quite sure what the better option is; it’s choosing between a user footgun and ugly missing-vs-null semantics.

[anoadragon453]

It does seem better to me to explicitly require url, as that prevents the footgun of users missing out url and thus "silently" disabling sending events to the AS. The spec indeed already required the url field, so from a purely spec perspective, it would be up to ruma to change their implementation.

However I recognise that this now likely means that ruma needs to implement a custom deserialiser function for this type to differentiate between url: null and the field being omitted, which is a pain. It likely will cause pain in other languages too.

I think the best way forwards is to:

1. Update ruma's implementation to align with the spec.
2. Update the spec to remove this silly distinction and add something more simple (i.e. send_events: true|false) to simply implementation code.

I've opened an issue on ruma's issue tracker to make them aware: ruma/ruma#2074

[anoadragon453]

Ruma ended up changing their implementation in ruma/ruma#2077.

I've created #2155 to track the second portion of the above plan.