Skip to content

Conversation

@samwyndham
Copy link
Contributor

@samwyndham samwyndham commented Oct 31, 2025

TaskWPB-19688 [iOS] Move DB data model to `WireData`

Issue

This PR is a new version of #3521. As that PR was quite far behind develop and the core data model has changed, I decided to combine changes from the original PR with a few updates.

This PR moves the zmessaging and ZMEventModel database models from WireDataModel framework to WireData package 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:

  • Model files were moved to WireData
  • Managed object subclass files were left in WireDataModel - these can be moved one by one as needed.
  • The module for each entity was changed to WireDataModelis previously it was set asCurrent Product Module` - see notes below.
  • The locations of the model bundles were updated in tests.
  • In Xcode project based modules that depend on core data, their test hosts were updated to link to the modules target. E.g. WireRequestStrategyTestHost was updated to link to WireRequestStrategy. Without doing this the WireDataBundle.bundle would 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 #3521
  • A new test host was added for WireDomain to 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:

Screenshot 2025-08-25 at 10 24 12

This can have 3 states:

  • Global Namespace
  • Current Product Module
  • Custom (we enter a target)

Global Namespace is 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 Module is like Custom but automatically sets the target based on where the database models reside. Currently Current Product Module resolves to WireDataModel but with this PR we move the database models to WireData while keeping the classes in WireDataModel. This means that we have to updates settings of Current Product Module to WireDataModel.

I was surprised at the need to update each version of zmessaging.xcdatamodeld, not just 2.129 but 2.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 of ParticipantRole is compatible with version 2.106.0 for example. It would be better to use just NSManagedObject with KVC. However it's probably best to leave things how they are.

Testing

  • Ensure app is building and running properly
  • Ensure all unit tests are passing test run here
  • Ensure database migration from 2.130 to 2.131 is working properly

Checklist

  • Title contains a reference JIRA issue number like [WPB-XXX].
  • Description is filled and free of optional paragraphs.
  • Adds/updates automated tests.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 31, 2025

Test Results

   27 files  1 257 suites   13m 2s ⏱️
9 511 tests 9 475 ✅ 32 💤 4 ❌
9 524 runs  9 492 ✅ 32 💤 0 ❌

For more details on these failures, see this check.

Results for commit 669dfc7.

♻️ This comment has been updated with latest results.

@datadog-wireapp
Copy link

datadog-wireapp bot commented Oct 31, 2025

✅ Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 669dfc7 | Docs | Was this helpful? Give us feedback!

<?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">
Copy link
Contributor

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?

Copy link
Contributor Author

@samwyndham samwyndham Nov 3, 2025

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.

Copy link
Contributor Author

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).

Copy link
Contributor

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.

Copy link
Collaborator

@netbe netbe left a 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">
Copy link
Collaborator

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 ?

Copy link
Contributor Author

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.

Copy link
Collaborator

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

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants