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

Add max-depth to global config #703

Closed
danthegoodman1 opened this issue Sep 9, 2021 · 5 comments · Fixed by #791
Closed

Add max-depth to global config #703

danthegoodman1 opened this issue Sep 9, 2021 · 5 comments · Fixed by #791
Labels
feat New feature or request. good first issue A good issue to tackle when being a novice to the project. help wanted We are looking for help on this one.

Comments

@danthegoodman1
Copy link

Is your feature request related to a problem? Please describe.

It would be nice for multi-tenant environmentss to be able to specify a global max-depth such that requests would not be able to cause high latency and demanding DB queries.

Describe the solution you'd like

Adding max-depth as a something the max-depth query param cannot exceed.

Describe alternatives you've considered

None

Additional context

@zepatrik
Copy link
Member

Makes sense 👍

@zepatrik zepatrik added feat New feature or request. good first issue A good issue to tackle when being a novice to the project. help wanted We are looking for help on this one. labels Sep 13, 2021
@doodlesbykumbi
Copy link
Contributor

Trying to confirm I understand what is meant by

max-depth to global config

Does this mean setting max-depth in the configuration passed to serve, such that the effective max-depth for any given request is the lesser one between the global max-depth vs the request max-depth ?

@danthegoodman1
Copy link
Author

Trying to confirm I understand what is meant by

max-depth to global config

Does this mean setting max-depth in the configuration passed to serve, such that the effective max-depth for any given request is the lesser one between the global max-depth vs the request max-depth ?

@doodlesbykumbi yep!

@doodlesbykumbi
Copy link
Contributor

doodlesbykumbi commented Dec 5, 2021

Thanks @danthegoodman1. I follow the gist of this feature request and the motivation. I think the implementation should be straightforward. I wonder though if the idea of "depth" will be semantically consistent across all the things that will make use of this global value. It definitely is consistent between check and expand, just not sure if that will generally be the case. Never-mind, I'm fairly certain it'll be consistent.

@doodlesbykumbi
Copy link
Contributor

doodlesbykumbi commented Dec 5, 2021

So here's my proposal for an implementation

  1. Add a serve.read.max-depth key to the config schema. Something like:
{
  "max-depth": {
    "type": "integer",
    "default": 1000,
    "title": "Maximum depth",
    "description": "The maximum depth on all read operations.",
    "minimum": 0,
    "maximum": 65535
  }
}
  1. Expose the value in the Config provider through something like
KeyReadMaxDepth = "serve.read.max-depth"
func (k *Config) ReadAPIMaxDepth() int {
	return k.p.Int(KeyReadMaxDepth)
}
  1. Consume the value via dependency injection in expand and check (for both gRPC and HTTP):
	// The lesser of the global max-depth and the request max-depth
	maxDepth := h.d.Config().ReadAPIMaxDepth()
	if int(req.MaxDepth) < maxDepth {
		maxDepth = int(req.MaxDepth)
	}
  1. Add unit tests to confirm that the lesser value takes precedence (for both gRPC and HTTP).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request. good first issue A good issue to tackle when being a novice to the project. help wanted We are looking for help on this one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants