goalert
goalert copied to clipboard
maint: set config value for ManyUUID limit
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
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.
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.
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
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).