Fix version sorting by using SemVer instead of publishedDate#85
Fix version sorting by using SemVer instead of publishedDate#85Moustachauve wants to merge 5 commits into
Conversation
Fixes #84 The app previously sorted versions by their GitHub publishedAt date to determine the latest available version. This could lead to incorrect results when older version branches receive patches with a newer publishedAt date (e.g. v0.15.5 appearing newer than v16.0.0). The fix replaces the Core Data query's publishedDate sort with an in-memory comparison using the existing SemanticVersion struct. Versions with invalid semver tags fall back to publishedDate comparison. Also adds comprehensive unit tests for ReleaseService covering the exact bug scenario, stable/beta filtering, nightly exclusion, and the getNewerReleaseTag helper.
There was a problem hiding this comment.
Code Review
This pull request updates the ReleaseService to determine the latest version using SemanticVersion logic rather than relying on Core Data's date-based sorting. It also introduces a comprehensive test suite for version fetching and comparison. Review feedback suggests extending the use of SemanticVersion to the getNewerReleaseTag method to avoid comparison errors, particularly when moving from beta to stable releases. Additionally, there is a recommendation to optimize memory usage by limiting the properties fetched from Core Data, as removing the fetch limit may lead to loading large release descriptions into memory. Finally, a specific test case for the beta-to-stable transition was proposed to ensure robust update detection.
I am having trouble creating individual review comments. Click here to see my feedback.
wled/Service/ReleaseService.swift (56)
While getLatestVersion now correctly identifies the latest version using SemanticVersion, the getNewerReleaseTag method (lines 22-38) still relies on .numeric string comparison to determine if an update is available. This creates an inconsistency and will fail to detect updates in specific scenarios. For example, if a user is on a beta version (e.g., "0.16.0-b1") and the stable "0.16.0" is released, .numeric comparison considers "0.16.0" to be less than "0.16.0-b1" (because the former is a prefix of the latter), resulting in no update being detected. You should update getNewerReleaseTag to use SemanticVersion for its comparison logic as well.
wled/Service/ReleaseService.swift (55)
Removing the fetchLimit and sortDescriptors means that every Version entity in the database is now fully instantiated into memory to find the maximum. Since each Version object contains a versionDescription (the release body) which can be quite large, this could lead to unnecessary memory pressure as the release history grows. Consider using propertiesToFetch to only load the tagName and publishedDate fields, or fetching the objects as faults to minimize the initial memory impact.
wledTests/ReleaseServiceTests.swift (177-193)
The current test suite for getNewerReleaseTag is missing a critical edge case: transitioning from a pre-release to a stable release of the same version number (e.g., current "0.16.0-b1" to latest "0.16.0") when the branch is set to .beta. Adding this test case will likely reveal the comparison bug in getNewerReleaseTag where .numeric string comparison fails to recognize the stable release as newer.
@Test func newerReleaseTagRespectsIgnoreVersion() throws {
let context = makeInMemoryContext()
try deleteAllVersions(context: context)
insertVersion(context: context, tagName: "0.16.0")
try context.save()
let service = ReleaseService(context: context)
let result = service.getNewerReleaseTag(
versionName: "0.15.0",
branch: .stable,
ignoreVersion: "0.16.0"
)
#expect(result == "")
}
@Test func newerReleaseTagDetectsUpdateFromBetaToStable() throws {
let context = makeInMemoryContext()
try deleteAllVersions(context: context)
insertVersion(context: context, tagName: "0.16.0")
try context.save()
let service = ReleaseService(context: context)
let result = service.getNewerReleaseTag(
versionName: "0.16.0-b1",
branch: .beta,
ignoreVersion: ""
)
#expect(result == "0.16.0")
}…ize getLatestVersion fetch, and add test for beta-to-stable update
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the ReleaseService to prioritize semantic versioning over publication date when identifying the latest release. It updates the getLatestVersion method to perform in-memory sorting using SemanticVersion and adds a comprehensive test suite to verify versioning logic. Feedback was provided regarding an ineffective Core Data optimization where propertiesToFetch was used with a managed object result type, which is ignored by the framework.
| fetchRequest.predicate = NSCompoundPredicate(andPredicateWithSubpredicates: predicates) | ||
| fetchRequest.propertiesToFetch = ["tagName", "publishedDate"] |
There was a problem hiding this comment.
The propertiesToFetch property is only effective when the resultType of the NSFetchRequest is set to .dictionaryResultType. Since this request uses the default .managedObjectResultType, this line is ignored by Core Data. For a small dataset like the one typically returned by the GitHub releases API, this optimization is unnecessary and should be removed to keep the code clean and avoid potential confusion.
| fetchRequest.predicate = NSCompoundPredicate(andPredicateWithSubpredicates: predicates) | |
| fetchRequest.propertiesToFetch = ["tagName", "publishedDate"] | |
| fetchRequest.predicate = NSCompoundPredicate(andPredicateWithSubpredicates: predicates) |
Fixes #84
This PR updates the version retrieval logic to use Semantic Versioning (SemVer) comparison instead of GitHub's
publishedAtdate.Currently, the app sorts versions by their publication date, which can be incorrect if an older version branch receives a patch after a newer major/minor version has been released (e.g., v0.15.5 having a newer
publishedAtdate than v16.0.0).Changes:
ReleaseService.getLatestVersion(branch:)to fetch all relevant versions and identify the latest one usingSemanticVersioncomparison.ReleaseServiceTests.swiftto verify the fix and ensure stable/beta filtering and nightly exclusion still work as expected.This mirrors the fix applied to the Android app in Moustachauve/WLED-Android#168.