Issue #250: Implements Bidirectional RelationshipResolver #256
Conversation
| - (NSInteger)countPendingRelationshipsWithSourceKey:(NSString *)sourceKey | ||
| andTargetKey:(NSString *)targetKey; | ||
|
|
||
| - (BOOL)verifyBidireccionalMappingBetweenKey:(NSString *)sourceKey |
There was a problem hiding this comment.
Small typo: "bidirectional"
| NSDictionary *metadata = [storage metadata]; | ||
|
|
||
| // If there's already nothing there, save some CPU by not writing anything | ||
| if ( self.pendingRelationships.count == 0 && metadata[SPRelationshipsPendingsNewKey] == nil ) { |
There was a problem hiding this comment.
Formatting consistency (spaces in the condition).
| block(); | ||
| } else { | ||
| dispatch_async(dispatch_get_main_queue(), block); | ||
| } |
There was a problem hiding this comment.
As a potential consumer of this API, this pattern seems unexpected. Any way to make it clearer?
Is this method actually being called from both the main thread and other threads? If called from the main thread, does it have to be executed synchronously like that, or could it still be dispatched asynchronously?
If it can be changed to always execute asynchronously, the API could be changed to be more clear (e.g. by adding an optional completion handler and documenting it).
There was a problem hiding this comment.
Save OP doesn't need to be synchronous. But access to the pendingRelationships map is performed, always, on the main thread. To make it super clear, we could move the dispatch_async call to the caller instead. What do you think?
There was a problem hiding this comment.
I think it tends to be clearer to do it that way (let the caller decide), or at least pick one way or the other and make it clear how it behaves in the method definition/documentation.
There was a problem hiding this comment.
@mikejohnstn in that case, would it be okay with you if we move the 'Relationship Resolver' queue to SPBucket, right by 'processorQueue' property, for consistency sake?
There was a problem hiding this comment.
Sure that sounds reasonable.
There was a problem hiding this comment.
Thanks for the feedback Mike, much appreciated!
| // Verify | ||
| XCTAssert( [self.resolver countPendingRelationships] == SPTestIterations, @"Inconsistency found" ); | ||
|
|
||
| for (NSInteger i = 0; ++i < SPTestIterations; ) { |
There was a problem hiding this comment.
Should be <= here?
For all the places where you're doing this, I think the more standard for (NSInteger i=0; i < SPTestIterations; ++i) is clearer (or i++).
|
@CapoChino may i ask you if you've been able to verify if the relationships bug is still present on this branch?. Thanks! |
| - (id)defaultValue { | ||
| return nil; | ||
| } | ||
|
|
||
| - (id)simperiumKeyForObject:(id)value { | ||
| NSString *simperiumKey = [value simperiumKey]; | ||
| return simperiumKey == nil ? @"" : simperiumKey; | ||
| return simperiumKey == nil ? @"" : simperiumKey; | ||
| } | ||
|
|
||
| - (SPManagedObject *)objectForKey:(NSString *)key context:(NSManagedObjectContext *)context { | ||
| // TODO: could possibly just request a fault? |
There was a problem hiding this comment.
Can we ditch this TODO now as well?
There was a problem hiding this comment.
Definitely, will nuke!
|
Hi Jorge, The problem was still present last I checked, but that was a couple weeks Thanks, On Tue, Jul 8, 2014 at 4:19 PM, Jorge Leandro Perez <
|
| + (NSArray *)parseFromLegacyDictionary:(NSDictionary *)rawLegacy; | ||
|
|
||
| + (instancetype)relationshipFromObjectWithKey:(NSString *)sourceKey | ||
| andAttribute:(NSString *)sourceAttribute |
There was a problem hiding this comment.
I have mixed feelings about using and in Obj-C method names. How about you?
There was a problem hiding this comment.
You're right, sounds better without and's. Let's change that!
|
@CapoChino may i ask you if you checked against this PR? exact same scenario, nil relationships? Thanks! |
| andAttribute:(NSString *)sourceAttribute | ||
| inBucket:(NSString *)sourceBucket | ||
| toObjectWithKey:(NSString *)targetKey | ||
| inBucket:(NSString *)targetBucket; |
There was a problem hiding this comment.
Similar comment: instead of two inBucket names, could instead do sourceBucket and targetBucket. Less of an English sentence, but perhaps a bit clearer.
There was a problem hiding this comment.
Nice catch! let's update that signature as well.
Issue #250: Implements Bidirectional RelationshipResolver
The goal of this PullRequest is to prevent any scenarios that could potentially cause relationships not-to-be-sync'ed, such as "Source Objects" being inserted after the "Target Objects".
The new mechanism will store a descriptor for each relationship, containing the following fields:
Whenever any of the involved objects get inserted (Source/Target), the relationship resolver will check the pending descriptors containing the inserted object's key, and will attempt to establish the relationship. This will be performed for both, Source + Target objects. As a result, insertion order should not be an issue anymore.
Fixes Issue #250