filters: Reduce duplication of bool filter parsing and invalidFilter error by vvoland · Pull Request #44003 · moby/moby

vvoland · 2022-08-22T12:27:02Z

When working on images prune for containerd branch I noticed that the logic for parsing the filter value as boolean is duplicated in multiple places and each package has its own invalidFilter type which basically does the same thing.

- What I did

Removed all duplicated invalidFilter types
Replaced repeated pattern of parsing boolean filter with the usage of GetBoolOrDefault

- How to verify it
CI, lint, tests

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

api/types/filters/errors.go

thaJeztah · 2022-08-22T15:27:36Z

daemon/images/image_prune.go

+	err = pruneFilters.ParseBoolIfPresent(&danglingOnly, "dangling")
+	if err != nil {


Wondering if it would make for an easier "api" to make this something like filters.GetBool("name", <default>)

Thinking of which, we have a httputils.BoolValueOrDefault (although that's in the api/server package, so I haven't fully checked what the consequences for that are). I recall I was looking at some of the httputils package to make (some of) them return a proper error (which could be "ignored" by the caller where suitable).

I'm also not sure if that function follows the same semantics for "what's valid" (looks like we may be a bit inconsistent there)

api/types/filters/parse.go

thaJeztah · 2022-08-22T15:40:05Z

daemon/list.go

 		code, err := strconv.Atoi(value)
 		if err != nil {
-			return err
+			return filters.InvalidFilter{Filter: "exited", Value: value}


If possible, I think we should avoid exporting InvalidFilter, as it's mostly a "convenience" function to format an errdefs.InvalidParam(). Basically the "contact" is to return an errdef type, and that it happens to be "because of an invalid filter" is mostly an internal things for the filters package.

We could either decide to have a local helper, or inline for these two occurrences (return errdefs.InvalidParam(errors.Errorf("invalid filter ..."))

Or we could consider having a method to get Integer values (filters.GetInt(name, <default>)), like we added for booleans. Of course the ideal would've been for the filter values to be typed, but I guess that requires more work.

I like the filters.GetInt approach, but didn't want to bloat the filters API too much. This would allow us to completely unexport invalidFilter though. I'll give it a shot.

I decided not to implement the function for int parsing - there's only one usage for it (parsing stars filter in image search) and it handles multiple values by choosing the biggest. I'm not sure if this behaviour would be expected by future usages of GetIntOrDefault function, so I think it's better not to "abstract too early".

cpuguy83 · 2022-11-09T21:43:53Z

api/types/filters/parse.go

+		return nil
+	}
+
+	if len(fieldValues) == 0 {


Maybe instead we ca error if len(fieldValues) != 1 which would also simplifly the conflict logic.

Oh, I forgot about this PR, thanks for reminding me. I'll try to update it once I have some spare time!

IIRC I did this this way specifically to not change the old behaviour of filter.ExactMatch("dangling", "true") || filter.ExactMatch("dangling", "1") , which doesn't error when you specify the filter twice with different syntax (true/1 or false/0) but with the same value. This is only the case for image prune and volume service list/prune though.

If we don't need to keep this behaviour then we can go with len != 1.

cpuguy83 · 2022-11-09T21:45:25Z

api/types/filters/parse.go


+// ParseBoolIfPresent overwrites the target with a boolean value of the key if it's present.
+// If the value cannot be interpreted as a boolean an error is returned.
+func (args Args) ParseBoolIfPresent(target *bool, key string) error {


Have you thought about returning a bool instead of dealing with the bool pointer?

Yes - returning bool doesn't distinguish between value not being present and being false. In this case if value is missing, the target is not overwritten (leaves the previously assigned value) and error is nil.

I think we should go ...OrDefault approach suggested by Sebastiaan though.

cpuguy83 · 2022-11-09T21:46:51Z

api/types/filters/parse_test.go

+
+func TestParseBoolIfPresent(t *testing.T) {
+
+	for _, tC := range []struct {


Can you give all the test cases a name and run them in a subtest? It helps when something is failing what test case it's failing on.

vvoland · 2023-01-05T11:07:08Z

@cpuguy83 @thaJeztah PTAL

tianon

Generally LGTM 😄

tianon · 2023-01-24T23:29:39Z

api/types/filters/parse.go

+	conflicting := isFalse && isTrue
+	invalid := !isFalse && !isTrue
+
+	if conflicting || invalid {


This could all just be if isFalse == isTrue {, right?

Hmm, don't we also want to error out if we have any values that aren't one of the four supported strings? (or is that a breaking change from the previous implementation?)

Yes, that's the invalid check (true if value is none of [0,1,true,false]) and it ends with the error.
There are tests to prove that 😄 :

moby/api/types/filters/parse_test.go

Lines 451 to 468 in 18e30a0

{

name: "single bad value",

args: map[string][]string{

"dangling": {"potato"},

},

defValue: true,

expectedErr: invalidFilter{Filter: "dangling", Value: []string{"potato"}},

expectedValue: true,

},

{

name: "two bad values",

args: map[string][]string{

"dangling": {"banana", "potato"},

},

defValue: true,

expectedErr: invalidFilter{Filter: "dangling", Value: []string{"banana", "potato"}},

expectedValue: true,

},

Yes, this is logically the same as if isFalse == isTrue, but it looked confusing (looks like false == true).
I went with if conflicting || invalid instead as it felt clearer and explicit about when this branch is taken (when the filter values are conflicting, or are invalid).
But maybe that's just my perception 😅

tianon · 2023-01-24T23:30:30Z

api/types/filters/parse.go

+		return true, nil
+	}
+
+	return defaultValue, nil


This should never be reached, right? Should this return some kind of error that it's an unexpected code path so we can chase it down easier if it ever somehow happens? 😅

Changed to an error. I was tempted to just panic since it's not possible to reach this, but.. better safe than sorry I guess 😄

thaJeztah · 2023-01-26T12:56:06Z

api/types/filters/parse.go

+		return true, nil
+	}
+
+	return defaultValue, fmt.Errorf("unexpected code path reached for values=%s", args.Get(key))


As we used an errdef for the other error, ideally this one would be an errdefs.ErrSystem. It's the default (if no error-type is defined), but will also be logged otherwise as "FIXME: didn't have a error-type for error.."

I wanted to avoid importing errdefs (it's not imported in this file, the invalidFilter just implements the InvalidParameter function, and thought that because this shouldn't happen anyway, then the "FIXME" message would at least be visible in the logs if it somehow ended up taking this code path 😄
I can add a helper struct that implements the errdefs.ErrSystem if you feel it would be better though.

Ah, right, yes. So ideally implementing a type for it, although it looks like errdefs is already imported for other parts, so probably not a huge deal to use it.

I'm fine with looking at that separately though; not a blocker.

Ah, it seems that errdefs somehow made it's way to the parse_test.go. Looking at the change that caused is makes me think this was accidental.
I dropped the errdefs dependency completely and changed the error to error-type.

thaJeztah · 2023-01-26T12:58:36Z

api/types/filters/parse.go

+// GetBoolOrDefault returns a boolean value of the key if the key is present
+// and is intepretable as a boolean value. Otherwise the default value is returned.
+// Error is not nil only if the filter values are not valid boolean or are conflicting.
+func (args Args) GetBoolOrDefault(key string, defaultValue bool) (bool, error) {


I wish Golang had a better option for variadic arguments, as then we could've made the defaultValue optional (I guess we could do bool... and check "maximum 1", but that'd be a bit of an ugly hack).

Bit of a bike-shed/nit, but perhaps GetBool(key, defaultValue) would work? It's shorter, and from the list of arguments I don't think it's really impacting readability / intent; WDYT?

Actually I think the OrDefault suffix makes it a bit more clearer what the second boolean argument is.
Generally boolean arguments make the code harder to understand from the invocation alone.

Consider:

isTask, err := psFilters.GetBoolOrDefault("is-task", false)

vs

isTask, err := psFilters.GetBool("is-task", false)

The first one makes it a lot more apparent that the false is the default. In the second case reader might not be sure what the false is - is it default, is it some setting that influences how it parsed?

Writing the extra OrDefault is mostly mitigated by the autocompletion, but when reading you don't necessarily see the argument names.

Of course it's highly subjective and maybe others view this differently, so I'm happy to hear other opinions on this one 😃

thaJeztah · 2023-01-26T13:02:27Z

api/types/filters/errors.go

+	if e.Filter != "" {
+		msg += " '" + e.Filter
+		if e.Value != nil {
+			msg += fmt.Sprintf("=%s", e.Value)


We're already concatenating, and Sprintf() doesn't add much here, so perhaps just;

Suggested change

msg += fmt.Sprintf("=%s", e.Value)

msg += "=" + e.Value

~~Ah, good one, thanks!~~
Ugh, not this time! 😂 Value is []string so it can't be concatenated with string:
invalid operation: "=" + e.Value (mismatched types string and []string)

🤦 doh! I hate the filter implementation.

Hmm, I'm thinking that this could be changed to msg = fmt.Sprintf("%s=%s", msg, e.Value) - if we already do the Sprintf, let it handle the concatenation too.

I think it was done because the value is "optionally there". Current code seems fine from that perspective.

thaJeztah · 2023-01-26T13:08:35Z

daemon/list.go

 		code, err := strconv.Atoi(value)
 		if err != nil {
-			return err
+			return errdefs.InvalidParameter(errors.Wrapf(err, "invalid filter 'exited=%s'", value))


Looks like a potential opportunity to also have a filters.GetInt(key, defaultValue)

I considered GetInt but decided not to implement it because at the time I thought it's only used for stars (didn't notice the exited 😄) which made it a bit awkward to consider how multiple integer values should be handled and generalize solution from one usage.

So, to reevaluate this one:
Currently we seem to have two usages for parsing filter as integer and both usages have different approach in how they handle multiple values.

exited - uses all values

stars - chooses the largest value (

moby/daemon/images/image_search.go

Line 49 in e040a9d

if iHasStar > hasStarFilter {

)

To handle both those cases, we would only be able to have something like GetInts(key string, default []int) ([]int, error) which would just make sure that all values for the given filter are valid integers and would return a parsed []int, or a default if the filter is missing.

This would simplify the exited case, because it would reduce this code to

filtExited, err := psFilter.GetInts("exited", nil) if err != nil { ...

And for stars this would still require to loop through the result, but now the int parsing would be gone, and it would focus only on the if iHasStar > hasStarFilter { hasStarFilter = iHasStar } part.

It's still a win I think. WDYT?

Perhaps that may work (definitely not for this PR though!). Will have to give it another (closer) look to see what everything would look like. The "multiple values" (and in many cases rather ambiguous) is rather awkward with the filters.

thaJeztah

LGTM

unless anyone else has thoughts on

Of course it's highly subjective and maybe others view this differently, so I'm happy to hear other opinions on this one 😃

(GetBoolOrDefault(key, defaultValue) vs GetBool(key, defaultValue))

thaJeztah · 2023-01-26T15:14:50Z

@cpuguy83 ptal

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

The pattern of parsing bool was repeated across multiple files and caused the duplication of the invalidFilter error helper. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland requested a review from cpuguy83 as a code owner August 22, 2022 12:27

vvoland force-pushed the invalidfilter branch 2 times, most recently from 0d18ae0 to 39469a0 Compare August 22, 2022 15:09

thaJeztah reviewed Aug 22, 2022

View reviewed changes

thaJeztah added area/api API status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Aug 22, 2022

cpuguy83 requested changes Nov 9, 2022

View reviewed changes

vvoland force-pushed the invalidfilter branch 3 times, most recently from 891cd6d to abc9923 Compare November 21, 2022 08:08

vvoland force-pushed the invalidfilter branch 2 times, most recently from a26705c to 18e30a0 Compare January 5, 2023 08:17

vvoland mentioned this pull request Jan 23, 2023

c8d/list: Support dangling image filter rumpl/moby#128

Merged

tianon approved these changes Jan 24, 2023

View reviewed changes

vvoland force-pushed the invalidfilter branch from 18e30a0 to e040a9d Compare January 25, 2023 10:56

tianon approved these changes Jan 25, 2023

View reviewed changes

thaJeztah reviewed Jan 26, 2023

View reviewed changes

thaJeztah added this to the v-next milestone Jan 26, 2023

thaJeztah approved these changes Jan 26, 2023

View reviewed changes

vvoland added 2 commits January 26, 2023 16:28

api/types/filters: Add GetBoolOrDefault

0d68591

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

Use GetBoolOrDefault to remove duplicated invalidFilter usages

a654cbf

The pattern of parsing bool was repeated across multiple files and caused the duplication of the invalidFilter error helper. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland force-pushed the invalidfilter branch from e040a9d to a654cbf Compare January 26, 2023 15:29

vvoland requested a review from cpuguy83 February 21, 2023 12:19

cpuguy83 approved these changes Feb 22, 2023

View reviewed changes

cpuguy83 merged commit 73db49f into moby:master Feb 22, 2023

This was referenced Jul 28, 2023

api/types/filters: fix errors not being matched by errors.Is() #46106

Merged

[24.0 backport] api/types/filters: fix errors not being matched by errors.Is() #46115

Merged

		err = pruneFilters.ParseBoolIfPresent(&danglingOnly, "dangling")
		if err != nil {


		func TestParseBoolIfPresent(t *testing.T) {

		for _, tC := range []struct {

	{
	name: "single bad value",
	args: map[string][]string{
	"dangling": {"potato"},
	},
	defValue: true,
	expectedErr: invalidFilter{Filter: "dangling", Value: []string{"potato"}},
	expectedValue: true,
	},
	{
	name: "two bad values",
	args: map[string][]string{
	"dangling": {"banana", "potato"},
	},
	defValue: true,
	expectedErr: invalidFilter{Filter: "dangling", Value: []string{"banana", "potato"}},
	expectedValue: true,
	},

Conversation

vvoland commented Aug 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvoland commented Jan 5, 2023

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvoland Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvoland Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jan 26, 2023

Uh oh!

vvoland commented Aug 22, 2022 •

edited

Loading

vvoland Jan 26, 2023 •

edited

Loading

vvoland Jan 26, 2023 •

edited

Loading