Skip to content
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

Merged
merged 25 commits into from
Oct 31, 2023
Merged

Add common DataStore protocol #24

merged 25 commits into from
Oct 31, 2023

Conversation

jonbarrow
Copy link
Member

@jonbarrow jonbarrow commented Oct 29, 2023

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's README anyway, plus the Super Mario Maker server already just uses the MinIO SDK

Moving 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% verified
  • DataStore::GetMeta. Tested against SMM. Functionality (mostly) verified
  • DataStore::GetMetas. Ported from MVDK:TS server. Functionality not 100% verified
  • DataStore::SearchObject. Ported from SM3DW server. Functionality not 100% verified
  • DataStore::RateObject. Ported from MVDK:TS server. Functionality not 100% verified
  • DataStore::PostMetaBinary. Ported from MVDK:TS server. Functionality not 100% verified
  • DataStore::PreparePostObject. Tested against SMM. Functionality (mostly) verified
  • DataStore::PrepareGetObject. Tested against SMM. Functionality (mostly) verified
  • DataStore::CompletePostObject. Tested against SMM. Functionality (mostly) verified
  • DataStore::GetMetasMultipleParam. Tested against SMM. Functionality (mostly) verified
  • DataStore::CompletePostObjects. Ported from MVDK:TS server. Functionality not 100% verified
  • DataStore::ChangeMeta. Tested against SMM. Functionality (mostly) verified
  • DataStore::RateObjects. Tested against SMM. Functionality (mostly) verified

@jonbarrow jonbarrow marked this pull request as ready for review October 29, 2023 16:02
@jonbarrow
Copy link
Member Author

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

Copy link
Member

@DaniElectra DaniElectra left a 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:

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

datastore/complete_post_object.go Outdated Show resolved Hide resolved
datastore/complete_post_object.go Outdated Show resolved Hide resolved
datastore/complete_post_object.go Outdated Show resolved Hide resolved
datastore/prepare_get_object.go Outdated Show resolved Hide resolved
datastore/search_object.go Outdated Show resolved Hide resolved
datastore/get_metas.go Outdated Show resolved Hide resolved
@jonbarrow
Copy link
Member Author

Using the MinIO SDK shouldn't affect implementations that want to use another server since they all use S3

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

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

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

we should consider integrating HPP here aswell

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 nex-go, so I'm unsure what changes need to be made to bring support here

MK7 will use V1 versions of methods

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

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)

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

@DaniElectra
Copy link
Member

What all work needs to be done for this? I haven't looked much into HPP, even the implementation in nex-go, so I'm unsure what changes need to be made to bring support here

I was thinking about this and I think we can't differentiate a NEX packet from an HPP packet right now? On nex-protocols-go we are checking the packet type, but we don't have access to that here (and there is no param on the server right now that differentiates one from another). I think we could add a boolean on nex-go to check this?

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.

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

This is over NEX on PRUDP, Mario Kart 7 uses methods like PreparePostObjectV1 working the same as the normal method.

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

Sure, we can add a note about this

@jonbarrow
Copy link
Member Author

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.

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 Sender() method instead, and then we have full access to all the context we've been missing

This is over NEX on PRUDP, Mario Kart 7 uses methods like PreparePostObjectV1 working the same as the normal method.

Gotcha gotcha

@jonbarrow
Copy link
Member Author

@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 Sender() method, and it would allow us to:

  • Seamlessly support HPP from what I can tell, with no further changes to either the protocols or common libs
  • Stop hard-coding the destination and source addresses in preparation for QRV support

I know it would make the function signatures look a bit goofy, but I'm thinking it's a worthwhile change considering the benefits

@DaniElectra
Copy link
Member

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 Sender() method, and it would allow us to:

* Seamlessly support HPP from what I can tell, with no further changes to either the protocols or common libs

* Stop hard-coding the destination and source addresses in preparation for QRV support

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!

@jonbarrow
Copy link
Member Author

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 Sender() method, and it would allow us to:

* Seamlessly support HPP from what I can tell, with no further changes to either the protocols or common libs
* Stop hard-coding the destination and source addresses in preparation for QRV support

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?

datastore/protocol.go Show resolved Hide resolved
datastore/protocol.go Show resolved Hide resolved
datastore/complete_post_object.go Show resolved Hide resolved
datastore/protocol.go Outdated Show resolved Hide resolved
Copy link
Member

@DaniElectra DaniElectra left a 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

@jonbarrow jonbarrow merged commit 2a06105 into main Oct 31, 2023
@binaryoverload binaryoverload deleted the datastore branch January 9, 2025 15:09
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.

2 participants