-
Notifications
You must be signed in to change notification settings - Fork 24
chore: move db models to wire data package (second attempt) - WPB-19688 #3819
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
base: develop
Are you sure you want to change the base?
Conversation
Test Results 27 files 1 257 suites 13m 2s ⏱️ For more details on these failures, see this check. Results for commit 669dfc7. ♻️ This comment has been updated with latest results. |
…reReturned flakiness
| <?xml version="1.0" encoding="UTF-8" standalone="yes"?> | ||
| <model type="com.apple.IDECoreDataModeler.DataModel" documentVersion="1.0" lastSavedToolsVersion="24299" systemVersion="24G231" minimumToolsVersion="Xcode 15.0" sourceLanguage="Swift" userDefinedModelVersionIdentifier="2.131.0"> | ||
| <entity name="Action" representedClassName=".Action" syncable="YES"> | ||
| <entity name="Action" representedClassName="WireDataModel.Action" syncable="YES"> |
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.
question: WireData cannot access WireDataModel. Are you sure referencing entity classes from WireDataModel is correct?
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.
@caldrian Yes I think this is fine. My understanding is that representedClassName is only accessed dynamically when we try to create/access in this case an Action. As we don't access Action from WireData this is OK. I expect that if this doesn't work it would be immediately obvious when running the app.
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.
Unfortunately I can't find documentation on this. The best I have is ChatGPTs answer to this question which I think has a good point - that we don't access xcdatamodeld directly. Instead this is compiled to a .momd that lives in the apps bundle and is accessed dynamically.
From chat GPT:
Let’s restate what’s going on:
- Target Bar contains your Core Data model (.xcdatamodeld).
- Target Foo contains your NSManagedObject subclasses.
- Foo imports Bar, so it can access the model if needed.
- The model in Bar references the classes in Foo via representedClassName.
- But Bar does not import Foo.
Now the key question:
Is it OK that Bar does not import Foo?
✅ Short answer: Yes, that’s perfectly fine — and actually desirable.
Why It’s OK
The .xcdatamodeld file doesn’t compile into code that needs to reference the managed object subclasses at build time. Instead, it compiles into a .momd resource (a Core Data model metadata file) that simply stores the class names as strings (e.g. "Foo.MyEntity").
When Core Data loads the model at runtime, it uses these class names as strings and asks the Objective-C runtime (or Swift runtime) to resolve them. That means:
- There’s no compile-time dependency between your model bundle (Bar) and your entity classes (Foo).
- The only requirement is that, at runtime, the classes referenced by representedClassName actually exist and can be found via the runtime (e.g. are linked into the process).
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.
Hm, I'm a bit skeptical but let's try it.
I guess once we want to have tests in WireData we have to move the entity files as well.
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.
left a few comments, but otherwise looks good to me :)
Question: Can we close #3521 ?
| <relationship name="removedUsers" optional="YES" toMany="YES" deletionRule="Nullify" destinationEntity="User" inverseName="showingUserRemoved" inverseEntity="User"/> | ||
| <relationship name="users" optional="YES" toMany="YES" deletionRule="Nullify" destinationEntity="User" inverseName="systemMessages" inverseEntity="User"/> | ||
| </entity> | ||
| <entity name="Team" representedClassName=".Team" syncable="YES"> |
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.
thought: So I wonder if we should just duplicate / freeze the model for older versions, that way all the models don't need to be updated
But this could cause some confusion for us, what do you think @samwyndham ?
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.
@netbe The issue is that we access the old models in tests, this is why they have also been updated. Keep in mind though that these changes are not schema changes so does not affect migrations.
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.
@samwyndham which test in particular ? it would make sense to remove or update the test
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.
@netbe The tests I know about are DatabaseMigrationTests_Conversations. It is possible there are more that I haven't checked. If during migration we rely on on NSManagedObject subclasses somewhere else that would also break.
# Conflicts: # wire-ios-data-model/WireDataModel.xcodeproj/project.pbxproj
Issue
This PR is a new version of #3521. As that PR was quite far behind
developand the core data model has changed, I decided to combine changes from the original PR with a few updates.This PR moves the
zmessagingandZMEventModeldatabase models fromWireDataModelframework toWireDatapackage ensuring that the managed objects that were previously pointing to the Current Product Module now points to WireDataModel (where still reside their related subclasses).To achieve this:
WireDataWireDataModel- these can be moved one by one as needed.is previously it was set asCurrent Product Module` - see notes below.WireRequestStrategyTestHostwas updated to link toWireRequestStrategy. Without doing this theWireDataBundle.bundlewould crash trying to find the bundle. This change ensures that the compiled core data module resides in the test host and is therefore accessible in tests. This was the main difference to the original PR: chore: move db models to wire data package - WPB-19688 #3521WireDomainto address the above issue as it was missing a test host.This PR is largely @jullianms good work - my changes mainly relate to resolving test host issue
Some additional notes
Some things I've learnt which I didn't know before regarding the classes module:
This can have 3 states:
Global Namespaceis required for managed object subclasses that are obj-c or those that are used from obj-c. This is because obj-c uses a global namespace (hence the need to add prefixes).Current Product Moduleis likeCustombut automatically sets the target based on where the database models reside. CurrentlyCurrent Product Moduleresolves toWireDataModelbut with this PR we move the database models toWireDatawhile keeping the classes inWireDataModel. This means that we have to updates settings ofCurrent Product ModuletoWireDataModel.I was surprised at the need to update each version of
zmessaging.xcdatamodeld, not just2.129but2.128,2.127, etc. This was necessary because some of our migration tests depend on our managed object subclasses with older database versions. For example,DatabaseMigrationTests_Conversations.testThatItPerformsMigrationFrom106_deleteConversationCascadesToParticipantRole(). I think this is a little problematic as there is no reason why the current version ofParticipantRoleis compatible with version2.106.0for example. It would be better to use just NSManagedObject with KVC. However it's probably best to leave things how they are.Testing
Checklist
[WPB-XXX].