-
Notifications
You must be signed in to change notification settings - Fork 4
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: Return fast before traverse for wrong ids #221
Conversation
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.
Tested this locally, approved from my end.
Another part that is important and not documented: the root object's closure's will always contain all children ids - as such, this is also part an optimisation to not check for any subsequent child's closures (I might've gotten this part wrong, cc @oguzhankoral )
This PR is conceptual fix, I will let @adamhathcock to refactor if he wishes. What I realized on debug, we were trying to traverse every ids in the closure of every object, which we didn't have to, root objects' closures are enough to get what we need. I felt a bit we do a lot of unnecessary work |
Needs to atleast be a hashset, ideally frozen as it's used a lot, but probably not copied at all. I'll fix it soon |
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.
@adamhathcock and I are confident that we have solved this problem already on send.
Old speckle commits may still fail, but I feel strongly that we add bloat to our deserialize process inorder to support a bugs in a previous beta that are already fixed.
Unless we can reproduce this problem on new commits, I'm tempted to say we shouldn't accept this.
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.
Disregard previous, after a quick chat with Oguzhan, existing projects that have datachunks with closures are still a problem for subsequent sends.
Lets follow Adam's suggestion to use a Frozen set, otherwise this is a good change.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #221 +/- ##
===========================================
+ Coverage 59.40% 70.64% +11.23%
===========================================
Files 262 278 +16
Lines 10408 11128 +720
Branches 1102 1104 +2
===========================================
+ Hits 6183 7861 +1678
+ Misses 3965 2912 -1053
- Partials 260 355 +95 ☔ View full report in Codecov by Sentry. |
Fixes
Object blabla not found in SQLite cache
error. Some projects are poisoned with bad serialization before and we prevent it with this fix. Actually we do not care closures except root object, if an id is not in root object closures, it smells very badly.