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: adding functionality to handle cycles in expand and check engine #623

Merged
merged 5 commits into from
Jun 16, 2021

Conversation

aravindarc
Copy link
Contributor

the cycle checking functionality will stop traversing along that particular path if a cycle is detected

Related issue #411 @zepatrik

Proposed changes

Currently there is no mechanism to break a traversal, when a cycle occurs. This leads to indefinite resolution.
We don't have to implement any additional traversal algorithm, we can check for cycles during the existing traversals in the check and expand engines. If a cycle is found the traversal along that particular path will be stopped.

Checklist

  • I have read the contributing guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

the cycle checking functionality will stop traversing along that particular path if a cycle  is detected
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.

Nice, this looks solid. I have some ideas how you can further improve it.

internal/utils/graph_utils.go Outdated Show resolved Hide resolved
internal/utils/graph_utils.go Outdated Show resolved Hide resolved
internal/utils/graph_utils.go Outdated Show resolved Hide resolved
internal/utils/graph_utils.go Outdated Show resolved Hide resolved
internal/check/engine.go Outdated Show resolved Hide resolved
internal/utils/graph_utils.go Outdated Show resolved Hide resolved
moving graph_utils.go and graph_utils_test.go to internal/x/graph and more changes suggested in the review
@aravindarc aravindarc requested a review from zepatrik June 16, 2021 15:31
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.

Nice, thank you very much 🎉

Children: []*expand.Tree{
{
Type: expand.Leaf,
Subject: sendlingerTorSS,
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if that was on purpose, but I like that we have can see the circle still in the tree. It makes it possible for the client to see circular references as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it was on purpose, or it will be confusing for the client and might be flagged as an unexpected behaviour.

@zepatrik zepatrik merged commit 8e30119 into ory:master Jun 16, 2021
@aravindarc
Copy link
Contributor Author

Nice, thank you very much

Awesome, thanks for the quick review and support, looking for forward to contribute more

@aravindarc aravindarc deleted the 411-add-cycle-handling branch June 16, 2021 20:40
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.

2 participants