From 45c141e9791ad353e91a42272ceb47457e553abc Mon Sep 17 00:00:00 2001
From: Alexey Yarmosh <yarmoshalexey@gmail.com>
Date: Mon, 10 Jul 2023 13:53:54 +0200
Subject: [PATCH] fix: make geoip client work even if redis is broken

---
 src/lib/geoip/client.ts       | 22 +++++++-------
 test/tests/unit/geoip.test.ts | 55 +++++++++++++++++++++++++++++++----
 2 files changed, 60 insertions(+), 17 deletions(-)

diff --git a/src/lib/geoip/client.ts b/src/lib/geoip/client.ts
index 3c862fbe..27410913 100644
--- a/src/lib/geoip/client.ts
+++ b/src/lib/geoip/client.ts
@@ -1,6 +1,5 @@
 import _ from 'lodash';
 import config from 'config';
-import type { Logger } from 'winston';
 import newrelic from 'newrelic';
 import type { CacheInterface } from '../cache/cache-interface.js';
 import { InternalError } from '../internal-error.js';
@@ -27,16 +26,12 @@ export type NetworkInfo = {
 	asn: number;
 };
 
-export const createGeoipClient = (): GeoipClient => new GeoipClient(
-	new RedisCache(getRedisClient()),
-	scopedLogger('geoip'),
-);
+const logger = scopedLogger('geoip');
+
+export const createGeoipClient = (): GeoipClient => new GeoipClient(new RedisCache(getRedisClient()));
 
 export default class GeoipClient {
-	constructor (
-		private readonly cache: CacheInterface,
-		private readonly logger: Logger,
-	) {}
+	constructor (private readonly cache: CacheInterface) {}
 
 	async lookup (addr: string): Promise<ProbeLocation> {
 		const results = await Promise
@@ -155,7 +150,7 @@ export default class GeoipClient {
 		}
 
 		if (!best || best.provider === 'fastly') {
-			this.logger.error(`failed to find a correct value for a field "${field}"`, { field, sources });
+			logger.error(`failed to find a correct value for a field "${field}"`, { field, sources });
 			throw new Error(`failed to find a correct value for a field "${field}"`);
 		}
 
@@ -163,7 +158,10 @@ export default class GeoipClient {
 	}
 
 	public async lookupWithCache<T> (key: string, fn: () => Promise<T>): Promise<T> {
-		const cached = await this.cache.get<T>(key);
+		const cached = await this.cache.get<T>(key).catch((error: Error) => {
+			logger.error('Failed to get cached geoip info for probe.', error);
+			newrelic.noticeError(error, { key });
+		});
 
 		if (cached) {
 			return cached;
@@ -173,7 +171,7 @@ export default class GeoipClient {
 		const ttl = Number(config.get('geoip.cache.ttl'));
 
 		await this.cache.set(key, info, ttl).catch((error: Error) => {
-			this.logger.error('Failed to cache geoip info for probe.', error);
+			logger.error('Failed to cache geoip info for probe.', error);
 			newrelic.noticeError(error, { key, ttl });
 		});
 
diff --git a/test/tests/unit/geoip.test.ts b/test/tests/unit/geoip.test.ts
index 1aa72073..06b8c1d9 100644
--- a/test/tests/unit/geoip.test.ts
+++ b/test/tests/unit/geoip.test.ts
@@ -6,8 +6,8 @@ import type { LocationInfo } from '../../../src/lib/geoip/client.js';
 import { fastlyLookup } from '../../../src/lib/geoip/providers/fastly.js';
 import GeoipClient from '../../../src/lib/geoip/client.js';
 import NullCache from '../../../src/lib/cache/null-cache.js';
-import { scopedLogger } from '../../../src/lib/logger.js';
 import { populateMemList } from '../../../src/lib/geoip/whitelist.js';
+import type { CacheInterface } from '../../../src/lib/cache/cache-interface.js';
 
 const mocks = JSON.parse(fs.readFileSync('./test/mocks/nock-geoip.json').toString()) as Record<string, any>;
 
@@ -19,16 +19,61 @@ describe('geoip service', () => {
 	before(async () => {
 		await populateMemList();
 
-		client = new GeoipClient(
-			new NullCache(),
-			scopedLogger('geoip:test'),
-		);
+		client = new GeoipClient(new NullCache());
 	});
 
 	afterEach(() => {
 		nock.cleanAll();
 	});
 
+	describe('GeoipClient', () => {
+		it('should work even if cache is failing', async () => {
+			class BrokenCache implements CacheInterface {
+				async delete (): Promise<undefined> {
+					throw new Error('delete is broken');
+				}
+
+				async get (): Promise<undefined> {
+					throw new Error('get is broken');
+				}
+
+				async set (): Promise<void> {
+					throw new Error('set is broken');
+				}
+			}
+
+			const clientWithBrokenCache = new GeoipClient(new BrokenCache());
+			nock('https://globalping-geoip.global.ssl.fastly.net')
+				.get(`/${MOCK_IP}`)
+				.reply(200, mocks['00.00'].fastly);
+
+			nock('https://ipinfo.io')
+				.get(`/${MOCK_IP}`)
+				.reply(200, mocks['00.00'].ipinfo);
+
+			nock('https://geoip.maxmind.com/geoip/v2.1/city/')
+				.get(`/${MOCK_IP}`)
+				.reply(200, mocks['00.00'].maxmind);
+
+			const info = await clientWithBrokenCache.lookup(MOCK_IP);
+
+			expect(info).to.deep.equal({
+				continent: 'SA',
+				country: 'AR',
+				normalizedRegion: 'south america',
+				region: 'South America',
+				state: undefined,
+				city: 'Buenos Aires',
+				normalizedCity: 'buenos aires',
+				asn: 61_493,
+				latitude: -34.602,
+				longitude: -58.384,
+				network: 'interbs s.r.l.',
+				normalizedNetwork: 'interbs s.r.l.',
+			});
+		});
+	});
+
 	it('should use maxmind & digitalelement consensus', async () => {
 		nock('https://globalping-geoip.global.ssl.fastly.net')
 			.get(`/${MOCK_IP}`)