pjechris/cohesionkit

Single source of truth library

cohesionkit's Issues

🐛 Crash

Description

In our last release (using CohesionKit 0.15.1) we have some new crashes:
crash_info_entry_0 Swift/KeyPath.swift:1809: Fatal error: Unexpectedly found nil while unwrapping an Optional value

Crashed: com.cohesionkit.identitymap
0  libswiftCore.dylib             0x3a3c8 closure #1 in closure #1 in closure #1 in _assertionFailure(_:_:file:line:flags:) + 228
1  libswiftCore.dylib             0x3a2a0 closure #1 in closure #1 in _assertionFailure(_:_:file:line:flags:) + 332
2  libswiftCore.dylib             0x39c2c _assertionFailure(_:_:file:line:flags:) + 184
3  libswiftCore.dylib             0x34a7a4 specialized RawKeyPathComponent._projectMutableAddress<A, B>(_:from:to:isRoot:keepAlive:) + 1596
4  libswiftCore.dylib             0x1650bc WritableKeyPath._projectMutableAddress(from:) + 424
5  libswiftCore.dylib             0x168a6c swift_setAtWritableKeyPath + 420
6  Aircall                        0xa936dc closure #1 in EntityNode.observeChild<A>(_:for:) + 72 (EntityNode.swift:72)
7  Aircall                        0xa93b0c closure #1 in EntityNode.observeChild<A, B>(_:identity:update:) + 100 (EntityNode.swift:100)
8  Aircall                        0xa8bc3c partial apply for thunk for @escaping @callee_guaranteed (@in_guaranteed A) -> () + 4341087292 (<compiler-generated>:4341087292)
9  Aircall                        0xa8b770 closure #1 in Observable.value.didset + 32 (Observable.swift:32)
10 Aircall                        0xa8bb8c partial apply for closure #1 in Observable.value.didset + 4341087116 (<compiler-generated>:4341087116)
11 libswiftCore.dylib             0xdee70 Sequence.forEach(_:) + 756
12 Aircall                        0xa8b6c0 Observable.value.didset + 32 (Observable.swift:32)
13 Aircall                        0xa93b1c closure #1 in EntityNode.observeChild<A, B>(_:identity:update:) + 100 (EntityNode.swift:100)
14 Aircall                        0xa8bc3c partial apply for thunk for @escaping @callee_guaranteed (@in_guaranteed A) -> () + 4341087292 (<compiler-generated>:4341087292)
15 Aircall                        0xa8b770 closure #1 in Observable.value.didset + 32 (Observable.swift:32)
16 Aircall                        0xa8bb8c partial apply for closure #1 in Observable.value.didset + 4341087116 (<compiler-generated>:4341087116)
17 libswiftCore.dylib             0xdee70 Sequence.forEach(_:) + 756
18 Aircall                        0xa8b6c0 Observable.value.didset + 32 (Observable.swift:32)
19 Aircall                        0xa93b1c closure #1 in EntityNode.observeChild<A, B>(_:identity:update:) + 100 (EntityNode.swift:100)
20 Aircall                        0xa8bc3c partial apply for thunk for @escaping @callee_guaranteed (@in_guaranteed A) -> () + 4341087292 (<compiler-generated>:4341087292)
21 Aircall                        0xa8b770 closure #1 in Observable.value.didset + 32 (Observable.swift:32)
22 Aircall                        0xa8bb8c partial apply for closure #1 in Observable.value.didset + 4341087116 (<compiler-generated>:4341087116)
23 libswiftCore.dylib             0xdee70 Sequence.forEach(_:) + 756
24 Aircall                        0xa8b6c0 Observable.value.didset + 32 (Observable.swift:32)
25 Aircall                        0xa8bbf0 specialized Observable.value.setter + 30 (Observable.swift:30)
26 Aircall                        0xa933b0 EntityNode.updateEntity(_:modifiedAt:) + 55 (EntityNode.swift:55)
27 Aircall                        0xa871c4 EntityStore.nodeStore<A>(in:entity:modifiedAt:) + 164 (EntityStore.swift:164)
28 Aircall                        0xa94580 EntityStoreStoreVisitor.visit<A, B>(context:entity:) + 4341122432
29 Aircall                        0xa94514 EntityStoreStoreVisitor.visit<A, B>(context:entity:) + 4341122324
30 Aircall                        0xa8991c closure #1 in PartialIdentifiableKeyPath.init<A>(_:) + 4341078300
31 Aircall                        0xa897b4 partial apply for closure #1 in PartialIdentifiableKeyPath.init<A>(_:) + 4341077940 (<compiler-generated>:4341077940)
32 Aircall                        0xa8757c EntityStore.nodeStore<A>(in:entity:modifiedAt:) + 188 (EntityStore.swift:188)
33 Aircall                        0xa94bac EntityStoreStoreVisitor.visit<A, B>(context:entities:) + 4341124012
34 Aircall                        0xa947cc EntityStoreStoreVisitor.visit<A, B>(context:entities:) + 4341123020
35 Aircall                        0xa8a174 closure #1 in PartialIdentifiableKeyPath.init<A>(_:) + 4341080436
36 Aircall                        0xa89d20 partial apply for closure #1 in PartialIdentifiableKeyPath.init<A>(_:) + 4341079328
37 Aircall                        0xa8757c EntityStore.nodeStore<A>(in:entity:modifiedAt:) + 188 (EntityStore.swift:188)
38 Aircall                        0xa879c4 EntityStore.storeAlias<A>(content:key:modifiedAt:) + 207 (EntityStore.swift:207)
39 Aircall                        0xa86c30 closure #1 in EntityStore.store<A>(entities:named:modifiedAt:) + 4341066800
40 Aircall                        0xa891ec partial apply for closure #1 in EntityStore.store<A>(entities:named:modifiedAt:) + 4341076460
41 Aircall                        0xa891b4 partial apply for closure #1 in EntityStore.store<A>(entities:named:modifiedAt:) + 4341076404
42 Aircall                        0xa896a8 partial apply for closure #1 in EntityStore.transaction<A>(_:) + 216 (EntityStore.swift:216)
43 libswiftDispatch.dylib         0x1d4c partial apply for thunk for @callee_guaranteed () -> (@out A, @error @owned Error) + 28
44 libswiftDispatch.dylib         0x9ea0 thunk for @callee_guaranteed () -> (@out A, @error @owned Error)partial apply + 16
45 libswiftDispatch.dylib         0x1c80 closure #1 in closure #1 in OS_dispatch_queue._syncHelper<A>(fn:execute:rescue:) + 192
46 libswiftDispatch.dylib         0x1bb4 partial apply for thunk for @callee_guaranteed () -> () + 28
47 libswiftDispatch.dylib         0x1b8c thunk for @escaping @callee_guaranteed () -> () + 28
48 libdispatch.dylib              0x3dd4 _dispatch_client_callout + 20
49 libdispatch.dylib              0x132c4 _dispatch_lane_barrier_sync_invoke_and_complete + 56
50 libswiftDispatch.dylib         0x28a4 implicit closure #2 in implicit closure #1 in OS_dispatch_queue.sync<A>(flags:execute:) + 196
51 libswiftDispatch.dylib         0x2540 OS_dispatch_queue._syncHelper<A>(fn:execute:rescue:) + 404
52 libswiftDispatch.dylib         0x39cc OS_dispatch_queue.sync<A>(flags:execute:) + 332
53 Aircall                        0xa87b18 EntityStore.transaction<A>(_:) + 213 (EntityStore.swift:213)
54 Aircall                        0xa86bb0 EntityStore.store<A>(entities:named:modifiedAt:) + 4341066672
55 Aircall                        0xa86b2c EntityStore.store<A>(entities:named:modifiedAt:) + 4341066540
56 Aircall                        0x177cb4 closure #2 in LineRepository.downloadLines() + 156 (LineRepository.swift:156)
57 Aircall                        0x4ecc44 thunk for @escaping @callee_guaranteed (@guaranteed [LineInformation]) -> () + 4335193156 (<compiler-generated>:4335193156)
58 Combine                        0x1898c Publishers.HandleEvents.Inner.receive(_:) + 128
59 Combine                        0x18900 protocol witness for Subscriber.receive(_:) in conformance Publishers.HandleEvents<A>.Inner<A1> + 24
60 Combine                        0x1d040 Future.Conduit.fulfill(_:) + 1952
61 Combine                        0x1c870 Future.Conduit.offer(_:) + 340
62 Combine                        0x1c70c partial apply for closure #1 in Future.promise(_:) + 68
63 Combine                        0x22040 ConduitList.forEach(_:) + 276
64 Combine                        0x20cd8 Future.promise(_:) + 1312
65 Combine                        0x207ac partial apply for closure #1 in Future.init(_:) + 28
66 Aircall                        0x534d50 specialized closure #1 in closure #1 in static Future<>.async(_:) + 29 (Publisher+Async.swift:29)
67 libswift_Concurrency.dylib     0x4d764 swift::runJobInEstablishedExecutorContext(swift::Job*) + 436
68 libswift_Concurrency.dylib     0x4e9c8 swift_job_runImpl(swift::Job*, swift::ExecutorRef) + 72
69 libdispatch.dylib              0x15894 _dispatch_root_queue_drain + 392
70 libdispatch.dylib              0x1609c _dispatch_worker_thread2 + 156
71 libsystem_pthread.dylib        0x1ee4 _pthread_wqthread + 228
72 libsystem_pthread.dylib        0x1fc0 start_wqthread + 8
  

