refactor: remove deprecated url.parse() method

[cbaker6]

New Pull Request Checklist

• I am not disclosing a vulnerability.
• I am creating this PR in reference to an issue.

Issue Description

The server uses the deprecated url.parse() method. Close #7762

Related issue: #7762

Approach

I switched all of the server to use new URL() and updated relevant tests.

TODOs before merging

• Add tests
• Change url.parse to new URL
• A changelog entry is created automatically using the pull request title (do not manually add a changelog entry)

[parse-github-assistant]

Thanks for opening this pull request!

• 🎉 We are excited about your hands-on contribution!

[parse-github-assistant]

Thanks for opening this pull request!

• ❌ Please check all required checkboxes at the top, otherwise your pull request will be closed.

• ⚠️ Remember that a security vulnerability must only be reported confidentially, see our Security Policy. If you are not sure whether the issue is a security vulnerability, the safest way is to treat it as such and submit it confidentially to us for evaluation.

[parse-github-assistant]

Thanks for opening this pull request!

• 🎉 We are excited about your hands-on contribution!

[codecov]

Codecov Report

Merging #7751 (5d23976) into alpha (a43638f) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            alpha    #7751      +/-   ##
==========================================
+ Coverage   93.95%   93.97%   +0.01%     
==========================================
  Files         183      183              
  Lines       13660    13655       -5     
==========================================
- Hits        12834    12832       -2     
+ Misses        826      823       -3     

