PR#465: Don't allow not-null empty arch/userID in listHosts - koji

koji

#465 Don't allow not-null empty arch/userID in listHosts

Merged 6 years ago by mikem. Opened 6 years ago by tkopecek.

tkopecek/koji issue463 into master

Don't allow not-null empty arch/userID in listHosts

Tomas Kopecek • 6 years ago

eeeef17

hub/kojihub.py

file modified

+7 -5

		`@@ -10498,27 +10498,29 @@`

		`clauses = []`
		`joins = []`
		`- if arches != None:`
		`+ if arches is not None:`
		`+ if not arches:`
		`+ raise koji.GenericError('arches option cannot be empty')`
		`# include the regex constraints below so we can match 'ppc' without`
		`# matching 'ppc64'`
		`if not (isinstance(arches, list) or isinstance(arches, tuple)):`
		`arches = [arches]`
		`archClause = [r"""arches ~ E'\\m%s\\M'""" % arch for arch in arches]`
		`clauses.append('(' + ' OR '.join(archClause) + ')')`
		`- if channelID != None:`
		`+ if channelID is not None:`
		`joins.append('host_channels on host.id = host_channels.host_id')`
		`clauses.append('host_channels.channel_id = %(channelID)i')`
		`- if ready != None:`
		`+ if ready is not None:`
		`if ready:`
		`clauses.append('ready is true')`
		`else:`
		`clauses.append('ready is false')`
		`- if enabled != None:`
		`+ if enabled is not None:`
		`if enabled:`
		`clauses.append('enabled is true')`
		`else:`
		`clauses.append('enabled is false')`
		`- if userID != None:`
		`+ if userID is not None:`
		`clauses.append('user_id = %(userID)i')`

		`query = QueryProcessor(columns=fields, tables=['host'],`

tkopecek commented 6 years ago

If empty string is passed, resulting SQL query is not valid.

puiterwijk commented 6 years ago

This looks good to me.

mikem commented 6 years ago

I don't think we should construe an explicit value of "" to mean the same as None. It's an invalid input and we should raise an error, just a more sensible one that we are currently raising.

tkopecek commented 6 years ago

I've added check for archs and removed second one as it will fail on '%i' expansion it it is not a number.

rebased

6 years ago

mikem commented 6 years ago

That puts us back to the ugly sql error. Maybe something like this?

https://github.com/mikem23/koji-playground/commits/issue463

tkopecek commented 6 years ago

It add more db queries (it is also checked in CLI), but still more readable exception :-)

mikem commented 6 years ago

Also added a unit test

mikem commented 6 years ago

I don't think the extra queries are a problem for us, and they only come in when the option is specified. I guess we could eventually remove the check in the cli, once the hub change has been out for a while.

mikem commented 6 years ago

Commit e7dd4b3 fixes this pull-request