From 57f89e88df316c91cc4a05e5ca1c1725d2c72533 Mon Sep 17 00:00:00 2001
From: Alexey Yarmosh <yarmoshalexey@gmail.com>
Date: Mon, 10 Jul 2023 19:09:32 +0200
Subject: [PATCH] refactor: get rid of useless sorting

---
 src/lib/geoip/client.ts       | 24 +++---------------------
 test/tests/unit/geoip.test.ts | 29 +++++++++++++++++++++++++----
 2 files changed, 28 insertions(+), 25 deletions(-)

diff --git a/src/lib/geoip/client.ts b/src/lib/geoip/client.ts
index 9e75137f..712c62db 100644
--- a/src/lib/geoip/client.ts
+++ b/src/lib/geoip/client.ts
@@ -53,6 +53,7 @@ export default class GeoipClient {
 			.then(([ ip2location, ipmap, maxmind, ipinfo, fastly ]) => {
 				const fulfilled: (LocationInfoWithProvider | null)[] = [];
 
+				// Providers here are pushed in a desc prioritized order
 				fulfilled.push(
 					ip2location.status === 'fulfilled' ? { ...ip2location.value, provider: 'ip2location' } : null,
 					ipmap.status === 'fulfilled' ? { ...ipmap.value, provider: 'ipmap' } : null,
@@ -147,29 +148,10 @@ export default class GeoipClient {
 	}
 
 	private bestMatch (field: keyof LocationInfo, sources: LocationInfoWithProvider[]): [LocationInfo, LocationInfoWithProvider[]] {
-		const DESC_PRIORITY_OF_PROVIDERS: Provider[] = [ 'ip2location', 'ipmap', 'maxmind', 'ipinfo', 'fastly' ];
 		const filtered = sources.filter(s => s[field]);
-		// Initially sort sources so they are sorted inside groups
-		const sorted = filtered.sort((sourceA, sourceB) => {
-			const indexSourceA = DESC_PRIORITY_OF_PROVIDERS.indexOf(sourceA.provider);
-			const indexSourceB = DESC_PRIORITY_OF_PROVIDERS.indexOf(sourceB.provider);
-			return indexSourceA - indexSourceB;
-		});
 		// Group sources by the same field value
-		const grouped = Object.values(_.groupBy(sorted, field));
-		const ranked = grouped.sort((sourcesA, sourcesB) => {
-			// Move bigger groups to the beginning
-			const groupsSizeDiff = sourcesB.length - sourcesA.length;
-
-			if (groupsSizeDiff === 0) {
-				// If group sizes are equal move the group with the prioritized item to the beginning
-				const smallestIndexSourceA = DESC_PRIORITY_OF_PROVIDERS.indexOf((sourcesA[0] as LocationInfoWithProvider).provider);
-				const smallestIndexSourceB = DESC_PRIORITY_OF_PROVIDERS.indexOf((sourcesA[0] as LocationInfoWithProvider).provider);
-				return smallestIndexSourceA - smallestIndexSourceB;
-			}
-
-			return groupsSizeDiff;
-		}).flat();
+		const grouped = Object.values(_.groupBy(filtered, field));
+		const ranked = grouped.sort((a, b) => b.length - a.length).flat();
 
 		const best = ranked[0];
 
diff --git a/test/tests/unit/geoip.test.ts b/test/tests/unit/geoip.test.ts
index 10b9f225..4b425eae 100644
--- a/test/tests/unit/geoip.test.ts
+++ b/test/tests/unit/geoip.test.ts
@@ -116,10 +116,31 @@ describe('geoip service', () => {
 		});
 	});
 
+	it('should choose top prioritized provider when there is a draw in returned results', async () => {
+		nockGeoIpProviders({ ipmap: 'argentina', ip2location: 'emptyCity', maxmind: 'newYork', ipinfo: 'argentina', fastly: 'newYork' });
+
+		const info = await client.lookup(MOCK_IP);
+
+		expect(info).to.deep.equal({
+			continent: 'SA',
+			country: 'AR',
+			state: undefined,
+			city: 'Buenos Aires',
+			region: 'South America',
+			normalizedRegion: 'south america',
+			normalizedCity: 'buenos aires',
+			asn: 61004,
+			latitude: -34.002,
+			longitude: -58.002,
+			network: 'InterBS S.R.L. (BAEHOST)',
+			normalizedNetwork: 'interbs s.r.l. (baehost)',
+		});
+	});
+
 	it('should choose top prioritized provider when all providers returned different cities', async () => {
 		nockGeoIpProviders({ ipmap: 'default', ip2location: 'argentina', maxmind: 'newYork', ipinfo: 'emptyCity', fastly: 'bangkok' });
 
-		const info = await client.lookup(MOCK_IP).catch((error: Error) => error);
+		const info = await client.lookup(MOCK_IP);
 
 		expect(info).to.deep.equal({
 			continent: 'SA',
@@ -359,7 +380,7 @@ describe('geoip service', () => {
 		it('should pass - non-vpn', async () => {
 			nockGeoIpProviders();
 
-			const response: LocationInfo | Error = await client.lookup(MOCK_IP).catch((error: Error) => error);
+			const response: LocationInfo | Error = await client.lookup(MOCK_IP);
 
 			expect(response).to.deep.equal({
 				continent: 'NA',
@@ -380,7 +401,7 @@ describe('geoip service', () => {
 		it('should pass - no client object', async () => {
 			nockGeoIpProviders({ fastly: 'noClient' });
 
-			const response: LocationInfo | Error = await client.lookup(MOCK_IP).catch((error: Error) => error);
+			const response: LocationInfo | Error = await client.lookup(MOCK_IP);
 
 			expect(response).to.deep.equal({
 				continent: 'NA',
@@ -409,7 +430,7 @@ describe('geoip service', () => {
 
 			nockGeoIpProviders({ fastly: 'proxyDescVpn' });
 
-			const response: LocationInfo | Error = await client.lookup(MOCK_IP).catch((error: Error) => error);
+			const response: LocationInfo | Error = await client.lookup(MOCK_IP);
 
 			expect(response).to.deep.equal({
 				continent: 'NA',