-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add common DataStore protocol #24
Conversation
Marking this as ready for review now because all DataStore methods that I have personally seen in servers has been implemented Since this PR comes from a branch on this repo though please feel free to push any new methods for games I have not touched |
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'm personally fine with moving the S3 client to the common protocols. Using the MinIO SDK shouldn't affect implementations that want to use another server since they all use S3. In order to allow custom configurations, I think we could let the server implementation create the MinIO client and pass it to the common protocols.
Also, we should consider integrating HPP here aswell, since the implementation seems to be fairly standard across all my dumps. For reference, these are the methods that it uses:
- PrepareGetObjectV1
- PreparePostObjectV1
- CompletePostObjectV1
- GetNotificationUrl
- GetNewArrivedNotificationsV1
- GetSpecificMetaV1
This doesn't affect the currently implemented methods on this PR, but it's worth mentioning since games like MK7 will use V1 versions of methods. On all HPP requests I've seen, the flags of PreparePostParamV1
is always 0x8, so I think we could check for that when handling notifications (unless we find a game that uses the flag outside of HPP).
Oh perfect. The MinIO docs have separate examples for when connecting to MinIO vs AWS so I assumed the MinIO client could only connect to MinIO servers. Awesome
This works for me. I was originally thinking about just providing a custom struct the user could fill their config details into and then common would create the client. But that was somewhat naive, and ignored advanced configurations. This is the better solution
I agree. As far as I know HPP should be, for servers which support it, a 1:1 substitute for NEX, and any protocols provided by NEX would be available to the HPP client (correct me if I'm wrong there). So it being supported here makes sense What all work needs to be done for this? I haven't looked much into HPP, even the implementation in
Over HPP or over NEX? Either way those should be implemented here tbh, we can add these to this PR if we have working implemenations of them. Just push to the branch
I'm not positive that's what this flag means, but it sounds like a good assumption for now. SMM doesn't use this flag for it's objects. Objects in SMM only have flags of 0, 256, or 3840 from what I can tell and I have no idea what they mean. We should make a note in the implementation though that this is basically just a heuristics guess and the exact flag use is not known |
I was thinking about this and I think we can't differentiate a NEX packet from an HPP packet right now? On Aside from this, I was thinking that we don't have access to the packet source and destination when we reach a method. We will need to look into this when we want to implement RV.
This is over NEX on PRUDP, Mario Kart 7 uses methods like
Sure, we can add a note about this |
I was also thinking about this the other day. Easiest solution would probably be to just send the original packet to the method instead of the client. The client can be obtained from the packets
Gotcha gotcha |
@DaniElectra @shutterbug2000 How are we feeling about the proposal to stop sending the client in the protocol handlers and instead sending the original packet? We can still get the client using the packet's
I know it would make the function signatures look a bit goofy, but I'm thinking it's a worthwhile change considering the benefits |
I agree. Go with it! |
Awesome, I'll get the changes made then! I don't think it's entirely relevant to this PR however so I'll make another one after this PR (seeing as all the protocols here will need updating) For now, is there anything else left before we merge this? |
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.
We may want to change the S3 data key to hold the update number in the future, but I think this is fine for now
Lays the ground work for implementing a common DataStore protocol. This one is going to be very heavy on helper methods just due to the nature of how DataStore needs to operate
So far this implements enough for Super Mario Maker, every stock DataStore method I've seen the game use is implemented here. All methods seen in Super Mario Maker have been heavily tested using NintendoClients to verify the functionality against the real server including many edge cases, and I have tried to leave detailed comments in areas that may have confusing handling
Note that this is far from a complete implementation at this time. Some additional methods outside of the ones seen in Super Mario Maker are also implemented, however they are ported from existing servers which use DataStore. These servers are older and the functionality of these methods has not been 100% tested, but it all seems correct
One thing we should also decide on now is how to handle S3. For several methods we need access to the servers S3 bucket. This is currently just set using
SetS3Bucket
. However we also have the option of just having the user pass some configuration parameters and then the common library creates and manages the S3 client. Originally this would not be done in order to give the developer as much control as possible. However this will lead to some, honestly unnecessary, duplicated code between server implementations. The same initialization and helper methods will be defined in basically all servers. And with S3 options now extremely limited, I'm not positive this is a worthwhile thing to support. I'm not a fan of vendor lock-in, but when it comes to DataStore there isn't much of a choice now besides MinIO. Trying to use the major cloud providers is no longer tenable, and I plan to recommend the use of MinIO in all servers which use DataStore in the repo'sREADME
anyway, plus the Super Mario Maker server already just uses the MinIO SDKMoving the S3 client to common (and providing a getter for it for game-specific stuff) would remove the need to set several helper methods as well, however I understand if there is opposition to this idea as it DOES enter the realm of vendor lock-in
I do not think we should control the database layer though, the proposal is only for the S3 layer. We make use of Postgres to keep things uniform across our servers, and that code will be duplicated, but there is a legitimate use case for not controlling that layer here. Other developers are not limited in terms of database solutions as they are with S3 options, so anyone wanting to fork and rehost our servers should always be allowed to swap out Postgres with whatever database solution they use so long as the helper functions are implemented correctly
Currently implements
DataStore::DeleteObject
. Ported from MVDK:TS server. Functionality not 100% verifiedDataStore::GetMeta
. Tested against SMM. Functionality (mostly) verifiedDataStore::GetMetas
. Ported from MVDK:TS server. Functionality not 100% verifiedDataStore::SearchObject
. Ported from SM3DW server. Functionality not 100% verifiedDataStore::RateObject
. Ported from MVDK:TS server. Functionality not 100% verifiedDataStore::PostMetaBinary
. Ported from MVDK:TS server. Functionality not 100% verifiedDataStore::PreparePostObject
. Tested against SMM. Functionality (mostly) verifiedDataStore::PrepareGetObject
. Tested against SMM. Functionality (mostly) verifiedDataStore::CompletePostObject
. Tested against SMM. Functionality (mostly) verifiedDataStore::GetMetasMultipleParam
. Tested against SMM. Functionality (mostly) verifiedDataStore::CompletePostObjects
. Ported from MVDK:TS server. Functionality not 100% verifiedDataStore::ChangeMeta
. Tested against SMM. Functionality (mostly) verifiedDataStore::RateObjects
. Tested against SMM. Functionality (mostly) verified