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

fix: #704 max-depth not optional. #755

Merged
merged 2 commits into from
Oct 7, 2021

Conversation

CypherpunkSamurai
Copy link
Contributor

@CypherpunkSamurai CypherpunkSamurai commented Oct 5, 2021

Fix #704

max-depth not working as optional as mentioned in docs.

Related issue(s)

#704

Checklist

Further Comments

This patch fixes issue #704 where max-depth parameter appears to be causing http errors (HTTP 400).

This patch reads the query parameter max-depth into variable d_ and checks if query parameter was not provided before throwing http error.

I hope you will review my PR soon, and accept it for Hacktoberfest. Happy Hacktoberfest :)

@CLAassistant
Copy link

CLAassistant commented Oct 5, 2021

CLA assistant check
All committers have signed the CLA.

@CypherpunkSamurai CypherpunkSamurai changed the title Fix #704 max-depth not optional. fix: #704 max-depth not optional. Oct 5, 2021
internal/expand/handler.go Outdated Show resolved Hide resolved
@zepatrik
Copy link
Member

zepatrik commented Oct 5, 2021

Further, please add a test that checks whether the right code is returned.
To actually make the parameter required, please add a // required: true comment here:

// in:query
MaxDepth int `json:"max-depth"`

You will find other definitions that do that, so please look up the right way to do this.
The parameter is not marked as required in the docs (anymore?): https://www.ory.sh/keto/docs/reference/rest-api/#operation/getExpand

@CypherpunkSamurai
Copy link
Contributor Author

CypherpunkSamurai commented Oct 5, 2021

Yes, sorry missed that. Thinking about it a second time, it probably makes more sense to make it actually required as I can't come up with a sane default value that makes sense for most use-cases.

As you're planning on making it required I'll make necessary changes as you suggested. This would make things more clearer :)

Further, please add a test that checks whether the right code is returned.

Ok. I'm new to test driven development area. I will try.

Also, side note, should I change the documentation files too to reflect the changes?

Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking pretty good already 👍
Did you find any docs pages that need to be updated except for the API reference? That one is automatically rendered from the openapi schema, which is autogenerated from the code and comments. So should be enough to just adjust the annotations.

docs/docs/.static/api.json Outdated Show resolved Hide resolved
internal/expand/handler.go Outdated Show resolved Hide resolved
internal/expand/handler.go Outdated Show resolved Hide resolved
internal/expand/handler_test.go Outdated Show resolved Hide resolved
@CypherpunkSamurai
Copy link
Contributor Author

CypherpunkSamurai commented Oct 6, 2021

I found a new problem after adding the patch to check max-depth. Is the function supposed to use max-depth as a *int64 rather than a int64?

image

Update:

Editing out pointerx.int64 seems to have fixed it. Is this ok?

image

internal/expand/handler_test.go Outdated Show resolved Hide resolved
internal/expand/handler.go Outdated Show resolved Hide resolved
internal/e2e/sdk_client_test.go Show resolved Hide resolved
@CypherpunkSamurai
Copy link
Contributor Author

CypherpunkSamurai commented Oct 6, 2021

Hm.

I'll squash those commits into one single commit to not mess up your codebase :)

Fix ory#704  is not an optional parameter. Added error handlers,(handler.go#L80) docs and tests(ehandler_tests.go#L38). Fix sdkClient maxDepth pointer. Rebased Commit history.
@CypherpunkSamurai
Copy link
Contributor Author

CypherpunkSamurai commented Oct 6, 2021

I've rebased into one single commit. Tests should work now :)

I'm sorry for turning the git commits. I'm new to PRs thank you for your patient with me :)

Quote test strings to match errors
@CypherpunkSamurai
Copy link
Contributor Author

Ok. All checks seems to have passed. Thank You for being patient with me. I'm new to PRs.

I'm still learning please excuse my commits. I'm very sorry for taking so much time.

I had a question and I feel like you're a great developer who can answer. How does one test their code offline before making PR? Everytime I push I see the CI takes over and runs the checks for me. Is it possible to run tests offline?

@zepatrik
Copy link
Member

zepatrik commented Oct 7, 2021

Don't worry, I will squash-merge anyways 😉

Sure, you can run tests locally. We even have a section about that in the README 😂 https://github.com/ory/keto#running-tests
Long story short, the CI basically runns go test -tags sqlite ./... and some other checks like golangci-lint run.

Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks perfect now, thank you 🎉

@zepatrik zepatrik merged commit 6d51422 into ory:master Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keto expand REST API does not parse query parameter max-depth correctly
3 participants