Could our ex but beloved staff engineer have a look to that 🙏

Reproduction steps

We don't know yet how to reproduce the problem

Screenshots/GIF

![DESCRIPTION](LINK.png)

Logs

No response

Framework version

0.15.1

Swift

No response

Xcode

No response

💡 Lift custom collection limits

Summary

Custom collections of Identifiable/Aggregate objects need to implement AccelerateMutableBuffer in order for the library to be able to update values inside it.

We could probably use improvements from 5.8 to lift that restriction

Basic Example

Today:

1. Given struct MyCollection<Element>: Collection -> identityMap.store(myCollection) doesn't work
2. Givenstruct MyCollection<Element>: Collection, AccelerateMutableBuffer -> identityMapMap.store(myCollection) works

1 should work (given Element is Identifiable/Aggregate obivously.)

Unresolved questions

No response

💡 Add a benchmark report

Summary

Would probably be interesting to have some benchmark reports when updating code to have an idea about the lib performances.

Probably using https://github.com/ordo-one/package-benchmark would?

Basic Example

When editing IdentityMap it could tell us if performance increased or decreased.

Unresolved questions

No response

💡 Create a transaction system

Summary

Now that we have ObserverRegistry, I think it would be possible to have a transaction mechanism:

• all updates would be grouped into a transaction
• updates would be triggered at the end of transaction
  This could help better managing the number of updates