Impacted Files	Coverage Δ
src/Adapters/Auth/gcenter.js	98.24% <100.00%> (+0.03%)	⬆️
src/Adapters/Auth/oauth2.js	100.00% <100.00%> (ø)
.../Adapters/Storage/Postgres/PostgresConfigParser.js	100.00% <100.00%> (ø)
src/Controllers/LoggerController.js	91.57% <100.00%> (ø)
src/Controllers/index.js	97.75% <100.00%> (-0.03%)	⬇️
src/ParseServerRESTController.js	96.96% <100.00%> (-0.05%)	⬇️
src/batch.js	94.73% <100.00%> (+3.35%)	⬆️
src/Adapters/Files/GridFSBucketAdapter.js	80.32% <0.00%> (+0.81%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a43638f...5d23976. Read the comment docs.

[cbaker6]

@davimacedo while working on this PR, I cleaned up the ParseServerRESTController.spec.js and ran into a problem with one of the transactions tests for mongo. I noticed in #5849 (comment) you mentioned mongo needed a replicaset when using transactions.

The problem I ran into is the extra reconfiguration of the server:

parse-server/spec/ParseServerRESTController.spec.js

Lines 348 to 349 in 191d80b

	it('should generate separate session for each call', async () => {
	await reconfigureServer();

seems to override the one configuration that uses the replicaSet:

parse-server/spec/ParseServerRESTController.spec.js

Lines 171 to 183 in 191d80b

	describe('transactions', () => {
	beforeEach(async () => {
	await TestUtils.destroyAllDataPermanently(true);
	if (
	semver.satisfies(process.env.MONGODB_VERSION, '>=4.0.4') &&
	process.env.MONGODB_TOPOLOGY === 'replicaset' &&
	process.env.MONGODB_STORAGE_ENGINE === 'wiredTiger'
	) {
	await reconfigureServer({
	databaseAdapter: undefined,
	databaseURI:
	'mongodb://localhost:27017/parseServerMongoAdapterTestDatabase?replicaSet=replicaset',
	});

When I remove the extra reconfigure and use the replicaset one, the following failures occur on Mongo (the Mongo CI that doesn't use replicaSet seems to pass in any case):

1) ParseServerRESTController transactions should generate separate session for each call
  - Expected false to equal true.
  - Expected false to equal true.
  - Expected false to equal true.
  - Expected false to equal true.

Note that removing the extra reconfigure on this test seems to work:

parse-server/spec/ParseServerRESTController.spec.js

Lines 189 to 190 in 191d80b

	it('should handle a batch request with transaction = true', async done => {
	await reconfigureServer();

My initial thoughts are the extra reconfigure shouldn't be there and the tests might be catching an actual bug. I don't know enough about Mongo to know if separate sessions are required for transactions.

Also, it looks like the exact set of transaction tests are in ParseServerRESTController.spec.js and batch.spec.js (the same problem I mentioned above exists in both). The difference I see is one is using the ParseServerRESTController in tests. It seems both tests randomly fail with connection issues (I'm not able to replicate this locally) on Postgres (so I've disabled the RESTController transaction tests on Postgres to reduce the amount of random failures in the CI).

parse-server/spec/batch.spec.js

Lines 170 to 590 in 4c29d4d

	if (
	(semver.satisfies(process.env.MONGODB_VERSION, '>=4.0.4') &&
	process.env.MONGODB_TOPOLOGY === 'replicaset' &&
	process.env.MONGODB_STORAGE_ENGINE === 'wiredTiger') ||
	process.env.PARSE_SERVER_TEST_DB === 'postgres'
	) {
	describe('transactions', () => {
	beforeEach(async () => {
	await TestUtils.destroyAllDataPermanently(true);
	if (
	semver.satisfies(process.env.MONGODB_VERSION, '>=4.0.4') &&
	process.env.MONGODB_TOPOLOGY === 'replicaset' &&
	process.env.MONGODB_STORAGE_ENGINE === 'wiredTiger'
	) {
	await reconfigureServer({
	databaseAdapter: undefined,
	databaseURI:
	'mongodb://localhost:27017/parseServerMongoAdapterTestDatabase?replicaSet=replicaset',
	});
	} else {
	await reconfigureServer();
	}
	});
	it('should handle a batch request with transaction = true', async done => {
	await reconfigureServer();
	const myObject = new Parse.Object('MyObject'); // This is important because transaction only works on pre-existing collections
	myObject
	.save()
	.then(() => {
	return myObject.destroy();
	})
	.then(() => {
	spyOn(databaseAdapter, 'createObject').and.callThrough();
	request({
	method: 'POST',
	headers: headers,
	url: 'http://localhost:8378/1/batch',
	body: JSON.stringify({
	requests: [
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 'value1' },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 'value2' },
	},
	],
	transaction: true,
	}),
	}).then(response => {
	expect(response.data.length).toEqual(2);
	expect(response.data[0].success.objectId).toBeDefined();
	expect(response.data[0].success.createdAt).toBeDefined();
	expect(response.data[1].success.objectId).toBeDefined();
	expect(response.data[1].success.createdAt).toBeDefined();
	const query = new Parse.Query('MyObject');
	query.find().then(results => {
	expect(databaseAdapter.createObject.calls.count() % 2).toBe(0);
	for (let i = 0; i + 1 < databaseAdapter.createObject.calls.length; i = i + 2) {
	expect(databaseAdapter.createObject.calls.argsFor(i)[3]).toBe(
	databaseAdapter.createObject.calls.argsFor(i + 1)[3]
	);
	}
	expect(results.map(result => result.get('key')).sort()).toEqual([
	'value1',
	'value2',
	]);
	done();
	});
	});
	});
	});
	it('should not save anything when one operation fails in a transaction', async () => {
	const myObject = new Parse.Object('MyObject'); // This is important because transaction only works on pre-existing collections
	await myObject.save();
	await myObject.destroy();
	try {
	await request({
	method: 'POST',
	headers: headers,
	url: 'http://localhost:8378/1/batch',
	body: JSON.stringify({
	requests: [
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 'value1' },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 10 },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 'value1' },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 10 },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 'value1' },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 10 },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 'value1' },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 10 },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 'value1' },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 10 },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 'value1' },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 10 },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 'value1' },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 10 },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 'value1' },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 10 },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 'value1' },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 10 },
	},
	],
	transaction: true,
	}),
	});
	} catch (error) {
	expect(error).toBeDefined();
	const query = new Parse.Query('MyObject');
	const results = await query.find();
	expect(results.length).toBe(0);
	}
	});
	it('should generate separate session for each call', async () => {
	await reconfigureServer();
	const myObject = new Parse.Object('MyObject'); // This is important because transaction only works on pre-existing collections
	await myObject.save();
	await myObject.destroy();
	const myObject2 = new Parse.Object('MyObject2'); // This is important because transaction only works on pre-existing collections
	await myObject2.save();
	await myObject2.destroy();
	spyOn(databaseAdapter, 'createObject').and.callThrough();
	let myObjectCalls = 0;
	Parse.Cloud.beforeSave('MyObject', async () => {
	myObjectCalls++;
	if (myObjectCalls === 2) {
	try {
	await request({
	method: 'POST',
	headers: headers,
	url: 'http://localhost:8378/1/batch',
	body: JSON.stringify({
	requests: [
	{
	method: 'POST',
	path: '/1/classes/MyObject2',
	body: { key: 'value1' },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject2',
	body: { key: 10 },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject2',
	body: { key: 'value1' },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject2',
	body: { key: 10 },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject2',
	body: { key: 'value1' },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject2',
	body: { key: 10 },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject2',
	body: { key: 'value1' },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject2',
	body: { key: 10 },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject2',
	body: { key: 'value1' },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject2',
	body: { key: 10 },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject2',
	body: { key: 'value1' },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject2',
	body: { key: 10 },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject2',
	body: { key: 'value1' },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject2',
	body: { key: 10 },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject2',
	body: { key: 'value1' },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject2',
	body: { key: 10 },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject2',
	body: { key: 'value1' },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject2',
	body: { key: 10 },
	},
	],
	transaction: true,
	}),
	});
	fail('should fail');
	} catch (e) {
	expect(e).toBeDefined();
	}
	}
	});
	const response = await request({
	method: 'POST',
	headers: headers,
	url: 'http://localhost:8378/1/batch',
	body: JSON.stringify({
	requests: [
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 'value1' },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject',
	body: { key: 'value2' },
	},
	],
	transaction: true,
	}),
	});
	expect(response.data.length).toEqual(2);
	expect(response.data[0].success.objectId).toBeDefined();
	expect(response.data[0].success.createdAt).toBeDefined();
	expect(response.data[1].success.objectId).toBeDefined();
	expect(response.data[1].success.createdAt).toBeDefined();
	await request({
	method: 'POST',
	headers: headers,
	url: 'http://localhost:8378/1/batch',
	body: JSON.stringify({
	requests: [
	{
	method: 'POST',
	path: '/1/classes/MyObject3',
	body: { key: 'value1' },
	},
	{
	method: 'POST',
	path: '/1/classes/MyObject3',
	body: { key: 'value2' },
	},
	],
	}),
	});
	const query = new Parse.Query('MyObject');
	const results = await query.find();
	expect(results.map(result => result.get('key')).sort()).toEqual(['value1', 'value2']);
	const query2 = new Parse.Query('MyObject2');
	const results2 = await query2.find();
	expect(results2.length).toEqual(0);
	const query3 = new Parse.Query('MyObject3');
	const results3 = await query3.find();
	expect(results3.map(result => result.get('key')).sort()).toEqual(['value1', 'value2']);
	expect(databaseAdapter.createObject.calls.count() >= 13).toEqual(true);
	let transactionalSession;
	let transactionalSession2;
	let myObjectDBCalls = 0;
	let myObject2DBCalls = 0;
	let myObject3DBCalls = 0;
	for (let i = 0; i < databaseAdapter.createObject.calls.count(); i++) {
	const args = databaseAdapter.createObject.calls.argsFor(i);
	switch (args[0]) {
	case 'MyObject':
	myObjectDBCalls++;
	if (!transactionalSession || (myObjectDBCalls - 1) % 2 === 0) {
	transactionalSession = args[3];
	} else {
	expect(transactionalSession).toBe(args[3]);
	}
	if (transactionalSession2) {
	expect(transactionalSession2).not.toBe(args[3]);
	}
	break;
	case 'MyObject2':
	myObject2DBCalls++;
	if (!transactionalSession2 || (myObject2DBCalls - 1) % 9 === 0) {
	transactionalSession2 = args[3];
	} else {
	expect(transactionalSession2).toBe(args[3]);
	}
	if (transactionalSession) {
	expect(transactionalSession).not.toBe(args[3]);
	}
	break;
	case 'MyObject3':
	myObject3DBCalls++;
	expect(args[3]).toEqual(null);
	break;
	}
	}
	expect(myObjectDBCalls % 2).toEqual(0);
	expect(myObjectDBCalls > 0).toEqual(true);
	expect(myObject2DBCalls % 9).toEqual(0);
	expect(myObject2DBCalls > 0).toEqual(true);
	expect(myObject3DBCalls % 2).toEqual(0);
	expect(myObject3DBCalls > 0).toEqual(true);
	});
	});
	}
	});

