-
-
Notifications
You must be signed in to change notification settings - Fork 341
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
feat: add max-depth for check command + global max-depth #791
Conversation
da97aaa
to
600856c
Compare
600856c
to
ac543a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks very good already 👍
I only have a view small comments, but no hurry 😉
Regarding your open questions, I agree with you that returning an error is not the right way to go. Instead, only the branch should be aborted. We could at some point add a boolean to the response that indicates whether not all branches were fully expanded, but I am not sure that there is a good use-case for that.
Except maybe of debugging, but for that you can add a log statement instead of returning an error.
Also, I think the max-depth
parameter should not be required, but instead optional. With #703 we will have a configurable fallback that will be used if no value is given.
Thanks again for looking into this.
b3a76e3
to
307a9e3
Compare
ad1b533
to
fc40494
Compare
@zepatrik this should be good for re-review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for all the improvements 👍
There seem to be some not cleaned up test files, I am not sure why sqlite is sometimes keeping them...
- internal/persistence/sql/TestRelationTupleSubjectTypeCheck
- internal/persistence/sql/migrations/TestToSingleTableMigrator_HasLegacyTable
- internal/persistence/sql/migrations/migratest/TestMigrations
0d68502
to
6afb2b4
Compare
max-depth for the check command limits the depth of the search, a safeguard against particularly expensive queries. This allows users more fine-grain control.
Make max-depth optional for check API. Add global max-depth, for which request-level only takes precedence if it is greater than 0 and less than the global-max depth.
632eea4
to
f9e73e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much, this looks perfect now 👍
@@ -56,6 +64,7 @@ func newCheckCmd() *cobra.Command { | |||
|
|||
client.RegisterRemoteURLFlags(cmd.Flags()) | |||
cmdx.RegisterFormatFlags(cmd.Flags()) | |||
cmd.Flags().Int32P(FlagMaxDepth, "d", 0, "Maximum depth of the search tree. If the value is less than 1 or greater than the global max-depth then the global max-depth will be used instead.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this 👍
type deps struct { | ||
*relationtuple.ManagerWrapper // managerProvider | ||
configProvider | ||
loggerProvider | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, now I get what you mean. I think the previous approach was fine as well, as the manager wrapper type is only a helper for testing, but this is of course the best way to do it 👍
Ah, the only missing thing now is the CLA 😅 |
5f964d2
to
07cb3ae
Compare
@zepatrik CLA done |
Thank you, CCI is somehow not running... I am looking into it. No idea why it does not like your fork, pushed now the same branch on this original repo: https://app.circleci.com/pipelines/github/ory/keto/1693/workflows/b840a908-0587-42e5-be87-51ba4a918573 |
@zepatrik It looks like the commit |
Yeah, for some reason the lock file resulted in a broken state... should be fixed now though |
Thank you very much, greatly appreciate your effort 👍 |
max-depth for the check command limits the depth of the search, a safeguard against particularly expensive queries. This allows users more fine-grain control.
At present the approach in this PR is to
checkOneIndirectionFurther
, the search along the given branch halts. Check returning false means a path to a relation is has not been found up to the max-depth.serve.read.max-depth
key in the config json. This global max-depth takes precedence if the request-level max-depth is less than 1 or if the global max-depth is less than the request-level max depth.Related issue(s)
closes #703
closes #702
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further Comments
Notes for self
This was straightforward and so I did so via Github Codespaces. The Makefile shows me how to do most common development activities. I was able to run the cat video example with a local server.
checkOneIndirectionFurther
is the method that is called recursively. NOTE: The search forketo check "*" view videos /cats/1.mp4
wastes diving 2 units of depth while the answer is present at 0 depth.checkOneIndirectionFurther
and error when maxDepth is spent