target / goalert

Open source on-call scheduling, automated escalations, and notifications so you never miss a critical alert
https://goalert.me
Apache License 2.0
2.17k stars 230 forks source link

maint: set config value for ManyUUID limit #3381

Open Forfold opened 8 months ago

Forfold commented 8 months ago

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
ethan-haynes commented 8 months ago

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.

mastercactapus commented 8 months ago

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.

Forfold commented 8 months ago

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

mastercactapus commented 8 months ago

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).