Support for more default ports in Uri class

[JKRhb]

The Uri class currently only supports the default ports for the http and https schemes:

sdk/sdk/lib/core/uri.dart

Lines 1718 to 1723 in 50b94ef

	/// The default port for the scheme of this Uri.
	static int _defaultPort(String scheme) {
	if (scheme == "http") return 80;
	if (scheme == "https") return 443;
	return 0;
	}

For a library I am contributing to, it would be handy if more URI schemes could be added here. Would you be fine with turning the checks in this method into a switch statement with the possibility to add more URI schemes? If so, I would open a PR/hand in a patch with an initial proposal.

[mit-mit]

cc @brianquinlan @lrhn

[lrhn]

We try not to have to recognize too may schemes, currently just package:, data:, http:, https: and file:.
The Uri class is already one of the largest classes in the core platform libraries, and if anything, we want to make it smaller, not larger.

It's easy to add more default ports, but I don't want to have to support any other special-cases of other schemes (like special canonicalization rules or path resolution rules). They'd have to be completely standard hierarchical URIs with normal host names and ports.
Adding some support for a scheme, but not fully supporting the scheme's special cases, can be deceptive.

So, which that in mind, which schemes do you propose to add default ports for?

From a quick glance, ws, wss, nntp, gopher, sftp, and maybe ftp seem to make sense.

In practice, I'd probably just support ws and wss, which have ports 80 and 443 too, and not get into too many "old" protocols.

[JKRhb]

Thank you, @irhn, for the quick response and your explanations!

So, which that in mind, which schemes do you propose to add default ports for?

My use-cache is probably a bit niche and concerns CoAP (RFC 7252), a web transfer protocol for IoT scenarios and the interaction with constrained devices, which is quite similar to HTTP, but more resource-efficient.

If I see it correctly, in order to support CoAP and its different URI schemes, adding the default ports would be sufficient (see section 6 of RFC 7252 for more information). However, I could understand if you want to focus on more commonly used protocols to keep things as maintainable as possible. Edit: Then again, this addition would probably cause little to no additional maintenance effort in the future due to the close alignment of CoAP with HTTP.

In practice, I'd probably just support ws and wss, which have ports 80 and 443 too, and not get into too many "old" protocols.

In any case, adding additional support for these two URI schemes sounds great :)

[JKRhb]

I created #51203 (https://dart-review.googlesource.com/c/sdk/+/280215) with an initial proposal which could be used for further discussing this issue.

[lrhn]

The implementation looks fine.

My main worry is that it feels arbitrary. Why these? What is the protocol for adding more in the future?
Should we just take the entire "official" list of default numbers once and for all? (And again, when and how do we update?)

A platform library is not the best place to place large swathes of data that is almost never used.
(There is a reason package:characters is a package, it has a 15K table just to do the most basic grapheme cluster breaking. You shouldn't include that unless you actually need it.)

Should we instead give user code a way to add default ports, so you can add coad ports once and for all, say before parsing your first coad URI, and then it'll just work for the rest of the program.
(But what if other code in the same program tries to set the value again, should it overwrite or throw? Probably throw.)

That's what we have considered doing for Encodings, allowing user provided name-encoding mappings to be added.

[JKRhb]

Thank you for the feedback, @lrhn!

My main worry is that it feels arbitrary. Why these? What is the protocol for adding more in the future?
Should we just take the entire "official" list of default numbers once and for all? (And again, when and how do we update?)

I understand your concern. I'd say if support for more default URI ports would be added to the core library, then new ports should be requested via an issue, and limited to those registered in the IANA registry that do not require additional handling in the Uri class (as you outlined before).

Should we instead give user code a way to add default ports, so you can add coad ports once and for all, say before parsing your first coad URI, and then it'll just work for the rest of the program.
(But what if other code in the same program tries to set the value again, should it overwrite or throw? Probably throw.)

That would be a very good alternative!

[JKRhb]

I'd say if support for more default URI ports would be added to the core library, then new ports should be requested via an issue, and limited to those registered in the IANA registry that do not require additional handling in the Uri class (as you outlined before).

Hmm, would you be fine with the PR/CL if this policy was followed for future additions? Or would you rather prefer closing/adapting it to a subset of the proposed schemes and introducing an extension mechanism instead?

[lrhn]

I'd probably prefer an extension mechanism only.

Keeping up with an living external definition is a Sisyphos task (q.v., Unicode), and having a policy where others can request updates is just adding more work for people who have no personal interest in the result.

Which means I'd lean towards adding a method like static addDefaultPorts(Map<String, int> protocolPorts) { ... } which allows anyone to add their favorite protocol defaults, whether the protocol is public or not, to the system.

The biggest issue with that is that the call need to happen before anyone calls Uri.parse or any other constructor.
Another, or additional, option is to make the mapping an optional parameter to Uri.parse, constructors and Uri.toString.
(Changing Uri.toString could be breaking, though, if anyone implements Uri.)

We can even choose to cache the input, and update the global tables, the first time we see a map argument to one of those functions.

[JKRhb]

I'd probably prefer an extension mechanism only.

Keeping up with an living external definition is a Sisyphos task (q.v., Unicode), and having a policy where others can request updates is just adding more work for people who have no personal interest in the result.

You're right about that. The extension mechanism you've proposed sounds good and would also cover my use-case. Should we close #51203 (and the associated CL) then? Or would some ports (e.g. those for WebSockets) still make sense to add?

Which means I'd lean towards adding a method like static addDefaultPorts(Map<String, int> protocolPorts) { ... } which allows anyone to add their favorite protocol defaults, whether the protocol is public or not, to the system.

The biggest issue with that is that the call need to happen before anyone calls Uri.parse or any other constructor.
Another, or additional, option is to make the mapping an optional parameter to Uri.parse, constructors and Uri.toString.
(Changing Uri.toString could be breaking, though, if anyone implements Uri.)

Adding an addDefaultPorts method and adjusting Uri.parse sounds like the least invasive approach to me. I suppose Uri.toString can probably not be adjusted, though, since then it would not be a valid override anymore?

We can even choose to cache the input, and update the global tables, the first time we see a map argument to one of those functions.

Hmm, would you say that all consecutive calls should then be ignored? Or should an exception be thrown if an already registered scheme is re-registered to a different port? In any case, I think it would be nice if you had the flexibility to integrate additional protocol support via dedicated libraries that can each register their needed URI ports independently (e.g., the first time a respective protocol client is created).

[JKRhb]

I now updated #51203 (and the corresponding CL) to only add default ports for ws and wss. Let me know if this change makes sense, otherwise we can simply close the PR/CL.

[lrhn]

I think this addition is general and valuable enough, so let's go ahead!
(I don't think this can break anyone. Right? 😅)

[lrhn]

Turns out it broke some tests which used ws as scheme and assumed no default port.

A default port is removed from the toString output string, which means that Uri.parse("ws://host:80/") changes its toString from "ws://host:80/" to "ws://host/".
If someone, somewhere, depends on that :80 being there, perhaps because they're reading the string back with an older version of Dart, then things break.

(Everything is breaking! Changes are hard. Let's go shopping.)

[JKRhb]

That is unfortunate, but probably not unexpected :/ Should we abandon this endeavor then (and maybe turn to the solution for passing additional default ports instead)? Or could this breaking change be a part of Dart 3?

[lrhn]

Not planning on any (further) breaking changes for Dart 3.0. The ones we do add are mostly language based, which means they are language versioned and won't affect code until it upgrades to using the 3.0 language.

It's probably safer to just add the configuration option. I'll consider how to do that (but with lower priority than the 3.0-related library changes).