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

Relationship Distribution gives inconsistent results when changing the limit parameter #6319

Closed
labo-flg opened this issue Mar 12, 2024 · 5 comments · Fixed by #6323
Closed
Assignees
Labels
bug use for describing something not working as expected solved use to identify issue that has been solved (must be linked to the solving PR)

Comments

@labo-flg
Copy link
Member

labo-flg commented Mar 12, 2024

Description

On this example dashboard:
https://testing.octi.staging.filigran.io/dashboard/workspaces/dashboards/d4374a78-de09-4a00-a5c3-37b673bcea8b

The 2 widgets are the same (second one duplicated from the first), I only changed the "limit" parameter of the second one (from 10 to 20).

One widget gives 0 results, the other gives 2 results.

This bug will not happen if the user has BYPASS capabilty, so test with a user with only access to knowledge + dashboards.

I also tested with a user with ALL capabilities but not BYPASS, the bug happens. So probably a part of the code excluded when user has BYPASS.

Environment

Testing environment 6.0

See user/role/dashboard with mention "6319"

Screenshots (optional)

image

@labo-flg labo-flg added bug use for describing something not working as expected needs triage use to identify issue needing triage from Filigran Product team labels Mar 12, 2024
@labo-flg
Copy link
Member Author

Results of our initial debug session with @marieflorescontact :

  • distributionRelations calls elAggregationRelationsCount which gives the 100 first results of this aggregation
  • then convertAggregateDistributions take the N first items (N being the limit variable, so 10 or 20 here)
  • then convertAggregateDistributions calls elFindByIds to resolve these N first objects and to return them

Problem: elFindByIds naturally checks the data restriction thanks to buildDataRestrictions. With BYPASS there is no restrictions, but once you have a regular user this will have an impact : in the end only few ids are actually resolved. Upon the 10 first you might have luck and get a few... Greater the value N, bigger the chance. Ultimately we cannot know for sure the results.

Could we include the data restrictions into the aggregation query directly ?
cc @SamuelHassine WDYT ?

@Jipegien Jipegien added critical use to identify critical bug to fix ASAP and removed needs triage use to identify issue needing triage from Filigran Product team labels Mar 12, 2024
@Jipegien Jipegien added this to the Release 6.0.6 milestone Mar 12, 2024
@labo-flg labo-flg self-assigned this Mar 12, 2024
@labo-flg
Copy link
Member Author

labo-flg commented Mar 12, 2024

Update:
elQueryBodyBuilder includes data restrictions, and it's injected in the "query" part of the aggregation query... so it should work properly I guess. But I keep getting objects I am not able to later resolve with elFindByIds.

EDIT: the injection concerns the relationships itself, not the objects from/to this relationship so it's normal.
We cannot inject data restriction filter on these objects on this aggregation over relationship indices.

@labo-flg
Copy link
Member Author

Shall we increase the MAX_AGGREGATION_SIZE to give us more room? I'm unsure of the perf cost here (cc @richard-julien).

@Kedae Kedae modified the milestones: Release 6.0.6, Release 6.0.7 Mar 13, 2024
@Kedae Kedae removed the critical use to identify critical bug to fix ASAP label Mar 13, 2024
@labo-flg
Copy link
Member Author

After some discussions, we agreed on the following solution:

  • the relationship distribution shall be the same in both cases, it represents a state of the database and not a state of a subset of the data depending on user's rights. Dashboards are meant to be shared, so they shall not be inconsistent
  • the items that are not accessible shall be presented as "restricted"

On the technical side it means changing the API of relationshipDistribution to return not an entity but just enough for the display, an unified API with the id, entity_type and representative.

Frontend side, this will simplify greatly the fragment (no more ... on XXX for every possible entity in the model) and removes the need for using the magic function defaultValue(node)

labo-flg added a commit that referenced this issue Mar 26, 2024
labo-flg added a commit that referenced this issue Mar 27, 2024
labo-flg added a commit that referenced this issue Apr 2, 2024
@SamuelHassine SamuelHassine modified the milestones: Release 6.0.9, Release 6.0.10 Apr 3, 2024
@labo-flg
Copy link
Member Author

labo-flg commented Apr 4, 2024

Here is how it looks with #6323

image

@SamuelHassine SamuelHassine added the solved use to identify issue that has been solved (must be linked to the solving PR) label Apr 4, 2024
@SamuelHassine SamuelHassine modified the milestones: Release 6.0.10, Release 6.1.0 Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug use for describing something not working as expected solved use to identify issue that has been solved (must be linked to the solving PR)
Projects
None yet
4 participants