Skip to content

provider: three postgismvt InvalidParam calls pass an extra argument, producing %!(EXTRA ...) #1004

Description

@Michad

Three InvalidParam calls in the postgis mvt provider pass an extra argument, so the startup validation error an operator sees ends in %!(EXTRA ...).

InvalidParam has two verbs, pkg/config/config.go:509:

InvalidParam: "Invalid value supplied for parameter %v: %v",

internal/providers/postgis_mvt.go:91, :96 and :102 each pass three arguments:

return nil, fmt.Errorf(deps.ErrorMessages.InvalidParam, "postgismvt.gid", cfg.GID, columnRegex)
return nil, fmt.Errorf(deps.ErrorMessages.InvalidParam, "postgismvt.geometry", cfg.Geometry, columnRegex)
return nil, fmt.Errorf(deps.ErrorMessages.InvalidParam, "postgismvt.attributes."+strconv.Itoa(i), attribute, columnRegex)

so a bad column name produces:

Invalid value supplied for parameter postgismvt.gid: bad-name%!(EXTRA *regexp.Regexp=^[a-zA-Z0-9_]+$)

The useful part still comes through, it just has formatter noise stapled to the end. go vet's printf check doesn't catch it because the format string is indirected through a struct field rather than being a literal.

I checked every InvalidParam call site in the tree and these three are the only ones that don't pass exactly two arguments. Line 115 in the same file is correct:

return nil, fmt.Errorf(deps.ErrorMessages.InvalidParam, "postgismvt.datastore", cfg.Datastore)

Two ways to resolve it, and the choice is worth a moment's thought rather than defaulting to the smaller diff:

  • Drop columnRegex from the three calls. Smallest change, but the operator loses any statement of what the accepted format actually is, and "bad-name" isn't self-explanatory.
  • Keep the regex and use a message that has somewhere to put it. Telling the operator the allowed pattern is genuinely more useful for this particular error, since the constraint isn't guessable. That likely means a new entry in ErrorMessages rather than changing InvalidParam, which is shared by ~35 call sites and is part of the config schema operators can localize.

The second is better for the operator but adds a message key, which touches the error message documentation. Either way this is contained to the three lines.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions