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

Datastorage is semi-hardcoded for accounts #96

Open
Zuzuesque opened this issue Aug 5, 2019 · 4 comments
Open

Datastorage is semi-hardcoded for accounts #96

Zuzuesque opened this issue Aug 5, 2019 · 4 comments
Milestone

Comments

@Zuzuesque
Copy link

While the documentation suggests it is enough to switch out the EntityLoader to implement your custom data source, this is currently not entirely true for accounts. You can load them from any alternate source, but the only proper method for creating/saving them is calling save() on the Account object (as done by the example bundles). Save() on Account.js in turn calles the save() function on Data.js, which currently looks like this:

 /**
   * Save data file (player/account) data to disk
   * @param {string} type
   * @param {string} id
   * @param {*} data
   * @param {function} callback
   */
  static save(type, id, data, callback) {
    fs.writeFileSync(this.getDataFilePath(type, id), JSON.stringify(data, null, 2), 'utf8');
    if (callback) {
      callback();
    }
  }

... regardless of the user implementation for the data source.

Work Around
To implement custom sources for accounts there are currently two ways--switching out Account.js with one imlementing a custom save() function (and consequently AccountManager which depends on it)--or to access the loader stored in AccountManager directly in your code via Accountmanager.loader.custom_loader_save_function(). The first is somewhat overkill if you have no intention of changing the fields/pw encryption Account provides while the later leaves it to the developer to make sure the loader they access from the Accountmanager is the one they hooked up and feels unintuitive.

Proposed Solution:

  • Add a create/save function to the EntityLoader (similar to the already existing update and replace functions), surrendering full CRUD control to the loader and consequently user implementation.
  • Add a save() function to AccountManager which takes an Account object, serializes it and passes the data on to the loader for handling--similar to save(player) on the PlayerManager.
  • Update Account.js to remove all calls to this.save() which currently happens in all functions changing account data. Instead call save explicitely outside of account, bringing it inline with how player requires explicit saving.
  • Update example bundles to do AccountManager.save(account) rather than account.save().
  • Deprecate/dissuade use of account.save(), but leave it in for backwards compatibility?

Optional Consideration:
EntityLoader currently has implementations for update/replace which are not used by AccountManager and inaccessible unless using the loader directly. There is also no delete function for cases when you do want to delete an account properly rather than just setting account.delete to true. It may be an interesting feature to add the full set of CRUD functions to the AccountManager so it can serve as a gateway to accessing all loader functions for usecases where people really only want to change how data is saved/loaded, but don't need custom implementations for accounts.

@shawncplus
Copy link
Member

shawncplus commented Aug 5, 2019

Add a create/save function to the EntityLoader (similar to the already existing update and replace functions),

There's no need for 4 methods. CRU can all be accomplished with the current EntityLoader architecture. It might need a remove added though

--

To all the rest that's the plan, it was mentioned in another ticket but the Data class serves no good purpose now with the EntityLoader system.

@shawncplus shawncplus added this to the 3.2 milestone Aug 22, 2019
@NuSkooler
Copy link

Any ETA on 3.2 where this will be fixed? I just ran into this when attempting to make case insensitive JSON based data/directory source (e.g. case insensitive account & player names/lookup).

@azigler
Copy link

azigler commented Jul 18, 2020

You can fix this by appending the AccountManager to Account when adding it to the manager (I used the this.__manager to be consistent with Item). Then change the Account.save() method like so:

/**
   * @param {function} callback after-save callback
   */
  save(callback) {
    this.__manager.loader.update(this.username, this.serialize())
      .then(() => callback());
  }

azigler added a commit to azigler/core that referenced this issue Jul 19, 2020
@NuSkooler
Copy link

NuSkooler commented Jan 10, 2021

@azigler Can you give a little more detail on what you're describing above? I looked at the commit referenced here but that doesn't seem to be enough to make it work.

EDIT: Wait, I think you just mean monkey patch in __manager, e.g.:

args.account.__manager = state.AccountManager;

?

hatertron3000 added a commit to hatertron3000/core that referenced this issue Nov 26, 2021
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

No branches or pull requests

4 participants