OData / AspNetCoreOData

ASP.NET Core OData: A server library built upon ODataLib and ASP.NET Core
Other
457 stars 158 forks source link

Report invalid routes as errors (optionally?) [enhancement] #321

Open kcaswick opened 3 years ago

kcaswick commented 3 years ago

When an entire controller is marked with [ODataAttributeRouting], I expect invalid OData routes to be clearly reported, not a warning buried in the log files. I would be fine opting into this with a parameter to the attribute like UseStrict=True, setting ODataOptions.EnableStrictRouting, or something.

I expect this would throw an exception on template parsing errors, and attribute routes that fail to match a prefix. Maybe even an error for any route on such a controller that is not an OData route, and does not have [ODataIgnored], no matter what the reason.

xuzhg commented 3 years ago

@kcaswick Thanks for sharing your thoughts.

Actually, the current implementation is a trade-off from a long period of discussion.

In 7.x and earlier versions, we throw expect if odata template parses errors. It introduces a lot of problems.

kcaswick commented 3 years ago

It's unfortunate that it introduces a lot of problems. Maybe there could be a specific validate call instead.

It's not pretty, but for now I've added a unit test that captures log output during Startup and fails if there are any warnings or errors. It seems to catch the invalid route templates, like when I forget to replace '[controller]' when changing an existing controller to OData.

using FluentAssertions;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using System;
using System.Linq;
using Xunit;
using Xunit.Abstractions;

    public class StartupTests : TestBase, IClassFixture<CustomWebApplicationFactory<Startup>>
    {
        private readonly CustomWebApplicationFactory<Startup> factory;
        public StartupTests(ITestOutputHelper testOutputHelper, Fixtures.CustomWebApplicationFactory<Startup> factory) : 
            base(testOutputHelper)
        {
            this.factory = factory;
        }
        [Fact]
        public void Startup_NoWarningsLogged_Succeeds()
        {
            // Arrange
            // CaptureTestOutputHelper is an internal wrapper of ITestOutputHelper that saves each line to an IList<string>
            var log = new _company_.Testing.CaptureTestOutputHelper(_testOutputHelper, "sut: ");
            var myFactory = factory.WithWebHostBuilder(webBuilder => webBuilder.ConfigureServices(services =>
            {
                services.AddSingleton<ILogger>(log.BuildLogger());
                services.AddLogging(config =>
                {
                    config.AddXunit(log);
                });
            }));

            // Act
            var server = myFactory.Server;

            // Assert
            server.Should().NotBeNull();
            log.Captured.Should().NotBeEmpty();
            log.Captured.Should().NotContain(x => x.Contains("Warning [0]:") || 
                x.Contains("Error [0]:"), "no warnings or errors should occur");
            // This can replace the above line without custom CaptureTestOutputHelper class, the output is just less 
            // readable because it doesn't list which lines failed
            (_testOutputHelper as Xunit.Sdk.TestOutputHelper).Output.Should()
                .NotContain("Warning [0]:").And
                .NotContain("Error [0]:");
        }
    }