8000 File-based DB support implementation is not usable out-of-the-box and deviates from the mongo implementation · Issue #948 · finos/git-proxy · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

File-based DB support implementation is not usable out-of-the-box and deviates from the mongo implementation #948

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.

Sign up for GitHub

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

Jump to bottom
Open
kriswest opened this issue Mar 24, 2025 · 3 comments · May be fixed by #979
Open

File-based DB support implementation is not usable out-of-the-box and deviates from the mongo implementation #948

kriswest opened this issue Mar 24, 2025 · 3 comments · May be fixed by #979

Comments

@kriswest
Copy link
Contributor

Describe the bug
There appear to be a number of flaws in the file-based DB implementation that impede its use. These include some small deviations from the mongo implementation AND some deviations in behaviotr between @seald/nedb and mongo that should be handled in the implementation.

  1. lowercasing of inputs (particularly of usernames and emails) is inconsistently applied and differs from mongo implementation. In places, inputs are lowercased before being sent to the database client and hence apply to both client implementations, however, the mongo clients applies lowercasing on its own in other situations. The database clients should consistently lowercase inputs where appropriate (in both record creation and query) to ensure consistent application. Example of deviation (there are many more cases):

    • Mongo:

      git-proxy/src/db/mongo/repo.js

      Lines 41 to 45 in 5d24d9d

      exports.addUserCanPush = async (name, user) => {
      name = name.toLowerCase();
      const collection = await connect(cnName);
      await collection.updateOne({ name: name }, { $push: { 'users.canPush': user } });
      };
    • File:

      git-proxy/src/db/file/repo.js

      Lines 54 to 73 in 5d24d9d

      exports.addUserCanPush = async (name, user) => {
      return new Promise(async (resolve, reject) => {
      const repo = await exports.getRepo(name);
      if (repo.users.canPush.includes(user)) {
      resolve(null);
      return;
      }
      repo.users.canPush.push(user);
      const options = { multi: false, upsert: false };
      db.update({ name: name }, repo, options, (err) => {
      if (err) {
      reject(err);
      } else {
      resolve(null);
      }
      });
      });
      };

  2. Queries are being ignored in user retrieval, causing all users to be retrieved and push parsing to fail. Query being ignored:

    git-proxy/src/db/file/users.js

    Lines 78 to 81 in 5d24d9d

    exports.getUsers = function (query) {
    if (!query) query = {};
    return new Promise((resolve, reject) => {
    db.find({}, (err, docs) => {

    (note that query is not passed in on last nline)
    Push parsing that requires DB filtering of users that is not happening:

    git-proxy/src/proxy/processors/push-action/checkUserPushPermission.js

    Lines 13 to 20 in 5d24d9d

    const list = await db.getUsers({ gitAccount: action.user });
    console.log(JSON.stringify(list));
    if (list.length == 1) {
    user = list[0].username;
    isUserAllowed = await db.isUserPushAllowed(repoName, user);
    }

    (note exactly one user must be found on line 17 - yet we are not actually filtering users)

  3. The implementation does not merge records on update, but replaces them. This causes fields not supplied in the update to be dropped (e.g. if a user is updated on login via an SSO integration, then the Github user name is dropped as that is manually entered later).

    git-proxy/src/db/file/users.js

    Lines 65 to 76 in 5d24d9d

    exports.updateUser = function (user) {
    return new Promise((resolve, reject) => {
    const options = { multi: false, upsert: false };
    db.update({ username: user.username }, user, options, (err) => {
    if (err) {
    reject(err);
    } else {
    resolve(null);
    }
    });
    });
    };

    (this looks correct by @seald/nedb does not actually merge records when called like this - it finds the existing record and replaces it with the user passed in. This needs rewriting to either use the $set keyword or to manually retrieve the existing doc and merge into before calling db.update.

  4. neDB implementations should be set to compact their data files periodically, unless they are restarted regularly (compacts on read) - neDB uses an append-only file format so it will create many duplicate lines, See https://github.com/seald/nedb?tab=readme-ov-file#persistence. Add calls to set-up compaction to avoid infinite growth and very redundant data files.

  5. No indexes are applied, which will result in performance losses as the dataset grows. Add calls to create indexes supporting common queries - these calls should be made on every start-up. See: https://github.com/seald/nedb?tab=readme-ov-file#indexing for details
    Expected behavior
    The behaviour of the file-based DB support should match that of the mongo implementation and it should work out-of-the-box.

Additional context
I'll raise a PR to address these issues in the implementation.
@grovesy
Copy link
Member
grovesy commented Mar 24, 2025 •
edited
Loading

Might be worth noting that the original intention of the file based DB was for local development - without the need for additional infrastructure.

i.e. it's not what I would call enterprise grade and not intended to be so

It could be worth considering dropping it entirely?

@kriswest
Copy link
Contributor Author

@grovesy understood on the intention, it remains useful for that purpose and I can see others using it. As we've just about got it working here I'd be happy to contribute the fixes to bring it up to spec (although perhaps not to an enterprise grade ;-) ).

@grovesy
Copy link
Member
grovesy commented Mar 24, 2025

@kriswest Sounds good to me!

@kriswest kriswest linked a pull request Apr 15, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: neDB implementation issues kriswest/git-proxy
2 participants
@kriswest @grovesy
0