Tweak API support S3 bucket stream #2289

kkwangsir · 2024-09-17T01:24:48Z

add new fields IsSyncedToS3 last_synced_to_s3_on S3Path to FileReference
add force param to GetFiles method , and upload-files-to-s3 endpoint
Update Editor/Subscriber/Content and Storage Controller to support priority use of S3 files.
add delete-old-local-files api

To make this PR small, I make another PR to refactor transcription service.

api/net/Areas/Editor/Controllers/StorageController.cs

laidaoyu · 2024-09-17T19:50:59Z

libs/net/dal/Services/FileReferenceService.cs

+
+
+    public async Task<(Stream? Stream, string? FileName, string? ContentType)> GetFileStreamAsync(string path)
+    {


should check the path is null first? so skip the rest if the path is empty.

laidaoyu · 2024-09-17T19:51:48Z

libs/net/dal/Services/FileReferenceService.cs

+
+    public async Task<(Stream? Stream, string? FileName, string? ContentType)> GetFileStreamAsync(string path)
+    {
+        var fileReference = await GetByS3PathAsync(path);


can check if the path is in the S3 path format, to avoid the if... if...

laidaoyu · 2024-09-17T19:55:16Z

libs/net/dal/Services/FileReferenceService.cs

+        var filesToDelete = await this.Context.FileReferences
+            .Where(fr => fr.CreatedOn < beforeDate.ToUniversalTime()
+                         && fr.IsSyncedToS3
+                         && (fr.ContentType.StartsWith("audio/") || fr.ContentType.StartsWith("video/"))


could make a const variable in the class as an array ["audio", "video"], or make an enum
Easier to add or delete later.

api/net/Areas/Editor/Controllers/StorageController.cs

api/net/Areas/Editor/Controllers/ContentController.cs

api/net/Areas/Editor/Controllers/StorageController.cs

Fosol · 2024-09-17T20:46:30Z

libs/net/dal/Services/FileReferenceService.cs

+    }
+
+
+    public async Task<(Stream? Stream, string? FileName, string? ContentType)> GetFileStreamAsync(string path)


Why are we searching for FileReferences by their path? We should only be searching by their primary key.

In fact, path is actually the key s3 path. However, that we don't have path defined as unique in our references, which could be a hidden problem. I've modified it to ignore the reference step and access s3 directly with path(key).

api/net/Areas/Editor/Controllers/StorageController.cs

Fosol · 2024-09-17T20:48:08Z

api/net/Areas/Editor/Controllers/StorageController.cs

+            return NotFound($"Stream does not exist: '{path}'");
+        }
+
+        if (fileReference.IsSyncedToS3)


Is there a reason this logic appears to be identical to the code in the FileReferenceService? It appears like it could be centralized.

libs/net/dal/Services/FileReferenceService.cs

Fosol · 2024-09-18T16:45:55Z

libs/net/dal/Services/FileReferenceService.cs

+
+    public async Task<FileReference?> GetByS3PathAsync(string s3Path)
+    {
+        return await this.Context.FileReferences


You shouldn't be searching the database for a file reference by path. You should only be searching for file reference by its primary key.

Fosol · 2024-09-18T16:45:59Z

libs/net/dal/Services/FileReferenceService.cs

+            .FirstOrDefaultAsync(fr => fr.S3Path == s3Path);
+    }
+
+    public async Task<FileReference?> GetByPathAsync(string path)


You shouldn't be searching the database for a file reference by path. You should only be searching for file reference by its primary key.

I am going to remove those. not search them. path come from endpoint, I will let them goes to s3 file query or local file finder directly.

Fosol · 2024-09-20T23:37:15Z

libs/net/dal/Config/S3Options.cs

+/// <summary>
+/// S3 config
+/// </summary>
+public static class S3Options


Generally using static classes this way isn't ideal. It makes it difficult or impossible to use dependency injection and then later unit tests.

Is there a reason you cannot use the default configuration options like everything else? Then dependency injection works out of the box correctly.

If you need a helper class, then it should also not be a static class. If you only need a single instance of a shared helper class then configure dependency injection so that it only uses that one class.

Sure. let me follow other options styles.

Fosol · 2024-09-20T23:37:43Z

libs/net/dal/Extensions/ServiceCollectionExtensions.cs

@@ -9,6 +9,7 @@
 using TNO.DAL.Config;
 using TNO.DAL.Services;
 using TNO.Elastic;
+using Amazon.S3;


Is this required?

libs/net/dal/Services/FileReferenceService.cs

Fosol · 2024-09-20T23:44:45Z

api/net/Areas/Editor/Controllers/StorageController.cs

    /// <returns>uploaded files and failed uploads</returns>
    [HttpPost("upload-files-to-s3")]
    [Produces(MediaTypeNames.Application.Json)]
    [ProducesResponseType(typeof(Dictionary<string, List<string>>), (int)HttpStatusCode.OK)]
    [ProducesResponseType(typeof(ErrorResponseModel), (int)HttpStatusCode.BadRequest)]
    [SwaggerOperation(Tags = new[] { "Storage" })]
-    public async Task<IActionResult> UploadFilesToS3([FromQuery] DateTime? updatedBefore = null, [FromQuery] int? limit = null)
+    public async Task<IActionResult> UploadFileToS3Async([FromQuery] DateTime? updatedBefore = null, [FromQuery] int? limit = null, [FromQuery] bool force = false)


When a file was updated last has little meaning or value to MMI. We use those for audit fields, but generally nothing else. Uploading to S3 probably should be based on the Content.PublishedOn date, as that has a relationship to the license. Also there will be a lot of historical content that was recently updated, but it's old and should be moved to S3.

Wrong word, it's a filter base on PublishedOn , I will change to a proper one like PublishBefore.

Fosol · 2024-09-25T16:19:02Z

libs/net/dal/Config/S3Options.cs

+/// </summary>
+public class S3Options
+{
+    public string BucketName => Environment.GetEnvironmentVariable("S3_BUCKET_NAME") ?? "";


This isn't how configuration options should be set. This will result in always using the environment variable value S3_BUCKET_NAME, where it should be following the dependency injection configuration you set below S3.BucketName. Expression-bodied member properties are not useful for configuration options.

You can review the existing code to see how all this works. Also the following are links to information to understand it.

https://learn.microsoft.com/en-us/dotnet/core/extensions/dependency-injection
https://learn.microsoft.com/en-us/dotnet/core/extensions/configuration

libs/net/dal/Extensions/ServiceCollectionExtensions.cs

Fosol · 2024-09-25T16:35:24Z

api/net/Areas/Editor/Controllers/StorageController.cs

    {
        _connection = connection;
        _storageOptions = storageOptions.Value;
        _apiOptions = apiOptions.Value;
        _fileReferenceService = fileReferenceService;
        _logger = logger;
+        _s3Options = s3Options.Value;


Here you have chosen to use the scoped version of the configuration options. Which is the standard and in my opinion the correct way to do this.

Fosol · 2024-09-25T16:36:08Z

libs/net/dal/Services/FileReferenceService.cs

@@ -23,9 +24,11 @@ public FileReferenceService(
        ClaimsPrincipal principal,
        IServiceProvider serviceProvider,
        StorageOptions options,
+        S3Options s3Options,


Here you are using the singleton. Is there a reason for this?

This is for configuration only. Use the standard IOptions<S3Options> approach. If you need a singleton for connecting to S3, then you create a helper class and configure it through dependency injection as a singleton.

Fosol · 2024-09-25T16:37:10Z

libs/net/dal/Services/FileReferenceService.cs

-                updatedBefore = updatedBefore.Value.ToUniversalTime();
-                query = query.Where(fr => fr.UpdatedOn < updatedBefore.Value);
+                createdBefore = createdBefore.Value.ToUniversalTime();
+                query = query.Where(fr => fr.CreatedOn < createdBefore.Value);


Why are we searching for files created before a date?

Why are we searching for files created before a date?

We don't want to move the latest files to the S3 bucket, might be move files before 30 days. I also add a parameter to let us select a date range.

We should be using the Content.PublishedOn date, not the FileReference.CreatedOn date. The CreatedOn date is an audit field that tells us when a row in the database was created. We are currently importing historical content, which means all the CreatedOn dates will be at the same time. I suppose for testing purposes this will be okay, but the implementation could be problematic if the performance of S3 isn't ideal.

Fosol · 2024-09-25T16:39:23Z

libs/net/dal/Services/FileReferenceService.cs

+        return entity;
+    }
+
+    public async Task<int> DeleteOldLocalFilesAsync(DateTime beforeDate)


I still don't understand the purpose of this method related to the business processes. What is the requirement that requires this function?

I will remove it as well. Endpoint was removed.

… appSettings.json

Fosol · 2024-09-26T14:30:07Z

libs/net/dal/Extensions/ServiceCollectionExtensions.cs

+    public static IServiceCollection AddS3Config(this IServiceCollection services, IConfiguration config)
+    {
+        services.Configure<S3Options>(config.GetSection("S3"));
+        services.AddSingleton(sp => sp.GetRequiredService<IOptions<S3Options>>().Value);


You don't need a singleton of the configuration values. You may only need a singleton of the helper class.

Fosol · 2024-09-26T14:31:46Z

libs/net/dal/Services/FileReferenceService.cs

@@ -23,9 +24,11 @@ public FileReferenceService(
        ClaimsPrincipal principal,
        IServiceProvider serviceProvider,
        StorageOptions options,
+        S3Options s3Options,


This is for configuration only. Use the standard IOptions<S3Options> approach. If you need a singleton for connecting to S3, then you create a helper class and configure it through dependency injection as a singleton.

Fosol · 2024-09-26T14:45:22Z

libs/net/dal/Services/FileReferenceService.cs

-                updatedBefore = updatedBefore.Value.ToUniversalTime();
-                query = query.Where(fr => fr.UpdatedOn < updatedBefore.Value);
+                createdBefore = createdBefore.Value.ToUniversalTime();
+                query = query.Where(fr => fr.CreatedOn < createdBefore.Value);


We should be using the Content.PublishedOn date, not the FileReference.CreatedOn date. The CreatedOn date is an audit field that tells us when a row in the database was created. We are currently importing historical content, which means all the CreatedOn dates will be at the same time. I suppose for testing purposes this will be okay, but the implementation could be problematic if the performance of S3 isn't ideal.

… publishedAfter publishedBefore, filter by content's publishedOn

kkwangsir added 2 commits September 16, 2024 18:08

add new fields to FileReference

90d4666

update api support s3 bucket

b6fd0a0

JacobWang-bc added DB Migration A DB Migration may require refreshing or simply updating your database. devops DevOps templates, documentation, configuration, tools. labels Sep 17, 2024

JacobWang-bc requested review from laidaoyu, Fosol and nehalaggarwal-bcgov September 17, 2024 01:26

File Delete API

a20112c

laidaoyu reviewed Sep 17, 2024

View reviewed changes

api/net/Areas/Editor/Controllers/StorageController.cs Outdated Show resolved Hide resolved

laidaoyu reviewed Sep 17, 2024

View reviewed changes

Fosol reviewed Sep 17, 2024

View reviewed changes

kkwangsir added 2 commits September 18, 2024 06:08

rename method

70789fe

remove delete endpoint, move the process to upload endpoint

449ec20

Fosol reviewed Sep 18, 2024

View reviewed changes

kkwangsir added 4 commits September 19, 2024 09:37

use config to use s3 client and info.

d24e682

s3options utils

af505db

update fileReferenceService

954cb2a

update controllers

936fe3b

Fosol reviewed Sep 20, 2024

View reviewed changes

kkwangsir added 2 commits September 24, 2024 07:33

use createdBefore

fef73f5

change s3Option to use regular class and refactoring.

86f1469

Fosol reviewed Sep 25, 2024

View reviewed changes

kkwangsir added 3 commits September 25, 2024 09:54

remove the remove function

bc8744e

clean up s3options, move some methods to fileReferenceService, update…

115b61c

… appSettings.json

Added createdAfter param to support select a date range.

995744a

Fosol reviewed Sep 26, 2024

View reviewed changes

using instance s3 client; auto Dispose, drop Singleton s3options; use…

526836e

… publishedAfter publishedBefore, filter by content's publishedOn

Fosol approved these changes Sep 26, 2024

View reviewed changes

Fosol merged commit 4287173 into bcgov:dev Sep 26, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweak API support S3 bucket stream #2289

Tweak API support S3 bucket stream #2289

kkwangsir commented Sep 17, 2024 •

edited by JacobWang-bc

Loading

laidaoyu Sep 17, 2024

laidaoyu Sep 17, 2024

laidaoyu Sep 17, 2024

Fosol Sep 17, 2024

JacobWang-bc Sep 18, 2024

Fosol Sep 17, 2024

Fosol Sep 18, 2024

Fosol Sep 18, 2024

JacobWang-bc Sep 18, 2024

Fosol Sep 20, 2024

JacobWang-bc Sep 20, 2024

Fosol Sep 20, 2024

Fosol Sep 20, 2024

JacobWang-bc Sep 20, 2024

Fosol Sep 25, 2024

Fosol Sep 25, 2024

Fosol Sep 25, 2024

Fosol Sep 26, 2024

Fosol Sep 25, 2024

JacobWang-bc Sep 25, 2024

Fosol Sep 26, 2024

Fosol Sep 25, 2024

JacobWang-bc Sep 25, 2024

Fosol Sep 26, 2024

Fosol Sep 26, 2024

Fosol Sep 26, 2024



		public async Task<(Stream? Stream, string? FileName, string? ContentType)> GetFileStreamAsync(string path)
		{

Tweak API support S3 bucket stream #2289

Tweak API support S3 bucket stream #2289

Conversation

kkwangsir commented Sep 17, 2024 • edited by JacobWang-bc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkwangsir commented Sep 17, 2024 •

edited by JacobWang-bc

Loading