Basic Example

// implicit transaction
entityStore.store(xx)
// registry send events

// implicit 2nd transaction
entityStore.store(xx)
// registry send events

// explicit transaction
transaction {
  entityStore.store(xx)
  entityStore.store(xx)
}
// registry send events

Unresolved questions

No response

Migrate to actor/async

Update the code to use actors instead of using DispatchQueue

🐛 Logger.didFailedToStore reports some false positive

Description

When storing a value multiple times in a row (with an array for example), subsequent storages won't happen but will trigger a falsely reported Logger.didFailedToStore

Reproduction steps

1. Create an object a:A and b:A, **both** having a child c:C
2. Call store([a,b])

Objects a,b, and c are reported as logged. But c is **also** reported as not stored.

Screenshots/GIF

No response

Logs

No response

Framework version

0.9

Swift

5.7

Xcode

No response

💡 Make `update` more fine grained about what to update

Summary

Today when update closure is called the whole value is updated even if:

• not all its relationships changed
• maybe even nothing changed at all

Idea would be to:

• do nothing when nothing changed (no update or update applied the same value)
• only update what changed (if a nested entity did not change: do not update it)

Basic Example

// BEFORE: triggers an update on A and all its children
// AFTER: triggers nothing
identityMap.store(A.self, id: a.id) { _ in
}