[mtrezza]

This PR seems to be contain many changes that are unrelated to "feat: support postgresql in database uri". This could make debugging difficult in the future. Could you focus this PR on what the issue and PR describe (the "postgresql" protocol) and move unrelated changes to a separate PR?

[cbaker6]

The postgresql addition is literally a one liner for a fall through case statement. The additional part of the PR removes the deprecated url.parse, switches some testcases to async/await, and reduces the amount of flaky tests. If there are questions, I can address them, but it's not a big PR. Switching from the old promise style to async await is most of the line changes.

If you look at the codecov diff, it's easier to see the changes to the server files. The testcases are easier to see looking at the GitHub Files Changed

[mtrezza]

Even better if it's a one-line then we can merge it easily. Mixing unrelated issues in a single PR is something we really want to avoid. We are asking every contributor to follow that because in case of bugs it becomes much more difficult and time intensive for everyone to investigate. I know it can seem to be a nuisance sometimes.

[cbaker6]

We are asking every contributor to follow that because in case of bugs it becomes much more difficult and time intensive for everyone to investigate.

You have also been asking every contributor to help with upgrading the tests to async/await. I've already split the PR's I've worked on into 4 to make it easier. Removing the one liner will still make this PR look almost the same, which will require a similar review

[mtrezza]

I appreciate the work you put into fixing the tests. Could you please remove unrelated changes from this PR. The basic practice we ask everyone to follow is to not mix unrelated issues in one PR for obvious reasons. Remember that we are also using release automation now that is based on distinct PR scopes.

If you have to modify a couple of test cases for a PR anyway and in the same go you refactor them using async/await, that should be fine. If you modify a lot of tests or unrelated tests, then you could do that in a separate PR, like has been done in #7564. This PR contains a lot of different things, like this new test it('validateAuthData invalid public key http url', async () => { where it is difficult to say what it relates to and why it was added.

[cbaker6]

These tests are added because the design of batch.makeBatchRoutingPathFunction attempts to handle this, but these two added test fail with the url.parse implementation of the code.

[mtrezza]

I guess let's merge all other split-off PRs first and then see the diff here, to make sure this PR has the right scope.

[mtrezza]

Looks good!

[parseplatformorg]

🎉 This change has been released in version 5.0.0-alpha.17

[parseplatformorg]

🎉 This change has been released in version 5.0.0-beta.10

[parseplatformorg]

🎉 This change has been released in version 5.1.0