Skip to content

CoreData Date issues #297

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

Merged
merged 1 commit into from
Jul 15, 2025
Merged

CoreData Date issues #297

merged 1 commit into from
Jul 15, 2025

Conversation

NachoEmbrace
Copy link
Contributor

This PR attempts to address the crash we've seen regarding non-optional Dates in CoreData.

  1. It sets a default value in the model for all non-optional Dates.
  2. We noticed there were some places where we were fetching an object in an operation (performAndWait), then reading / modifying that object in another performAndWait block. This PR combines those operations into 1 in all places that makes sense.

Reducing thread switching when adding and updating objects into CoreData
@NachoEmbrace NachoEmbrace requested a review from a team as a code owner July 11, 2025 15:54
Copy link

Dependency Review

The following issues were found:
  • ✅ 0 vulnerable package(s)
  • ✅ 0 package(s) with incompatible licenses
  • ✅ 0 package(s) with invalid SPDX license definitions
  • ⚠️ 2 package(s) with unknown licenses.
See the Details below.

License Issues

Package.resolved

PackageVersionLicenseIssue Type
github.com/undefinedlabs/opentracing-objc0.5.2NullUnknown License
github.com/undefinedlabs/thrift-swift1.1.2NullUnknown License

OpenSSF Scorecard

PackageVersionScoreDetails
swift/github.com/undefinedlabs/opentracing-objc 0.5.2 UnknownUnknown
swift/github.com/undefinedlabs/thrift-swift 1.1.2 UnknownUnknown

Scanned Files

  • Package.resolved

Copy link

codecov bot commented Jul 11, 2025

Codecov Report

Attention: Patch coverage is 94.31818% with 10 lines in your changes missing coverage. Please review.

Project coverage is 88.60%. Comparing base (1414906) to head (f3d6840).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...orageInternal/Records/EmbraceStorage+Session.swift 93.44% 4 Missing ⚠️
...rageInternal/Records/EmbraceStorage+Metadata.swift 94.44% 3 Missing ⚠️
...eStorageInternal/Records/EmbraceStorage+Span.swift 94.44% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #297      +/-   ##
==========================================
- Coverage   88.63%   88.60%   -0.03%     
==========================================
  Files         461      461              
  Lines       31675    31749      +74     
==========================================
+ Hits        28074    28132      +58     
- Misses       3601     3617      +16     
Files with missing lines Coverage Δ
...rces/EmbraceCoreDataInternal/CoreDataWrapper.swift 73.99% <100.00%> (+0.23%) ⬆️
...ces/EmbraceStorageInternal/Records/LogRecord.swift 91.72% <100.00%> (+0.06%) ⬆️
...mbraceStorageInternal/Records/MetadataRecord.swift 98.66% <100.00%> (+0.01%) ⬆️
...EmbraceStorageInternal/Records/SessionRecord.swift 99.15% <100.00%> (+<0.01%) ⬆️
...es/EmbraceStorageInternal/Records/SpanRecord.swift 98.96% <100.00%> (+0.01%) ⬆️
...rageInternal/Records/EmbraceStorage+Metadata.swift 95.81% <94.44%> (-0.45%) ⬇️
...eStorageInternal/Records/EmbraceStorage+Span.swift 97.20% <94.44%> (-0.21%) ⬇️
...orageInternal/Records/EmbraceStorage+Session.swift 96.11% <93.44%> (-0.02%) ⬇️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@@ -138,7 +138,9 @@ public class CoreDataWrapper {
}

do {
try context.save()
if context.hasChanges {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should help fix high disk writes and cpu reported by Apple as well, but not completely.

@naftaly
Copy link
Collaborator

naftaly commented Jul 14, 2025

I think the real fix here is putting everything behind the performOperation and this way we don't cause any race conditions so I'd prefer if we didn't add the default values, or the hasChanges so that we can really know what fixes the issue.

Copy link
Collaborator

@ArielDemarco ArielDemarco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@NachoEmbrace NachoEmbrace merged commit fce29f2 into main Jul 15, 2025
9 checks passed
@NachoEmbrace NachoEmbrace deleted the more_core_data_fixes branch July 15, 2025 15:10
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.

5 participants