-
Notifications
You must be signed in to change notification settings - Fork 47
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
[4/4] Query for Updates from Nexus, and Fix Miscellaneous Bugs in DataStore #2113
Conversation
…ystem used by Mod Pages
…s of Existing Types
…2 GameId and Friends
…idForFile and UidForMod
This PR conflicts with |
This PR doesn't conflict with |
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.
Looks pretty solid aside from the above comment.
/// The 'Feed' in the context of the Nexus App is the individual game's 'updated.json' endpoint; | ||
/// i.e. a 'Game Mod Feed' | ||
/// </remarks> | ||
public class PerFeedCacheUpdater<TUpdateableItem> where TUpdateableItem : ICanGetLastUpdatedTimestamp, ICanGetUidForMod |
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.
This code project is overkill for the task at hand. Instead implement what we can on the V2 API and put that code inside the NexusWebApi project.
When we over-abstract like this we endup with a lot of code that has to be assembled to do something that is only ever used with one set of data. We have a "Per feed updater" that can update any number of things, mixins for Mod and Page metadata, ICanGet...
interfaces. All that can be reduced to a few small method in the API class if we assume that what we're caching is mod info from the V2 API
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.
I can merge ICanGetLastUpdatedTimestamp
and ICanGetUidForMod
interfaces if needed.
I actually didn't write the abstractions because of the V1 API, my original intent here was:
- Easier unit testing.
- Code deduplication.
For the latter, it was specifically due to an existing detail in the API. The current website has 2 separate timestamps in the legacy API, latest_mod_activity
and latest_file_update
. We don't yet know backend's plan around these. More specifically, if we'll be eventually able to get ModFiles
from Mod
node in V2, then we'll only need one of them; otherwise we'll need both.
In the event we do need both timestamps, this class allows for code reuse, you just make a separate mixin struct implementing interface that pulls from the other timestamp and all logic is reused.
Since the code already exists, would it be possible to have this code merged as-is, and instead alter it based on results of speaking with backend? (i.e. Open a ticket for the work to deabstract, assign it to me, and close it or have me do it depending on result from backend. i.e. Deabstract if 1 timestamp, keep if 2 timestamps) I think that would be the least amount of work (most efficient way to proceed).
I made a small improvement here. This code now also closes: I also:
I would still advise to keep the abstraction, until backend lets us know whether |
This PR conflicts with |
# Conflicts: # src/Abstractions/NexusMods.Abstractions.NexusModsLibrary/NexusModsFileMetadata.cs # src/Abstractions/NexusMods.Abstractions.NexusModsLibrary/NexusModsModPageMetadata.cs # src/Abstractions/NexusMods.Abstractions.NexusWebApi/Types/V2/GameId.cs # src/Abstractions/NexusMods.Abstractions.NexusWebApi/Types/V2/Uid/UidForFile.cs # src/Abstractions/NexusMods.Abstractions.NexusWebApi/Types/V2/Uid/UidForMod.cs # src/Networking/NexusMods.Networking.ModUpdates/MultiFeedCacheUpdater.cs # src/Networking/NexusMods.Networking.ModUpdates/NexusMods.Networking.ModUpdates.csproj # src/Networking/NexusMods.Networking.ModUpdates/PerFeedCacheUpdater.cs # src/Networking/NexusMods.Networking.ModUpdates/PerFeedCacheUpdaterResult.cs # src/Networking/NexusMods.Networking.NexusWebApi/Extensions/FragmentExtensions.cs # src/Networking/NexusMods.Networking.NexusWebApi/NexusModsLibrary.cs # tests/Networking/NexusMods.Networking.ModUpdates.Tests/Helpers/TestItem.cs # tests/Networking/NexusMods.Networking.ModUpdates.Tests/NexusMods.Networking.ModUpdates.Tests.csproj # tests/Networking/NexusMods.Networking.NexusWebApi.Tests/Types/V2/UidForModTests.cs
This PR doesn't conflict with |
This is last PR in chain. Continues from:
Review this if you want to review all parts at once, else review by parts sequentially.
Also contributes a bit towards:
Summary
This PR adds the final puzzle pieces towards efficient checking for updates; and also
a little bit of work towards migration to V2 that was required as part of this.
Changelog
UidForFile
,UidForMod
,GameId
).GameId
andGameDomain
. This is temporary, and helps us iterativelymigrate towards full usage of V2.
uid
strings.UpdatingModPageMetadata_ViaWebApi_ShouldWork
) for fetching file updates, which:The last mentioned test will be further improved in the future; with mocks.
For now this is left out in order to reduce amount of work done in long run.
i.e. Waiting for new APIs from backend, and not mocking anything V1.
Extra: Use
uid
as primary key ofNexusModsFileMetadata
This makes it so the primary key uniquely identifies a file on Nexus.
Previously, the code used
FileId
, howeverFileId
is game specific;multiple games may have a file with same
FileId
.This actually fixes some bugs. To give an example, if you installed Mod 1000 in Stardew Valley,
installing Mod 1000 from Cyberpunk2077 as part of a collection will no longer assume you
have that specific mod installed.
Some Notes
We currently use the time the mod page itself was last updated for determining if we should
refersh local mod page info. This is due to a missing field in the V2 API. Trivial 1 line change
once we do get it.
fixes #2095
fixes #2092
fixes #2008