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

Multi-Root #6

Merged
merged 4 commits into from Aug 23, 2017
Merged

Multi-Root #6

merged 4 commits into from Aug 23, 2017

Conversation

JaySunSyn
Copy link
Contributor

Patrik Kullman and others added 4 commits August 21, 2017 22:58
- make the tree a little bit more tricky
- correct existing "mat"-tests
- add some more scenarios

Signed-off-by: Patrik Kullman <patrik.kullman@neovici.se>
- old method renamed _getPathNodes
-- fix for nodes on path logic
- make sure we try to find to path on all root nodes
-- the path with the least amount of 'undefined' wins
--- should really be 0 unless something is badly configured
---- all undefined from the beginning of the path is excluded to
     account for missing ancestors

Signed-off-by: Patrik Kullman <patrik.kullman@neovici.se>
- Adjusted sort of getPathNodes
- Return tree if !pathLocator
- added node to test tree: The response object of iron-ajax gets sorted (I guess not guaranteed) by its object keys. Therefore it was hard to let the test fail if same amount of undefined values but not equal amount of defined values. The test case took randomly the right node.
@JaySunSyn JaySunSyn mentioned this pull request Aug 22, 2017
@nomego
Copy link
Contributor

nomego commented Aug 22, 2017

Yes and the same conditions are still present.

The biggest change is that we now support pathLocators that address ancestors that we don't have access to. And then we had a few discussions on other cases with other types of trees for wider support.

A lot of our discussions are spread out in PR 1, but I think the biggest one on the subject is #1 (comment) .

I'm not sure if it's there or somewhere else but we discussed this point and I do agree that for our purposes, we can simplify the logic by saying "it's fine to lack parts of that pathLocator in the beginning, but invalidate any path that can't find a needed path part in the middle or end".
What we're doing now is basically saying "if we can't find any valid path on any node, should we return the best candidate we could find?"
I actually think we shouldn't, just return undefined/null since "best candidate" can be anything, but it's still not right. This would simplify _getPathNodes a bit and enable some performance improvements.

What I'm not sure about though is if I request the pathNodes for 1.2.3.4.5 and only have permission from 3 (recursively), should we return an array with 5 elements, first 2 undefined, or just the last three that we actually had permissions too ?
I would expect to receive as many elements back as I had parts in my pathLocator, but on the other hand that just puts additional error-checking on the caller, likely every time.

Thoughts @plequang @JaySunSyn ?

@nomego
Copy link
Contributor

nomego commented Aug 22, 2017

Sorry, yes your implementation also supports missing ancestors but adds a dependency on the "pathLocator" property.

@plequang
Copy link
Contributor

adds a dependency on the "pathLocator" property.
I don't understand. The getPathNodes function modified by the PR takes pathLocator as an argument.

Also I keep thinking that :

the function that returns a path should return an array of nodes that start at one of the root, and without any undefined.

If no such path exists, this means you don't have access.

I think you over complicated the test cases in missing ancestor tree tests:

  1. The id used as the key for path locator is unique. So id "301" can appear only below only one path.
  2. The test for path 1.2.7.8 should return null, as that node does not exists/not visible. There is no point in returning the beginning of the path.

What I'm not sure about though is if I request the pathNodes for 1.2.3.4.5 and only have permission from 3 (recursively), should we return an array with 5 elements, first 2 undefined

I think it's useless, we won't display anything regarding the 2 undefined nodes. 3 will appear as if it was a root. Think of network shares, or the "shared with me" folder in Google drive.

@plequang
Copy link
Contributor

Anyway, I made some tests and this implementation works in real situation scenario.
Will merge it and we can take this discussion later on.

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.

None yet

3 participants