Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(daemon): handle invalid notice types in filter #329

Merged
merged 3 commits into from
Nov 20, 2023

Conversation

olivercalder
Copy link
Member

@olivercalder olivercalder commented Nov 16, 2023

Invalid notice types are ignored when parsing the API request to construct the notice filter. However, if only invalid notice types are included in the request, discarding all types results in a filter indistinguishable from one which never had any types in the first place, and thus would return notices of any type. This commit fixes this behaviour, so no notices are returned if only invalid notice types are requested.

olivercalder and others added 2 commits November 16, 2023 13:35
Invalid notice types are ignored when parsing the API request to
construct the notice filter. However, if only invalid notice types are
included in the request, discarding all types results in a filter
indistinguishable from one which never had any types in the first place,
and thus should return notices of any type. This commit fixes this
behaviour, so no notices are returned if only invalid notice types are
requested.

Signed-off-by: Oliver Calder <[email protected]>
@benhoyt
Copy link
Contributor

benhoyt commented Nov 20, 2023

As you probably saw, we did make a conscious decision to ignore invalid notice types. The comment is:

			// Ignore invalid notice types (so requests from newer clients
			// with unknown types succeed).

But I just want to confirm what you have in mind.

When a client specifies say /v1/notices?types=custom&types=verynewtype, we want an older server to return just the notices with the type the server knows about, "custom". But if a client specifies /v1/notices?types=verynewtype&types=anothernewtype, neither of which the older server knows about, we want it to return no notices (as there of course won't be any) rather than all notices. Correct?

Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of very minor suggestions, and a question clarifying the intent.

internals/daemon/api_notices_test.go Outdated Show resolved Hide resolved
internals/daemon/api_notices.go Outdated Show resolved Hide resolved
@olivercalder
Copy link
Member Author

When a client specifies say /v1/notices?types=custom&types=verynewtype, we want an older server to return just the notices with the type the server knows about, "custom". But if a client specifies /v1/notices?types=verynewtype&types=anothernewtype, neither of which the older server knows about, we want it to return no notices (as there of course won't be any) rather than all notices. Correct?

Yes, that's what I intended. We want to prevent the default "all notices" behavior from kicking in if we prune all the unknown notice types and are left with no types remaining.

@benhoyt benhoyt merged commit f3a718c into canonical:master Nov 20, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants