feat(ui-web): refactor the web backend, create tests #88

followynne · 2023-09-01T07:42:23Z

this PR aims to refactor the backend part of Web.Ui, to improve maintanability and to be able to test it in depth with a simpler approach.

(I did this part while working on the react-ui; since I'm going longer than expected with the react part (missing the css parts and a couple feat related to the auth token), I'm making this PR with the pieces that are independent-completed)

It includes

refactoring of Ui.Web
new options parameter to block access to index.html from the start (default to false, to preserve compatibility)
coverage of UI.Web project backend
some sonarqube smell fixed
bumped nuget packages (safe ones, I left out any "risky" package for another time)
replaced Moq with Nsubstitute due to the issues raised last month with Moq

mo-esmp · 2023-09-01T20:05:04Z

Hey @followynne,
Thanks for the PR. I will review it soon and since the PR is big, it takes some time.

followynne · 2023-09-01T20:11:30Z

@mo-esmp sure, no worries - take all the time you need! 👍

Since I didn't proceed in the last weeks on the React part, I wanted to at least submit the completed piece as a separate PR, to get something rollin' 😄 thus 0 hurry :)

mo-esmp · 2023-09-13T21:11:32Z

src/Serilog.Ui.ElasticSearchProvider/Serilog.Ui.ElasticSearchProvider.csproj

@@ -9,7 +9,7 @@
  <ItemGroup>
 	  <PackageReference Include="NEST" Version="7.11.1" />
    <PackageReference Include="NEST.JsonNetSerializer" Version="7.11.1" />
-    <PackageReference Include="Newtonsoft.Json" Version="13.0.2" />


Let's move Newtonsoft.Json to Serilog.Ui.Core project since it is referenced in most projects.

mo-esmp · 2023-09-13T21:15:51Z

src/Serilog.Ui.Web/Authorization/AuthorizationFilterService.cs

+
+namespace Serilog.Ui.Web.Authorization
+{
+    internal interface IAuthorizationFilterService


Move the interface to a separate file.

mo-esmp · 2023-09-13T21:19:12Z

src/Serilog.Ui.Web/Authorization/AuthorizationFilterService.cs

+            Func<HttpResponse, Task> onFailure = null)
+        {
+            var accessCheck = await CanAccess(httpContext, options);
+            if (!accessCheck)


Revert if condition.

mo-esmp · 2023-09-13T21:22:48Z

src/Serilog.Ui.Web/Endpoints/AppStreamLoader.cs

+
+namespace Serilog.Ui.Web.Endpoints
+{
+    internal interface IAppStreamLoader


Move to a separate file.

mo-esmp · 2023-09-13T21:23:19Z

src/Serilog.Ui.Web/Endpoints/AppStreamLoader.cs

+    internal class AppStreamLoader : IAppStreamLoader
+    {
+        private const string AppManifest = "Serilog.Ui.Web.wwwroot.dist.index.html";
+        public Stream GetIndex() =>


Add a break line.

mo-esmp · 2023-09-13T21:44:05Z

src/Serilog.Ui.Web/Endpoints/SerilogUiEndpoints.cs

+    internal class SerilogUiEndpoints : ISerilogUiEndpoints
+    {
+        private readonly ILogger<SerilogUiEndpoints> _logger;
+        private readonly JsonSerializerSettings _jsonSerializerOptions;


Make it static.

mo-esmp · 2023-09-13T21:47:01Z

src/Serilog.Ui.Web/Endpoints/SerilogUiEndpoints.cs

+
+            var provider = httpContext.RequestServices.GetService<AggregateDataProvider>();
+
+            string key = dbKey;


Remove the temp variable.

mo-esmp · 2023-09-13T21:51:07Z

src/Serilog.Ui.Web/Extensions/HttpRequestExtensions.cs

+        public static bool IsLocal(this HttpRequest request)
+        {
+            var ipAddress = request.Headers["X-forwarded-for"].FirstOrDefault();
+            if (!string.IsNullOrWhiteSpace(ipAddress))


Add {} ->

if(..) { ... }

mo-esmp · 2023-09-13T21:57:03Z

src/Serilog.Ui.Core/Serilog.Ui.Core.csproj

@@ -7,7 +7,7 @@
  </PropertyGroup>

  <ItemGroup>
-    <PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="3.1.3" />
+    <PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="7.0.0" />
  </ItemGroup>

  <ItemGroup>


Please remove the below from csproj.

<ItemGroup> <Folder Include="Models\" /> </ItemGroup>

mo-esmp · 2023-09-13T21:57:53Z

src/Serilog.Ui.Web/SerilogUiMiddleware.cs

+            var httpMethod = httpContext.Request.Method;
+            var isGet = httpMethod == "GET";
+
+            if (isGet)


Invert if.

if (!isGet) { return _staticFileMiddleware.Invoke(httpContext); }

mo-esmp · 2023-09-14T13:17:21Z

tests/Serilog.Ui.Web.Tests/Authorization/AuthorizationFilterTest.cs

+
+
+[Trait("Ui-Authorization", "Web")]
+public class AuthorizationDefaultTest : IClassFixture<WebAppFactory.Default>


Keep all authorization tests in one class or move test classes to separate files.

moved filters utils to the Utilities folder, separated test classes in files

mo-esmp · 2023-09-14T13:19:01Z

tests/Serilog.Ui.Web.Tests/Endpoints/SerilogUiAppRoutesTest.cs

+        [Fact]
+        public async Task It_gets_app_home()
+        {
+            sut.SetOptions(new()


It would be great if you add // Arrange, // Act and // Assert comments to new tests.

should be added now on entire ui.web :)

mo-esmp · 2023-09-14T13:20:25Z

tests/Serilog.Ui.Web.Tests/Endpoints/SerilogUiAppRoutesTest.cs

+    [Trait("Ui-Api-Routes", "Web")]
+    public class SerilogUiAppRoutesTest
+    {
+        private readonly IAppStreamLoader streamLoaderMock;


Please use _ for all private fields in test classes.

mo-esmp · 2023-09-14T13:22:44Z

tests/Serilog.Ui.Web.Tests/Extensions/HttpRequestExtensionsTest.cs

+        [Fact]
+        public void It_is_local_when_remote_ip_address_equals_local_ip_address()
+        {
+            var mock = Substitute.For<HttpRequest>();


Optional: mockHttpRequest

mo-esmp · 2023-09-14T13:23:00Z

tests/Serilog.Ui.Web.Tests/Extensions/HttpRequestExtensionsTest.cs

+        public void It_is_local_when_remote_ip_address_equals_local_ip_address()
+        {
+            var mock = Substitute.For<HttpRequest>();
+            var context = Substitute.For<HttpContext>();


Optional: mockHttpContext (and for the rest of test methods)

followynne · 2023-09-19T21:44:13Z

Hello @mo-esmp

thanks for the deep review, I updated the code on all comments except one (where you'll find a question).

I'll leave to you each comment resolution, to be sure I didn't miss anything while doing them 😄

mo-esmp · 2023-09-20T07:45:26Z

@followynne, as always, thanks for the contribution and improving stuff.

followynne added 12 commits September 1, 2023 09:23

fix(build): remove workflow extra condition

9c01a7f

feat(web): refactor the ui backend

f4fa51a

test(ui-web): cover the new refactor

d06bc74

tests(routes-auth): add case based on boolean default auth enabler

c951b18

chore(nuget): bump safe packages

00b3838

chore: bump other packages

f97fab0

fix: packages bumps, tests failing for whitespaces

83acda3

perf(moq): replace with nsubstitute after past months

cda3f77

fix: small warnings

a674a31

fix: try fixing whitespaces tests

2115cdb

chore: remove some sonar smells

f99437d

chore: update nuke

5d6e750

followynne marked this pull request as ready for review September 1, 2023 16:44

followynne added 4 commits September 1, 2023 18:47

Merge branch 'master' into feat/refactor-ui-web-backend

0fc9064

fix(sonar): remove security issue with regex timespan

d961636

fix: mongodb indexing, newline test

0e2c2e1

chore: rework mongodb test

59d8c87

followynne changed the title ~~feat(ui.web): refactor the web backend, create tests~~ feat(ui-web): refactor the web backend, create tests Sep 1, 2023

followynne enabled auto-merge (squash) September 1, 2023 17:22

followynne requested a review from mo-esmp September 1, 2023 17:27

Merge branch 'master' into feat/refactor-ui-web-backend

d8b74cd

mo-esmp reviewed Sep 13, 2023

View reviewed changes

mo-esmp reviewed Sep 14, 2023

View reviewed changes

followynne and others added 5 commits September 16, 2023 09:21

Update ISerilogUiOptionsSetter.cs

db3417c

Update AppStreamLoader.cs

f4c552b

chore(tests): rename mock variables

64d74d4

chore: rename private fields

30bf460

chore: add tests setup comments

32c22b7

followynne and others added 12 commits September 19, 2023 23:02

tests(auth): separate test classes and move utils to correct folder

75f0384

fix(middleware): invert if on path check

716f6c1

chore: remove folder from csproj

5c7a863

chore: add missing braces

4f9f9bc

chore: remove extra var

4f9567f

fix(ui-endpoints): serializer options as static

f8e4c23

fix(decorators): separate class, create local func

a591ff6

chore: rename methods to async, clean directives

1d4fdec

fix: revert if in auth service

2f7d607

chore: move newtonsoft to core

5dcdd56

chore: clean test method

60ee772

Merge branch 'master' into feat/refactor-ui-web-backend

d33c5fc

followynne requested a review from mo-esmp September 19, 2023 21:48

mo-esmp approved these changes Sep 20, 2023

View reviewed changes

followynne merged commit 44fc5c7 into serilog-contrib:master Sep 20, 2023
3 checks passed

followynne deleted the feat/refactor-ui-web-backend branch September 20, 2023 08:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ui-web): refactor the web backend, create tests #88

feat(ui-web): refactor the web backend, create tests #88

followynne commented Sep 1, 2023 •

edited

Loading

mo-esmp commented Sep 1, 2023

followynne commented Sep 1, 2023

mo-esmp Sep 13, 2023

followynne Sep 19, 2023

mo-esmp Sep 13, 2023

followynne Sep 19, 2023

mo-esmp Sep 13, 2023

followynne Sep 19, 2023

mo-esmp Sep 13, 2023

followynne Sep 19, 2023

mo-esmp Sep 13, 2023

followynne Sep 19, 2023

mo-esmp Sep 13, 2023

followynne Sep 19, 2023

mo-esmp Sep 13, 2023

followynne Sep 19, 2023

mo-esmp Sep 13, 2023

followynne Sep 19, 2023

mo-esmp Sep 13, 2023

followynne Sep 19, 2023

mo-esmp Sep 13, 2023

followynne Sep 19, 2023

mo-esmp Sep 14, 2023

followynne Sep 19, 2023

mo-esmp Sep 14, 2023

followynne Sep 19, 2023

mo-esmp Sep 14, 2023

followynne Sep 19, 2023

mo-esmp Sep 14, 2023

followynne Sep 19, 2023

mo-esmp Sep 14, 2023

followynne Sep 19, 2023

followynne commented Sep 19, 2023

mo-esmp commented Sep 20, 2023


		var provider = httpContext.RequestServices.GetService<AggregateDataProvider>();

		string key = dbKey;



		[Trait("Ui-Authorization", "Web")]
		public class AuthorizationDefaultTest : IClassFixture<WebAppFactory.Default>

feat(ui-web): refactor the web backend, create tests #88

feat(ui-web): refactor the web backend, create tests #88

Conversation

followynne commented Sep 1, 2023 • edited Loading

mo-esmp commented Sep 1, 2023

followynne commented Sep 1, 2023

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

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

followynne commented Sep 19, 2023

mo-esmp commented Sep 20, 2023

followynne commented Sep 1, 2023 •

edited

Loading