// BEFORE: triggers an update on A, B and C
// AFTER: triggers an update on A and C (B did not change)
identityMap.store(A.self, id: a.id) {
  $0.b = $0.b
  $0.c = newValue
}

Unresolved questions

No response

💡 Add a `get` method

Summary

Today we have find methods which returns a successful observer only if the value is already stored.

Idea would be to add a get counterpart which would always suceeds. This would allow to declare an observer even if the value is not yet present.

To be consistent find(named:) would be renamed get(named:) as aliases never fail

Basic Example

identityMap.find(Foo.self, id: 1) // returns nil because value is not stored yet
identityMap.get(Foo.self, id: 1) // returns an Observer even if value is not stored. Might never receive any value if Foo(id:1) is never stored

identityMap.store(Foo(id: 1, title: "hello world")) // observer is notified

Unresolved questions

Implementing such feature would require refactoring on how observers are handled currently: for now observer is tightly tied with the presence of the value in IdentityMap.

We would need a way of declaring such an observer even when value is not stored yet.

💡 Coalesce multiple changes

Summary

For now CohesionKit is notifying for each changes happening. This has some drawbacks:

• some items might change multiple times in a cycle but trigger multiple updates
• collections for sure have high change to change multiple times. Each time an items changes it triggers a new change notification.

Like probably what is doing SwiftUI, idea is to coalesce multiple changes in a single notification.

Basic Example

// BEFORE: triggers 3 values (initial + 2 changes)
identityMap.store(a)
identityMap.store(a) 
identityMap.update(A.self, id: a.id) { a.foo = xx }


// AFTER: triggers 2 values (initial + 1 change)
identityMap.store(a)
identityMap.store(a) 
identityMap.update(A.self, id: a.id) { a.foo = xx }

Unresolved questions

I guess how many changes should be collapsed would be undefined?

🐛 Aliases do not report indirect updates

Description

Aliases do not report entity updates when they are updated without using alias.

Reproduction steps

This should print "update triggered" but doesn't

identityMap.store(entity: SingleNodeFixture(id: 0), named: .test) // with alias

identityMap.find(named: .test).observe { _ in print("update triggered") }

identitMap.store(entity: SingleNodeFixture(id: 0, primitive: "update")) // with no alias

Once the alias node changed at least once its works as expected:

identityMap.store(entity: SingleNodeFixture(id: 0), named: .test) // with alias

identityMap.find(named: .test).observe { _ in print("update triggered") }

identityMap.store(entity: SingleNodeFixture(id: 0), named: .test) // triggers alias observe registering

identitMap.store(entity: SingleNodeFixture(id: 0, primitive: "update")) // with no alias

Screenshots/GIF

No response

Logs

No response

Framework version

0.9

Swift

5.7

Xcode

14.1

Add SwiftLint

Add SwiftLint to lint code:

2. Add a step to run linter in github test workflow
3. Add swift format file with following rules (if they exist)
   • space/indentation: 4
   • max line width: 120
   • cases should not be indented
   • else and catch should go on a new line
   • return should go on a new line
   • warnings should be disallowed (aka max warning is 0)

(The rules should be adapted to make it work with current code)