Web API: Endpoint does not work when identifier contains space

[tdesveaux]

I setup a reproduction unit test here: https://github.com/tdesveaux/buildbot/tree/tds/api_space_identifier_issue

I found this issue while writing a script to get a Build's logs. Some of the steps names contained spaces and I was getting 404 when querying their log with /builders/n:builderid/builds/n:build_number/steps/i:step_name/logs/i:log_slug/raw.

I also reproduced the issue by creating a master, and changing the Builder name from runtests to run tests /.
checkconfig didn't complain, running the master and a build worked fine.
However, a query on /api/v2/builders/run+tests+%2F results in {"error": "Invalid path: builders/run+tests+/"}.

It looks like the issue is due to the data api using the Identifier to validate the arguments, which does not allow spaces and other characters. But Builder.name, BuildStep.name (and probably others) which are used as identifiers are not validated at config time to bring up the issue.

I'm unsure how to proceed to fix this.
Either, check that any field that can be used as an API identifier is valid, which can be tricky if some of them are renderered (fail checkconfig, log a warn at runtime?).
Or make the API accept identifiers which contains any string.

[tdesveaux]

I looked a bit more into this and I'm getting contradicting information.

In DB, Builder's name has no restrictions:

buildbot/master/buildbot/db/model.py

Line 672 in 82d0eaf

sa.Column('name', sa.Text, nullable=False),

There is restriction on size or characters in the config either:

buildbot/master/buildbot/config/builder.py

Lines 52 to 60 in 82d0eaf

	if not name or type(name) not in (bytes, str):
	error("builder's name is required")
	name = '<unknown>'
	elif name[0] == '_' and name not in RESERVED_UNDERSCORE_NAMES:
	error(f"builder names must not start with an underscore: '{name}'")
	try:
	self.name = bytes2unicode(name, encoding="ascii")
	except UnicodeDecodeError:
	error("builder names must be unicode or ASCII")

There is even tests that lead to a broken data-api:

buildbot/master/buildbot/test/unit/config/test_builder.py

Lines 119 to 122 in 82d0eaf

	def test_unicode_name(self):
	cfg = BuilderConfig(name='a \N{SNOWMAN} c', workername='a', factory=self.factory)
	self.assertIdentical(cfg.factory, self.factory)
	self.assertAttributes(cfg, name='a \N{SNOWMAN} c')

The only place that seam to put restriction on a builder's name is the data-api here:

buildbot/master/buildbot/data/builders.py

Line 99 in 82d0eaf

name = types.Identifier(70)

(which make it validate that a builder name's length it less than 70 and match this regex: ^[a-zA-Z_-][a-zA-Z0-9._-]*$

There is a mention of limiting the buildername length in #3957 to avoid breaking the UI.
If that's the only reason, it should be up to the UI to correctly handle longer names, not the API. The UI is not the only client of the API (this break accessing these builders with Rest and probably GraphQL as well).

I updated my branch with an even clearer reproduction: master...tdesveaux:buildbot:tds/api_space_identifier_issue#diff-91b181695ef480960bcd9482cd44a4cd1408038053e86ec7924cedd71881871d

Using the same buildername used in the config.test_builder.py unicode test (which is considered valid), accessing the builder by name with the data api fails.