From 4512a2300f23eef2efea83114001d6398eb8c88c Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Tue, 24 Jun 2025 11:58:40 +0300 Subject: [PATCH 1/8] refactor: reduce s3 listing --- MergerLogic/Clients/S3Client.cs | 43 +++--- MergerLogicUnitTests/Utils/S3UtilsTest.cs | 157 +++++++++++++--------- 2 files changed, 116 insertions(+), 84 deletions(-) diff --git a/MergerLogic/Clients/S3Client.cs b/MergerLogic/Clients/S3Client.cs index 8dd39d96..b7a41812 100644 --- a/MergerLogic/Clients/S3Client.cs +++ b/MergerLogic/Clients/S3Client.cs @@ -53,8 +53,8 @@ public S3Client(IAmazonS3 client, IPathUtils pathUtils, IGeoUtils geoUtils, ILog catch (AggregateException e) { string message = $"exception while getting key {key}, Message: {e.Message}"; - this._logger.LogError($"[{methodName}] {message}"); - throw new Exception(message, e); + this._logger.LogDebug($"[{methodName}] {message}"); + return null; } } @@ -62,14 +62,13 @@ public S3Client(IAmazonS3 client, IPathUtils pathUtils, IGeoUtils geoUtils, ILog { string methodName = MethodBase.GetCurrentMethod().Name; this._logger.LogDebug($"[{methodName}] start z: {z}, x: {x}, y: {y}"); - var key = this.GetTileKey(z, x, y); - if (key == null) + string keyPrefix = this._pathUtils.GetTilePathWithoutExtension(this.path, z, x, y, true); + byte[]? imageBytes = this.GetImageBytes(keyPrefix); + if (imageBytes == null) { - this._logger.LogDebug($"[{methodName}] tileKey is null for z: {z}, x: {x}, y: {y}"); return null; } - byte[]? imageBytes = this.GetImageBytes(key); this._logger.LogDebug($"[{methodName}] end z: {z}, x: {x}, y: {y}"); return this.CreateTile(z, x, y, imageBytes); } @@ -78,13 +77,13 @@ public S3Client(IAmazonS3 client, IPathUtils pathUtils, IGeoUtils geoUtils, ILog { string methodName = MethodBase.GetCurrentMethod().Name; this._logger.LogDebug($"[{methodName}] start key: {key}"); - Coord coords = this._pathUtils.FromPath(key, true); byte[]? imageBytes = this.GetImageBytes(key); if (imageBytes == null) { return null; } this._logger.LogDebug($"[{methodName}] end key: {key}"); + Coord coords = this._pathUtils.FromPath(key, true); return this.CreateTile(coords, imageBytes); } @@ -92,9 +91,9 @@ public override bool TileExists(int z, int x, int y) { string methodName = MethodBase.GetCurrentMethod().Name; this._logger.LogDebug($"[{methodName}] start z: {z}, x: {x}, y: {y}"); - bool isExists = this.GetTileKey(z, x, y) != null; + bool exists = this.GetTileKey(z, x, y) != null; this._logger.LogDebug($"[{methodName}] end z: {z}, x: {x}, y: {y}"); - return isExists; + return exists; } public void UpdateTile(Tile tile) @@ -105,10 +104,10 @@ public void UpdateTile(Tile tile) var request = new PutObjectRequest() { - BucketName = this._bucket, - CannedACL = S3CannedACL.PublicRead, - Key = String.Format(key), - StorageClass=this._storageClass + BucketName = this._bucket, + CannedACL = S3CannedACL.PublicRead, + Key = String.Format(key), + StorageClass = this._storageClass }; byte[] buffer = tile.GetImageBytes(); @@ -124,11 +123,21 @@ public void UpdateTile(Tile tile) private string? GetTileKey(int z, int x, int y) { + string methodName = MethodBase.GetCurrentMethod().Name; string keyPrefix = this._pathUtils.GetTilePathWithoutExtension(this.path, z, x, y, true); - var listRequests = new ListObjectsV2Request { BucketName = this._bucket, Prefix = keyPrefix, MaxKeys = 1 }; - var listObjectsTask = this._client.ListObjectsV2Async(listRequests); - string? result = listObjectsTask.Result.S3Objects.FirstOrDefault()?.Key; - return result; + + try + { + var getRequest = new GetObjectRequest { BucketName = this._bucket, Key = keyPrefix }; + var getObjectTask = this._client.GetObjectAsync(getRequest); + string result = getObjectTask.Result.Key; + return result; + } + catch (Exception e) + { + this._logger.LogDebug($"[{methodName}] error getting key: {e.Message}"); + return null; + } } } } diff --git a/MergerLogicUnitTests/Utils/S3UtilsTest.cs b/MergerLogicUnitTests/Utils/S3UtilsTest.cs index 19739c8b..d41350f8 100644 --- a/MergerLogicUnitTests/Utils/S3UtilsTest.cs +++ b/MergerLogicUnitTests/Utils/S3UtilsTest.cs @@ -27,7 +27,8 @@ public class S3UtilsTest #region mocks private MockRepository _repository; - private Mock _s3ClientMock; + private Mock _s3ClientMock; + private Mock _amazonS3ClientMock; private Mock _pathUtilsMock; private Mock _geoUtilsMock; private Mock _imageFormatterMock; @@ -42,7 +43,8 @@ public class S3UtilsTest public void BeforeEach() { this._repository = new MockRepository(MockBehavior.Strict); - this._s3ClientMock = this._repository.Create(); + this._s3ClientMock = this._repository.Create(); + this._amazonS3ClientMock = this._repository.Create(); this._pathUtilsMock = this._repository.Create(); this._geoUtilsMock = this._repository.Create(); this._imageFormatterMock = this._repository.Create(); @@ -85,48 +87,45 @@ public void GetTile(bool exist, GetTileParamType paramType, TileFormat tileForma } else { - var keyPrefix = $"test/{cords.Z}/{cords.X}/{cords.Y}"; this._pathUtilsMock .InSequence(seq) - .Setup(utils => - utils.GetTilePathWithoutExtension("test", cords.Z, cords.X, cords.Y, true)) - .Returns(keyPrefix); - this._s3ClientMock - .InSequence(seq) - .Setup(s3 => s3.ListObjectsV2Async(It.Is(req => - req.BucketName == "bucket" && req.Prefix == keyPrefix && req.MaxKeys == 1), - It.IsAny())) - .ReturnsAsync(new ListObjectsV2Response() - { - S3Objects = exist ? new List() { new S3Object() { Key = "key" } } : new List() - }); + .Setup(utils => utils.GetTilePathWithoutExtension("test", 0, 0, 0, true)) + .Returns("key"); } - using (var dataStream = new MemoryStream(data)) + if (exist) { - if (exist) + this._amazonS3ClientMock.Setup(s3 => s3.GetObjectAsync(It.Is(req => + req.BucketName == "bucket" && req.Key == "key"), It.IsAny())) + .ReturnsAsync(new GetObjectResponse() { ResponseStream = dataStream }); + this._imageFormatterMock.Setup(formatter => formatter.GetTileFormat(It.IsAny())) + .Returns(tileFormat); + + if (paramType == GetTileParamType.String) { - this._s3ClientMock - .InSequence(seq) - .Setup(s3 => s3.GetObjectAsync(It.Is(req => - req.BucketName == "bucket" && req.Key == "key"), It.IsAny())) - .ReturnsAsync(new GetObjectResponse() { ResponseStream = dataStream }); - this._imageFormatterMock.Setup(formatter => formatter.GetTileFormat(It.IsAny())) - .Returns(tileFormat); + this._s3ClientMock.Setup(s3 => s3.GetTile(It.IsAny())).Returns(new Tile(cords, data)); } else { - this._s3ClientMock - .InSequence(seq) - .Setup(s3 => s3.GetObjectAsync(It.Is(req => - req.BucketName == "bucket" && req.Key == "key"), It.IsAny())) - .ThrowsAsync(new AggregateException("test")); + this._s3ClientMock.Setup(s3 => s3.GetTile(It.IsAny(), It.IsAny(), + It.IsAny())).Returns(new Tile(cords, data)); } + } + else + { + this._amazonS3ClientMock + .InSequence(seq) + .Setup(s3 => s3.GetObjectAsync(It.Is(req => + req.BucketName == "bucket" && req.Key == "key"), It.IsAny())) + .ThrowsAsync(new AggregateException("test")); + } - var s3Utils = new S3Client(this._s3ClientMock.Object, this._pathUtilsMock.Object, + using (var dataStream = new MemoryStream(data)) + { + var s3Utils = new S3Client(this._amazonS3ClientMock.Object, this._pathUtilsMock.Object, this._geoUtilsMock.Object, this._loggerMock.Object, "STANDARD", "bucket", "test"); - Tile tile = null; + Tile? tile = null; switch (paramType) { case GetTileParamType.Coord: @@ -136,15 +135,7 @@ public void GetTile(bool exist, GetTileParamType paramType, TileFormat tileForma tile = s3Utils.GetTile(cords.Z, cords.X, cords.Y); break; case GetTileParamType.String: - if (exist) - { - tile = s3Utils.GetTile("key"); - } - else - { - Assert.ThrowsException(() => s3Utils.GetTile("key")); - tile = null; - } + tile = exist ? s3Utils.GetTile("key") : null; break; } @@ -154,27 +145,18 @@ public void GetTile(bool exist, GetTileParamType paramType, TileFormat tileForma } else { + this._amazonS3ClientMock.Verify(s3 => s3.GetObjectAsync(It.IsAny(), It.IsAny()), Times.Once); Assert.AreEqual(cords.Z, tile.Z); Assert.AreEqual(cords.X, tile.X); Assert.AreEqual(cords.Y, tile.Y); CollectionAssert.AreEqual(data, tile.GetImageBytes()); } } - if (paramType == GetTileParamType.String) - { - this._pathUtilsMock.Verify(utils => utils.FromPath(It.IsAny(), It.IsAny()), Times.Once); - } - else - { - this._pathUtilsMock.Verify(utils => utils.GetTilePathWithoutExtension(It.IsAny(), It.IsAny(), It.IsAny(), - It.IsAny(), It.IsAny()), Times.Once); - this._s3ClientMock.Verify(s3 => s3.ListObjectsV2Async(It.IsAny(), It.IsAny()), Times.Once); - } - if (exist || paramType == GetTileParamType.String) + if (paramType != GetTileParamType.String) { - this._s3ClientMock.Verify(s3 => - s3.GetObjectAsync(It.IsAny(), It.IsAny()), Times.Once); + this._pathUtilsMock.Verify(utils => utils.GetTilePathWithoutExtension(It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny()), Times.Once()); } this.VerifyAll(); @@ -195,27 +177,68 @@ public void TileExists(bool exist) .InSequence(seq) .Setup(utils => utils.GetTilePathWithoutExtension("test", 0, 0, 0, true)) .Returns("key"); - this._s3ClientMock + + if (exist) + { + this._amazonS3ClientMock .InSequence(seq) - .Setup(s3 => s3.ListObjectsV2Async(It.Is(req => - req.BucketName == "bucket" && req.Prefix == "key" && req.MaxKeys == 1), + .Setup(s3 => s3.GetObjectAsync(It.Is(req => + req.BucketName == "bucket" && req.Key == "key"), It.IsAny())) - .ReturnsAsync(new ListObjectsV2Response() - { - S3Objects = exist ? new List() { new S3Object() { Key = "key" } } : new List() - }); + .ReturnsAsync(new GetObjectResponse() { Key = "key" }); + } + else + { + this._amazonS3ClientMock + .InSequence(seq) + .Setup(s3 => s3.GetObjectAsync(It.Is(req => + req.BucketName == "bucket" && req.Key == "key"), + It.IsAny())) + .Throws(new Exception()); + } - var s3Utils = new S3Client(this._s3ClientMock.Object, this._pathUtilsMock.Object, + var s3Utils = new S3Client(this._amazonS3ClientMock.Object, this._pathUtilsMock.Object, this._geoUtilsMock.Object, this._loggerMock.Object, "STANDARD", "bucket", "test"); Assert.AreEqual(exist, s3Utils.TileExists(0, 0, 0)); this._pathUtilsMock.Verify(utils => utils.GetTilePathWithoutExtension("test", 0, 0, 0, true), Times.Once); - this._s3ClientMock.Verify(s3 => s3.ListObjectsV2Async(It.Is(req => - req.BucketName == "bucket" && req.Prefix == "key"), It.IsAny()), Times.Once); + this._amazonS3ClientMock.Verify(s3 => s3.GetObjectAsync(It.Is(req => + req.BucketName == "bucket" && req.Key == "key"), It.IsAny()), Times.Once); this.VerifyAll(); } + // [TestMethod] + // [TestCategory("TileExists")] + // [DataRow(true)] + // [DataRow(false)] + // public void TileDoesNotExists(bool exist) + // { + // var seq = new MockSequence(); + // this._pathUtilsMock + // .InSequence(seq) + // .Setup(utils => utils.GetTilePathWithoutExtension("test", 0, 0, 0, true)) + // .Returns("key"); + + // this._s3ClientMock + // .InSequence(seq) + // .Setup(s3 => s3.GetObjectAsync(It.Is(req => + // req.BucketName == "bucket" && req.Key == "key"), + // It.IsAny())) + // .Throws(new Exception()); + + // var s3Utils = new S3Client(this._s3ClientMock.Object, this._pathUtilsMock.Object, + // this._geoUtilsMock.Object, this._loggerMock.Object, "STANDARD", "bucket", "test"); + + // Assert.AreEqual(exist, s3Utils.TileExists(0, 0, 0)); + + // this._pathUtilsMock.Verify(utils => utils.GetTilePathWithoutExtension("test", 0, 0, 0, true), Times.Once); + // this._s3ClientMock.Verify(s3 => s3.GetObjectAsync(It.Is(req => + // req.BucketName == "bucket" && req.Key == "key"), It.IsAny()), Times.Once); + // Assert.ThrowsException(() => s3Utils.TileExists(0, 0, 0)); + // this.VerifyAll(); + // } + #endregion #region UpdateTile @@ -232,7 +255,7 @@ public void UpdateTile() .InSequence(seq) .Setup(utils => utils.GetTilePath("test", testTile, true)) .Returns("key"); - this._s3ClientMock + this._amazonS3ClientMock .InSequence(seq) .Setup(s3 => s3.PutObjectAsync(It.Is(req => req.BucketName == "bucket" && req.Key == "key"), It.IsAny())) @@ -242,12 +265,12 @@ public void UpdateTile() return Task.FromResult(new PutObjectResponse()); }); - var s3Utils = new S3Client(this._s3ClientMock.Object, this._pathUtilsMock.Object, + var s3Utils = new S3Client(this._amazonS3ClientMock.Object, this._pathUtilsMock.Object, this._geoUtilsMock.Object, this._loggerMock.Object, "STANDARD", "bucket", "test"); s3Utils.UpdateTile(testTile); this._pathUtilsMock.Verify(utils => utils.GetTilePath(It.IsAny(), It.IsAny(), It.IsAny()), Times.Once); - this._s3ClientMock.Verify(s3 => s3.PutObjectAsync(It.Is(req => + this._amazonS3ClientMock.Verify(s3 => s3.PutObjectAsync(It.Is(req => req.BucketName == "bucket" && req.Key == "key"), It.IsAny()), Times.Once); Assert.AreEqual(buff.Length, readLen); @@ -261,7 +284,7 @@ public void UpdateTile() private void VerifyAll() { - this._s3ClientMock.VerifyAll(); + this._amazonS3ClientMock.VerifyAll(); this._pathUtilsMock.VerifyAll(); this._geoUtilsMock.VerifyAll(); } From 4fc98c49d2f5e831cc96d5085fae3e39135109b4 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Tue, 24 Jun 2025 12:07:47 +0300 Subject: [PATCH 2/8] test: fix broken test --- MergerLogicUnitTests/Utils/S3UtilsTest.cs | 44 +++++++++++------------ 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/MergerLogicUnitTests/Utils/S3UtilsTest.cs b/MergerLogicUnitTests/Utils/S3UtilsTest.cs index d41350f8..db065c6d 100644 --- a/MergerLogicUnitTests/Utils/S3UtilsTest.cs +++ b/MergerLogicUnitTests/Utils/S3UtilsTest.cs @@ -93,35 +93,35 @@ public void GetTile(bool exist, GetTileParamType paramType, TileFormat tileForma .Returns("key"); } - if (exist) + using (var dataStream = new MemoryStream(data)) { - this._amazonS3ClientMock.Setup(s3 => s3.GetObjectAsync(It.Is(req => - req.BucketName == "bucket" && req.Key == "key"), It.IsAny())) - .ReturnsAsync(new GetObjectResponse() { ResponseStream = dataStream }); - this._imageFormatterMock.Setup(formatter => formatter.GetTileFormat(It.IsAny())) - .Returns(tileFormat); - - if (paramType == GetTileParamType.String) + if (exist) { - this._s3ClientMock.Setup(s3 => s3.GetTile(It.IsAny())).Returns(new Tile(cords, data)); + this._amazonS3ClientMock.Setup(s3 => s3.GetObjectAsync(It.Is(req => + req.BucketName == "bucket" && req.Key == "key"), It.IsAny())) + .ReturnsAsync(new GetObjectResponse() { ResponseStream = dataStream }); + this._imageFormatterMock.Setup(formatter => formatter.GetTileFormat(It.IsAny())) + .Returns(tileFormat); + + if (paramType == GetTileParamType.String) + { + this._s3ClientMock.Setup(s3 => s3.GetTile(It.IsAny())).Returns(new Tile(cords, data)); + } + else + { + this._s3ClientMock.Setup(s3 => s3.GetTile(It.IsAny(), It.IsAny(), + It.IsAny())).Returns(new Tile(cords, data)); + } } else { - this._s3ClientMock.Setup(s3 => s3.GetTile(It.IsAny(), It.IsAny(), - It.IsAny())).Returns(new Tile(cords, data)); + this._amazonS3ClientMock + .InSequence(seq) + .Setup(s3 => s3.GetObjectAsync(It.Is(req => + req.BucketName == "bucket" && req.Key == "key"), It.IsAny())) + .ThrowsAsync(new AggregateException("test")); } - } - else - { - this._amazonS3ClientMock - .InSequence(seq) - .Setup(s3 => s3.GetObjectAsync(It.Is(req => - req.BucketName == "bucket" && req.Key == "key"), It.IsAny())) - .ThrowsAsync(new AggregateException("test")); - } - using (var dataStream = new MemoryStream(data)) - { var s3Utils = new S3Client(this._amazonS3ClientMock.Object, this._pathUtilsMock.Object, this._geoUtilsMock.Object, this._loggerMock.Object, "STANDARD", "bucket", "test"); From 5cce1d6ebe96c4c350a50a143da851ebe95a42c5 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Sun, 29 Jun 2025 14:56:34 +0300 Subject: [PATCH 3/8] test: remove commented test --- MergerLogicUnitTests/Utils/S3UtilsTest.cs | 31 ----------------------- 1 file changed, 31 deletions(-) diff --git a/MergerLogicUnitTests/Utils/S3UtilsTest.cs b/MergerLogicUnitTests/Utils/S3UtilsTest.cs index db065c6d..87cd7486 100644 --- a/MergerLogicUnitTests/Utils/S3UtilsTest.cs +++ b/MergerLogicUnitTests/Utils/S3UtilsTest.cs @@ -208,37 +208,6 @@ public void TileExists(bool exist) this.VerifyAll(); } - // [TestMethod] - // [TestCategory("TileExists")] - // [DataRow(true)] - // [DataRow(false)] - // public void TileDoesNotExists(bool exist) - // { - // var seq = new MockSequence(); - // this._pathUtilsMock - // .InSequence(seq) - // .Setup(utils => utils.GetTilePathWithoutExtension("test", 0, 0, 0, true)) - // .Returns("key"); - - // this._s3ClientMock - // .InSequence(seq) - // .Setup(s3 => s3.GetObjectAsync(It.Is(req => - // req.BucketName == "bucket" && req.Key == "key"), - // It.IsAny())) - // .Throws(new Exception()); - - // var s3Utils = new S3Client(this._s3ClientMock.Object, this._pathUtilsMock.Object, - // this._geoUtilsMock.Object, this._loggerMock.Object, "STANDARD", "bucket", "test"); - - // Assert.AreEqual(exist, s3Utils.TileExists(0, 0, 0)); - - // this._pathUtilsMock.Verify(utils => utils.GetTilePathWithoutExtension("test", 0, 0, 0, true), Times.Once); - // this._s3ClientMock.Verify(s3 => s3.GetObjectAsync(It.Is(req => - // req.BucketName == "bucket" && req.Key == "key"), It.IsAny()), Times.Once); - // Assert.ThrowsException(() => s3Utils.TileExists(0, 0, 0)); - // this.VerifyAll(); - // } - #endregion #region UpdateTile From 2f687a81415ac447ef8337682abc50b459de22fc Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Sun, 29 Jun 2025 15:27:33 +0300 Subject: [PATCH 4/8] chore: change exception type in GetTileKey --- MergerLogic/Clients/S3Client.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MergerLogic/Clients/S3Client.cs b/MergerLogic/Clients/S3Client.cs index b7a41812..b573bef3 100644 --- a/MergerLogic/Clients/S3Client.cs +++ b/MergerLogic/Clients/S3Client.cs @@ -133,7 +133,7 @@ public void UpdateTile(Tile tile) string result = getObjectTask.Result.Key; return result; } - catch (Exception e) + catch (AggregateException e) { this._logger.LogDebug($"[{methodName}] error getting key: {e.Message}"); return null; From c7769970e63ef2af546bd1ca565c103308e6d1cb Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Sun, 29 Jun 2025 15:31:47 +0300 Subject: [PATCH 5/8] test: change exception thrown --- MergerLogicUnitTests/Utils/S3UtilsTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MergerLogicUnitTests/Utils/S3UtilsTest.cs b/MergerLogicUnitTests/Utils/S3UtilsTest.cs index 87cd7486..03849bca 100644 --- a/MergerLogicUnitTests/Utils/S3UtilsTest.cs +++ b/MergerLogicUnitTests/Utils/S3UtilsTest.cs @@ -194,7 +194,7 @@ public void TileExists(bool exist) .Setup(s3 => s3.GetObjectAsync(It.Is(req => req.BucketName == "bucket" && req.Key == "key"), It.IsAny())) - .Throws(new Exception()); + .Throws(new AggregateException()); } var s3Utils = new S3Client(this._amazonS3ClientMock.Object, this._pathUtilsMock.Object, From aeee1b44dac1f9c5e2241e97ef34b03f4d0bafbd Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Mon, 30 Jun 2025 16:53:40 +0300 Subject: [PATCH 6/8] refactor: handle S3 errors better when getting object --- MergerLogic/Clients/S3Client.cs | 35 +++++++++++++++++++---- MergerLogicUnitTests/Utils/S3UtilsTest.cs | 4 +-- 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/MergerLogic/Clients/S3Client.cs b/MergerLogic/Clients/S3Client.cs index b573bef3..68cff809 100644 --- a/MergerLogic/Clients/S3Client.cs +++ b/MergerLogic/Clients/S3Client.cs @@ -27,6 +27,21 @@ public S3Client(IAmazonS3 client, IPathUtils pathUtils, IGeoUtils geoUtils, ILog this._storageClass = new S3StorageClass(storageClass ?? S3StorageClass.Standard); } + private bool IsKeyError(Exception e) + { + if (e is AmazonS3Exception ex) + { + return ex.ErrorCode == "NoSuchKey"; + } + + if (e.InnerException is AmazonS3Exception en) + { + return en.ErrorCode == "NoSuchKey"; + } + + return false; + } + private byte[]? GetImageBytes(string key) { string? methodName = MethodBase.GetCurrentMethod()?.Name; @@ -52,9 +67,14 @@ public S3Client(IAmazonS3 client, IPathUtils pathUtils, IGeoUtils geoUtils, ILog } catch (AggregateException e) { - string message = $"exception while getting key {key}, Message: {e.Message}"; - this._logger.LogDebug($"[{methodName}] {message}"); - return null; + if (IsKeyError(e)) + { + string message = $"exception while getting key {key}, Message: {e.Message}"; + this._logger.LogDebug($"[{methodName}] {message}"); + return null; + } + // In case there are other errors such as connection to S3 + throw e; } } @@ -135,8 +155,13 @@ public void UpdateTile(Tile tile) } catch (AggregateException e) { - this._logger.LogDebug($"[{methodName}] error getting key: {e.Message}"); - return null; + if (IsKeyError(e)) + { + this._logger.LogDebug($"[{methodName}] error getting key: {e.Message}"); + return null; + } + // In case there are other errors such as connection to S3 + throw e; } } } diff --git a/MergerLogicUnitTests/Utils/S3UtilsTest.cs b/MergerLogicUnitTests/Utils/S3UtilsTest.cs index 03849bca..a9d03eaa 100644 --- a/MergerLogicUnitTests/Utils/S3UtilsTest.cs +++ b/MergerLogicUnitTests/Utils/S3UtilsTest.cs @@ -119,7 +119,7 @@ public void GetTile(bool exist, GetTileParamType paramType, TileFormat tileForma .InSequence(seq) .Setup(s3 => s3.GetObjectAsync(It.Is(req => req.BucketName == "bucket" && req.Key == "key"), It.IsAny())) - .ThrowsAsync(new AggregateException("test")); + .ThrowsAsync(new AmazonS3Exception("", Amazon.Runtime.ErrorType.Unknown, "NoSuchKey", "", System.Net.HttpStatusCode.NoContent)); } var s3Utils = new S3Client(this._amazonS3ClientMock.Object, this._pathUtilsMock.Object, @@ -194,7 +194,7 @@ public void TileExists(bool exist) .Setup(s3 => s3.GetObjectAsync(It.Is(req => req.BucketName == "bucket" && req.Key == "key"), It.IsAny())) - .Throws(new AggregateException()); + .ThrowsAsync(new AmazonS3Exception("", Amazon.Runtime.ErrorType.Unknown, "NoSuchKey", "", System.Net.HttpStatusCode.NoContent)); } var s3Utils = new S3Client(this._amazonS3ClientMock.Object, this._pathUtilsMock.Object, From 1fada346b010141f4a9f2c961a7b96d1c7544f58 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Mon, 7 Jul 2025 18:23:26 +0300 Subject: [PATCH 7/8] fix(s3): get tile by exact key --- MergerLogic/Clients/S3Client.cs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/MergerLogic/Clients/S3Client.cs b/MergerLogic/Clients/S3Client.cs index 68cff809..f06c0e23 100644 --- a/MergerLogic/Clients/S3Client.cs +++ b/MergerLogic/Clients/S3Client.cs @@ -2,6 +2,7 @@ using Amazon.S3.Model; using MergerLogic.Batching; using MergerLogic.DataTypes; +using MergerLogic.ImageProcessing; using MergerLogic.Utils; using Microsoft.Extensions.Logging; using System.Reflection; @@ -82,11 +83,17 @@ private bool IsKeyError(Exception e) { string methodName = MethodBase.GetCurrentMethod().Name; this._logger.LogDebug($"[{methodName}] start z: {z}, x: {x}, y: {y}"); - string keyPrefix = this._pathUtils.GetTilePathWithoutExtension(this.path, z, x, y, true); + string keyPrefix = this._pathUtils.GetTilePath(this.path, z, x, y, TileFormat.Jpeg, true); + byte[]? imageBytes = this.GetImageBytes(keyPrefix); if (imageBytes == null) { - return null; + keyPrefix = this._pathUtils.GetTilePath(this.path, z, x, y, TileFormat.Png, true); + imageBytes = this.GetImageBytes(keyPrefix); + if (imageBytes == null) + { + return null; + } } this._logger.LogDebug($"[{methodName}] end z: {z}, x: {x}, y: {y}"); @@ -102,6 +109,7 @@ private bool IsKeyError(Exception e) { return null; } + this._logger.LogDebug($"[{methodName}] end key: {key}"); Coord coords = this._pathUtils.FromPath(key, true); return this.CreateTile(coords, imageBytes); From b89b9fba39e3ec8ef45764d316faf992ee9f9ffd Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Wed, 9 Jul 2025 18:42:35 +0300 Subject: [PATCH 8/8] test(S3Utils): update tests to new logic for GetTile --- MergerLogicUnitTests/Utils/S3UtilsTest.cs | 190 +++++++++++++++++++--- 1 file changed, 171 insertions(+), 19 deletions(-) diff --git a/MergerLogicUnitTests/Utils/S3UtilsTest.cs b/MergerLogicUnitTests/Utils/S3UtilsTest.cs index a9d03eaa..b3e65ae1 100644 --- a/MergerLogicUnitTests/Utils/S3UtilsTest.cs +++ b/MergerLogicUnitTests/Utils/S3UtilsTest.cs @@ -69,6 +69,103 @@ public static IEnumerable GenGetTileParams() }); } + // TODO: revert to this test + // [TestMethod] + // [TestCategory("GetTile")] + // [DynamicData(nameof(GenGetTileParams), DynamicDataSourceType.Method)] + // public void GetTile(bool exist, GetTileParamType paramType, TileFormat tileFormat) + // { + // var seq = new MockSequence(); + // var data = this._jpegImageData; + // var cords = new Coord(0, 0, 0); + + // if (paramType != GetTileParamType.String) + // { + // this._pathUtilsMock + // .InSequence(seq) + // .Setup(utils => utils.GetTilePath("test", 0, 0, 0, TileFormat.Jpeg, true)) + // .Returns("key"); + // } + + // using (var dataStream = new MemoryStream(data)) + // { + // if (exist) + // { + // this._amazonS3ClientMock + // .Setup(s3 => s3.GetObjectAsync(It.Is(req => + // req.BucketName == "bucket" && req.Key == "key"), It.IsAny())) + // .ReturnsAsync(new GetObjectResponse() { ResponseStream = dataStream }); + // this._imageFormatterMock.Setup(formatter => formatter.GetTileFormat(It.IsAny())) + // .Returns(tileFormat); + + // if (paramType == GetTileParamType.String) + // { + // this._s3ClientMock.Setup(s3 => s3.GetTile(It.IsAny())).Returns(new Tile(cords, data)); + // } + // else + // { + // this._s3ClientMock.Setup(s3 => s3.GetTile(It.IsAny(), It.IsAny(), + // It.IsAny())).Returns(new Tile(cords, data)); + // } + // } + // else + // { + // this._amazonS3ClientMock + // .InSequence(seq) + // .Setup(s3 => s3.GetObjectAsync(It.Is(req => + // req.BucketName == "bucket" && req.Key == "key"), It.IsAny())) + // .ThrowsAsync(new AmazonS3Exception("", Amazon.Runtime.ErrorType.Unknown, "NoSuchKey", "", System.Net.HttpStatusCode.NoContent)); + // } + + // if (paramType == GetTileParamType.String) + // { + // this._pathUtilsMock + // .InSequence(seq) + // .Setup(utils => utils.FromPath("key", true)) + // .Returns(cords); + // } + + // var s3Utils = new S3Client(this._amazonS3ClientMock.Object, this._pathUtilsMock.Object, + // this._geoUtilsMock.Object, this._loggerMock.Object, "STANDARD", "bucket", "test"); + + // Tile? tile = null; + // switch (paramType) + // { + // case GetTileParamType.Coord: + // tile = s3Utils.GetTile(cords); + // break; + // case GetTileParamType.Ints: + // tile = s3Utils.GetTile(cords.Z, cords.X, cords.Y); + // break; + // case GetTileParamType.String: + // tile = exist ? s3Utils.GetTile("key") : null; + // break; + // } + + // if (!exist) + // { + // Assert.IsNull(tile); + // } + // else + // { + // this._amazonS3ClientMock.Verify(s3 => s3.GetObjectAsync(It.IsAny(), It.IsAny()), Times.Once); + // Assert.AreEqual(cords.Z, tile.Z); + // Assert.AreEqual(cords.X, tile.X); + // Assert.AreEqual(cords.Y, tile.Y); + // CollectionAssert.AreEqual(data, tile.GetImageBytes()); + // } + // } + + // if (paramType != GetTileParamType.String) + // { + // this._pathUtilsMock.Verify(utils => utils.GetTilePath(It.IsAny(), It.IsAny(), It.IsAny(), + // It.IsAny(), It.IsAny(), It.IsAny()), Times.Once()); + // } + + // this.VerifyAll(); + // } + + // TODO: remove this test after fileFormat fix (pass as part of each source) [TestMethod] [TestCategory("GetTile")] [DynamicData(nameof(GenGetTileParams), DynamicDataSourceType.Method)] @@ -78,26 +175,20 @@ public void GetTile(bool exist, GetTileParamType paramType, TileFormat tileForma var data = this._jpegImageData; var cords = new Coord(0, 0, 0); - if (paramType == GetTileParamType.String) - { - this._pathUtilsMock - .InSequence(seq) - .Setup(utils => utils.FromPath("key", true)) - .Returns(cords); - } - else - { - this._pathUtilsMock - .InSequence(seq) - .Setup(utils => utils.GetTilePathWithoutExtension("test", 0, 0, 0, true)) - .Returns("key"); - } - using (var dataStream = new MemoryStream(data)) { + if (paramType != GetTileParamType.String) + { + this._pathUtilsMock + .InSequence(seq) + .Setup(utils => utils.GetTilePath("test", 0, 0, 0, TileFormat.Jpeg, true)) + .Returns("key"); + } + if (exist) { - this._amazonS3ClientMock.Setup(s3 => s3.GetObjectAsync(It.Is(req => + this._amazonS3ClientMock + .Setup(s3 => s3.GetObjectAsync(It.Is(req => req.BucketName == "bucket" && req.Key == "key"), It.IsAny())) .ReturnsAsync(new GetObjectResponse() { ResponseStream = dataStream }); this._imageFormatterMock.Setup(formatter => formatter.GetTileFormat(It.IsAny())) @@ -122,6 +213,47 @@ public void GetTile(bool exist, GetTileParamType paramType, TileFormat tileForma .ThrowsAsync(new AmazonS3Exception("", Amazon.Runtime.ErrorType.Unknown, "NoSuchKey", "", System.Net.HttpStatusCode.NoContent)); } + if (paramType == GetTileParamType.String) + { + this._pathUtilsMock + .InSequence(seq) + .Setup(utils => utils.FromPath("key", true)) + .Returns(cords); + } + + // Repeat + if (paramType != GetTileParamType.String) + { + this._pathUtilsMock + .InSequence(seq) + .Setup(utils => utils.GetTilePath("test", 0, 0, 0, TileFormat.Png, true)) + .Returns("key"); + } + + if (exist) + { + this._amazonS3ClientMock + .Setup(s3 => s3.GetObjectAsync(It.Is(req => + req.BucketName == "bucket" && req.Key == "key"), It.IsAny())) + .ReturnsAsync(new GetObjectResponse() { ResponseStream = dataStream }); + this._imageFormatterMock.Setup(formatter => formatter.GetTileFormat(It.IsAny())) + .Returns(tileFormat); + + if (paramType != GetTileParamType.String) + { + this._s3ClientMock.Setup(s3 => s3.GetTile(It.IsAny(), It.IsAny(), + It.IsAny())).Returns(new Tile(cords, data)); + } + } + else + { + this._amazonS3ClientMock + .InSequence(seq) + .Setup(s3 => s3.GetObjectAsync(It.Is(req => + req.BucketName == "bucket" && req.Key == "key"), It.IsAny())) + .ThrowsAsync(new AmazonS3Exception("", Amazon.Runtime.ErrorType.Unknown, "NoSuchKey", "", System.Net.HttpStatusCode.NoContent)); + } + var s3Utils = new S3Client(this._amazonS3ClientMock.Object, this._pathUtilsMock.Object, this._geoUtilsMock.Object, this._loggerMock.Object, "STANDARD", "bucket", "test"); @@ -145,7 +277,19 @@ public void GetTile(bool exist, GetTileParamType paramType, TileFormat tileForma } else { - this._amazonS3ClientMock.Verify(s3 => s3.GetObjectAsync(It.IsAny(), It.IsAny()), Times.Once); + if (paramType == GetTileParamType.String) + { + this._amazonS3ClientMock.Verify(s3 => s3.GetObjectAsync(It.IsAny(), It.IsAny()), Times.Once); + } + else if (!exist) + { + this._amazonS3ClientMock.Verify(s3 => s3.GetObjectAsync(It.IsAny(), It.IsAny()), Times.Exactly(2)); + } + else + { + this._amazonS3ClientMock.Verify(s3 => s3.GetObjectAsync(It.IsAny(), It.IsAny()), Times.Once()); + } + Assert.AreEqual(cords.Z, tile.Z); Assert.AreEqual(cords.X, tile.X); Assert.AreEqual(cords.Y, tile.Y); @@ -155,8 +299,16 @@ public void GetTile(bool exist, GetTileParamType paramType, TileFormat tileForma if (paramType != GetTileParamType.String) { - this._pathUtilsMock.Verify(utils => utils.GetTilePathWithoutExtension(It.IsAny(), It.IsAny(), It.IsAny(), - It.IsAny(), It.IsAny()), Times.Once()); + if (!exist) + { + this._pathUtilsMock.Verify(utils => utils.GetTilePath(It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny()), Times.Exactly(2)); + } + else + { + this._pathUtilsMock.Verify(utils => utils.GetTilePath(It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny()), Times.Once()); + } } this.VerifyAll();