goalert icon indicating copy to clipboard operation
goalert copied to clipboard

maint: set config value for ManyUUID limit

Open Forfold opened this issue 1 year ago • 4 comments

Most of our calls to validate.ManyUUID use a "magic number" of either 50, 100, or 200. It might be good to start a conversation around standardizing these values as global config variable.

Reference: https://github.com/target/goalert/pull/3231#discussion_r1373493755

Screenshot 2023-10-27 at 9 52 43 AM

Forfold avatar Oct 27 '23 14:10 Forfold

Looks like the only one that has a set pattern is search.MaxResults with the struct:

package search

// Shared configuration for search methods.
const (
	MaxQueryLen       = 255
	MaxResults        = 150
	DefaultMaxResults = 15
)

Maybe we follow that pattern going forward so that there is a single source of truth.

ethan-haynes avatar Oct 27 '23 15:10 ethan-haynes

Yeah, this is a good idea. At the very least, the reason the values are what they are can be documented easily that way.

I agree with @ethan-haynes on following the search package pattern; constant/fixed values will be easier to convey for API use since they are essential for limiting payload size/DB load/memory usage for a single request (and to keep our guarantees around things like response time).

Configurable limits are for things like data size-at-rest where you want to limit things like open alerts so the DB doesn't grow until it tips over if something goes wrong, but some installations may require them to be increased or decreased.

mastercactapus avatar Oct 27 '23 15:10 mastercactapus

By configurable, I basically meant the same as what Ethan proposed, so it works out 🙂

i.e. a higher level place to set the value for re-use in many areas

Forfold avatar Oct 27 '23 15:10 Forfold

Maybe part of the validation package?

That way the code could read like:

validate.ManyUUID("Omit", ids, validate.MaxOmit)

Alternatively, we could look into moving from UUID-strings to actual UUIDs (move the parsing to the generated GraphQL code) -- then we could move the length-limit to the schema via directive potentially, which could look like this:

input ServiceSearchOptions {
  first: Int = 15
  after: String = ""
  search: String = ""
  omit: [ID!] @max(length: 50)

Which would remove the need for ManyUUID in almost if not all places.

The latter I don't think could use a constant value, but it would be all in one place, discoverable, and scannable (i.e., you could search for omit: and validate they all have the same value).

mastercactapus avatar Oct 27 '23 15:10 mastercactapus