-
Notifications
You must be signed in to change notification settings - Fork 443
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: KeyPath crash when querying CareStore #718
base: main
Are you sure you want to change the base?
Conversation
@@ -45,7 +45,7 @@ extension OCKStoreCoordinator { | |||
} | |||
|
|||
let sortDescriptor = NSSortDescriptor( | |||
keyPath: \OCKAnyOutcome.id, | |||
keyPath: \OCKCDOutcome.id, |
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.
I have some thoughts here...
It looks like the public SortDescriptor
for OCKOutcomeQuery
was removed in 3.0 (previous code here). My guess is the code above is here to maintain some consistency when the same query is made when the exact same data is in the database?
I think there is at least one case (maybe two) to add the public SortDescriptor
back. In #553 OCKOutcome
was turned into a versioned entity, but querying for OCKOutcome
doesn't necessarily follow the comments and documentation which can make since due to the nature of an outcome (may want to know the result for a particular day as oppose to the latest version). Still, there are times where a developer will want results by effectiveDate
, which all current queries to versioned objects currently allow, except for OCKOutcome
. Because of this, I've added extensions in my helper framework to assist with handling OCKOutcome
results netreconlab/CareKitEssentials#12
If you agree, we can add the public SortDescriptor
back, but include effectiveDate
like:
CareKit/CareKitStore/CareKitStore/Structs/Queries/OCKTaskQuery.swift
Lines 36 to 64 in 7416cce
/// Specifies the order in which query results will be sorted. | |
public enum SortDescriptor: Equatable { | |
case effectiveDate(ascending: Bool) | |
case groupIdentifier(ascending: Bool) | |
case title(ascending: Bool) | |
var nsSortDescriptor: NSSortDescriptor { | |
switch self { | |
case let .effectiveDate(ascending): | |
return NSSortDescriptor( | |
keyPath: \OCKAnyTask.effectiveDate, | |
ascending: ascending | |
) | |
case let .groupIdentifier(ascending): | |
return NSSortDescriptor( | |
keyPath: \OCKAnyTask.groupIdentifier, | |
ascending: ascending | |
) | |
case let .title(ascending): | |
return NSSortDescriptor( | |
keyPath: \OCKAnyTask.title, | |
ascending: ascending | |
) | |
} | |
} | |
} |
Another case can be added to the SortDescriptor
for taskOccurrenceIndex
, which could be useful. We can then add the case from sorting by id
(the above code), but mark that one as internal as I can imagine this could over time. We can prepend the sort by id
to all queries to replicate the current behavior of the above block of code.
Let me know what you think and I can create a follow-up PR after a decision is made on this one.
The framework crashes when you stream using:
OCKOutcomeQuery
,OCKCarePlanQuery
,OCKContactQuery
,OCKPatientQuery
,OCKTaskQuery
. Most likely happens with a standard fetch as well, but I didn't test those...Resulting in:
This can be replicated by using a simple streaming query within a view:
This most likely results from
NSSortDescriptor
not liking keyPaths of value types and prefers reference types (I'm guessing as I'm seen some other issues in the past with value type keyPaths and using NS...). In addition, since we are querying from CoreData, we should probably use the CD representations. Lastly, all other uses ofNSSortDescriptor
inCareKitStore
use theirOCKCD...
counterparts instead of the value type counterparts.Note
When I ran the test suite, none of the current tests for the framework hit the lines I changed which is probably why this wasn't caught 🙃. This is because the tests are conducted at a lower level than the value type representations, but solidifies that we should use the CoreData Entity representations for querying keyPaths:
CareKit/CareKitStore/CareKitStoreTests/Streaming/TestCoreDataQueryMonitor.swift
Lines 65 to 70 in 7416cce
CareKit/CareKitStore/CareKitStore/Streaming/CoreDataQueryMonitor.swift
Lines 47 to 63 in 7416cce