Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

maint: set config value for ManyUUID limit #3381

Open
Forfold opened this issue Oct 27, 2023 · 4 comments
Open

maint: set config value for ManyUUID limit #3381

Forfold opened this issue Oct 27, 2023 · 4 comments
Labels
bug Something isn't working

Comments

@Forfold
Copy link
Contributor

Forfold commented Oct 27, 2023

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: #3231 (comment)

Screenshot 2023-10-27 at 9 52 43 AM
@Forfold Forfold added the bug Something isn't working label Oct 27, 2023
@ethan-haynes
Copy link
Collaborator

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
Copy link
Member

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
Copy link
Contributor Author

Forfold commented Oct 27, 2023

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
Copy link
Member

mastercactapus commented Oct 27, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants