fix: proxy preparations mismatch bug

[jescalada]

Fixes #1282
Fixes #1278

[netlify]

✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name	Link
🔨 Latest commit	3afa917
🔍 Latest deploy log	https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/692a974121db46000820bd30

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.35%. Comparing base (85e418f) to head (3afa917).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1284   +/-   ##
=======================================
  Coverage   83.35%   83.35%           
=======================================
  Files          70       70           
  Lines        3004     3004           
  Branches      499      499           
=======================================
  Hits         2504     2504           
  Misses        397      397           
  Partials      103      103           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[kriswest]

Looks good to me but could include a test... The tests in testProxyRoute are closest to this...

E.g. the before at

git-proxy/test/testProxyRoute.test.js

Lines 408 to 439 in 85e418f

	// if our default repo is not set-up, create it
	const repo = await db.getRepoByUrl(TEST_DEFAULT_REPO.url);
	if (!repo) {
	const res2 = await chai
	.request(apiApp)
	.post('/api/v1/repo')
	.set('Cookie', `${cookie}`)
	.send(TEST_DEFAULT_REPO);
	res2.should.have.status(200);
	}
	});
	after(async () => {
	sinon.restore();
	await service.stop();
	await proxy.stop();
	await cleanupRepo(TEST_DEFAULT_REPO.url);
	await cleanupRepo(TEST_GITLAB_REPO.url);
	});
	it('should proxy requests for the default GitHub repository', async function () {
	// proxy a fetch request
	const res = await chai
	.request(proxy.getExpressApp())
	.get(`${TEST_DEFAULT_REPO.proxyUrlPrefix}/info/refs?service=git-upload-pack`)
	.set('user-agent', 'git/2.42.0')
	.set('accept', 'application/x-git-upload-pack-request')
	.buffer();
	expect(res.status).to.equal(200);
	expect(res.text).to.contain('git-upload-pack');
	});

is actually creating the default repo, where we could instead make sure its gone and then check proxyPreparations is creating it, and then not duplicating (with different project and name, but same URL) it if started again?

[jescalada]

@kriswest Rather than modifying the before logic, I figured we could just remove the recently added repo and check if the restart adds it correctly and doesn't add an extra one upon re-restart:

  it('should create the default repo if it does not exist', async function () {
    // Remove the default repo from the db and check it no longer exists
    await cleanupRepo(TEST_DEFAULT_REPO.url);

    const repo = await db.getRepoByUrl(TEST_DEFAULT_REPO.url);
    expect(repo).to.be.null;

    // Restart the proxy
    await proxy.stop();
    await proxy.start();

    // Check that the default repo was created in the db
    const repo2 = await db.getRepoByUrl(TEST_DEFAULT_REPO.url);
    expect(repo2).to.not.be.null;

    // Check that the default repo isn't duplicated on subsequent restarts
    await proxy.stop();
    await proxy.start();

    const repo3 = await db.getRepoByUrl(TEST_DEFAULT_REPO.url);
    expect(repo3).to.not.be.null;
    expect(repo3._id).to.equal(repo2._id);
  });

Does this look correct? 🤔

[kriswest]

@jescalada I think that approach to a test is valid, however this bit at the end may not be right:

 const repo3 = await db.getRepoByUrl(TEST_DEFAULT_REPO.url);
    expect(repo3).to.not.be.null;
    expect(repo3._id).to.equal(repo2._id);

Who's to say that it doesn't return the first one that was created, meaning it will always pass even if a duplicate was created?

Use getRepos instead and filter the list on the repo URL. If you find more or less than 1 repo its a fail.

[jescalada]

@kriswest Updated the test as follows:

const allRepos = await db.getRepos();
const matchingRepos = allRepos.filter((r) => r.url === TEST_DEFAULT_REPO.url);

expect(matchingRepos).to.have.length(1);

Will be merging this to convert the test to Vitest and speed up the merge process on that PR 😃