Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/39 path matching bugfix #43

Closed
wants to merge 2 commits into from
Closed

Feature/39 path matching bugfix #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af568cf
Added some basic specs for OpenApiDocumentProvider, as well as a fail…
jongeorge1 May 21, 2020
a959699
Added support for the Server override in the root and "Path", and han…
mwadams May 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 53 additions & 13 deletions Solutions/Menes.Abstractions/Menes/Internal/OpenApiDocumentProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,20 @@
namespace Menes
{
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.IO;
using System.Linq;
using System.Reflection.Metadata;
using System.Text;
using System.Text.Encodings.Web;
using System.Text.RegularExpressions;
using Corvus.Extensions;
using Menes.Exceptions;
using Menes.Internal;
using Menes.Validation;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.Extensions.Logging;
using Microsoft.OpenApi.Models;
using Tavis.UriTemplates;
Expand Down Expand Up @@ -41,6 +46,8 @@ namespace Menes
/// </remarks>
public class OpenApiDocumentProvider : IOpenApiDocumentProvider
{
private static readonly ConcurrentDictionary<OpenApiServer, Regex> RegexCache = new ConcurrentDictionary<OpenApiServer, Regex>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So...what is this? I think it's a lazily populated dictionary of regular expressions intended to match the url of a Server Object in an API spec. However, I think there may be a subtle problem here.

I presume the reason you're using RegEx here, and not just matching with string.StartsWith is that the server url property supports Server Variables, so it might look like https://{appname}.azurewebsites.net/{apiVersion}. The OpenAPI spec apparently makes a decision of convenience here: in order to support using variables in this way, the URL is, syntactically speaking, a URL template. (They don't explicitly say that in the url property docs, but they do mention it in the Server Object's variables docs.)

And so you've used Tavis.UriTemplates to build a RegEx that matches strings against URL templates. Sounds perfectly sensible. Except I don't think it'll do what you meant.

Take an even simpler example than the one I gave: https://{appname}.azurewebsites.net/. That will match literally any Azure web app. I don't think that's what you actually mean. And let me give an example of why.

In OpenApiDocumentProvider.feature you have this:

Scenario Outline: Match requests to operation path templates - failure

and you include this line in the examples:

| No matching path | https://duff.server.io/v2/pet/65 | GET |

I venture to suggest that if I were to add this similar line:

| No matching path | https://duff.swagger.io/v2/pet/65 | GET |

you would also expect the test to pass (i.e., it the code under test should fail in the way the test says it should fail) for exactly the same reason the previous example fails: the server name is wrong here.

And indeed it does fail. But only because your example OpenAPI spec doesn't hit the problem I'm describing. Now consider changing the OpenAPI spec by changing the first server's url thus:

servers:
  - url: 'https://{servername}.swagger.io/v2'
    variables:
      servername:
        default: petstore
        enum: [petstore, petstoredev, petstoretest]

(And this is exactly how this feature of OpenAPI is meant to be used as far as I can tell.) The presence of that enum line constrains this to be one of three possible values, meaning that there are exactly three possible roots for the https form here. That means that while https://petstore.swagger.io/v2/pet/65, https://petstoredev.swagger.io/v2/pet/65, and https://petstoretest.swagger.io/v2/pet/65 test are all valid, https://duff.swagger.io/v2/pet/65 definitely isn't.

However, with the YAML set up in that way, and with the extra test scenario example added as shown above, the test now fails, because the Regex you end up creating here is effectively matching https://*.swagger.io/v2.

That is the correct logic for cases where a URI Template is being used in the most usual way, i.e., to match anything that conforms to the template. But it's not what's required here.

It's not entirely clear to me what is really required here. I'm not sure whether the intention is that individual environments can say what value they want to plug in for each server variable, or whether it may actually be valid for a single environment to be able to specify multiple values for each variable, and for things to match any time the URL matches for any combination of the variables.

But what does seem clear is that the current behaviour is demonstrably wrong: it will accept any value in a template placeholder even when the relevant Open API spec clearly states that this is not allowed (as in my example above).

So my suggestion for now would be simply not to support variable-based server URLs, and to revisit this if we ever turn out to need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right - this is exactly what I didn't fully understand about the intended use of those variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better just to support matching the possible variable substitutions. I also note that I have missed the fact that there are Operation overrides as well as Path overrides for the servers and so I need to support both of those cases too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your suggestion to defer this to "a later date" - the later date being imminent on the type generation branch.


private readonly IList<OpenApiPathTemplate> pathTemplates = new List<OpenApiPathTemplate>();
private readonly ILogger<OpenApiDocumentProvider> logger;
private readonly List<OpenApiDocument> addedOpenApiDocuments;
Expand Down Expand Up @@ -137,7 +144,7 @@ public void Add(OpenApiDocument document)
}

this.addedOpenApiDocuments.Add(document);
this.pathTemplates.AddRange(document.Paths.Select(path => new OpenApiPathTemplate(path.Key, path.Value)));
this.pathTemplates.AddRange(document.Paths.Select(path => new OpenApiPathTemplate(path.Key, path.Value, document)));
this.pathTemplatesByOperationId = null;
if (this.logger.IsEnabled(LogLevel.Trace))
{
Expand All @@ -150,20 +157,27 @@ public bool GetOperationPathTemplate(string requestPath, string method, [NotNull
{
foreach (OpenApiPathTemplate template in this.pathTemplates)
{
if (template.Match.IsMatch(requestPath))
Match match = template.Match.Match(requestPath);

if (match.Success)
{
if (template.PathItem.Operations.TryGetValue(method.ToOperationType(), out OpenApiOperation operation))
OpenApiServer? server = MatchServer(match, requestPath, template);

if (server != null)
{
this.logger.LogInformation(
"Matched request [{method}] [{requestPath}] with template [{template}] to [{operation}]",
method,
requestPath,
template.UriTemplate,
operation.GetOperationId());

// This is the success path
operationPathTemplate = new OpenApiOperationPathTemplate(operation, template);
return true;
if (template.PathItem.Operations.TryGetValue(method.ToOperationType(), out OpenApiOperation operation))
{
this.logger.LogInformation(
"Matched request [{method}] [{requestPath}] with template [{template}] to [{operation}]",
method,
requestPath,
template.UriTemplate,
operation.GetOperationId());

// This is the success path
operationPathTemplate = new OpenApiOperationPathTemplate(operation, template, server);
return true;
}
}
}
}
Expand All @@ -176,5 +190,31 @@ public bool GetOperationPathTemplate(string requestPath, string method, [NotNull
operationPathTemplate = null;
return false;
}

private static OpenApiServer? MatchServer(Match match, string requestPath, OpenApiPathTemplate template)
{
string precursor = match.Index == 0 ? string.Empty : requestPath.Substring(0, match.Index);
if (template.PathItem.Servers.Count > 0)
{
return MatchPrecursor(precursor, template.PathItem.Servers);
}

if (template.Document?.Servers.Count > 0)
{
return MatchPrecursor(precursor, template.Document.Servers);
}

return null;
}

private static OpenApiServer? MatchPrecursor(string precursor, IList<OpenApiServer> servers)
{
return servers.FirstOrDefault(s =>
{
Regex regex = RegexCache.GetOrAdd(s, new Regex(UriTemplate.CreateMatchingRegex2(s.Url), RegexOptions.Compiled));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been unable to work out why UriTemplate has both CreateMatchingRegex and CreateMatchingRegex2. I have been able to find only two differences at https://github.com/tavis-software/Tavis.UriTemplates/blob/master/src/UriTemplates/UriTemplate.cs#L384-L445

  1. A comment after the first statement in the method (absent in the 2 version)
  2. Meaninless whilespace in the case "#": body (absent in the 2 version)

Match match = regex.Match(precursor);
return match.Success && match.Index == 0;
});
}
}
}
23 changes: 21 additions & 2 deletions Solutions/Menes.Abstractions/Menes/OpenApiOperationPathTemplate.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ namespace Menes
using System.Linq;
using Corvus.Extensions;
using Microsoft.OpenApi.Models;
using Tavis.UriTemplates;

/// <summary>
/// A URI match for an operation.
Expand All @@ -20,10 +21,12 @@ public class OpenApiOperationPathTemplate
/// </summary>
/// <param name="operation">The matching OpenAPI operation.</param>
/// <param name="openApiPathTemplate">The matching OpenAPI path template.</param>
public OpenApiOperationPathTemplate(OpenApiOperation operation, OpenApiPathTemplate openApiPathTemplate)
/// <param name="server">The server for this path template.</param>
public OpenApiOperationPathTemplate(OpenApiOperation operation, OpenApiPathTemplate openApiPathTemplate, OpenApiServer? server)
{
this.Operation = operation;
this.OpenApiPathTemplate = openApiPathTemplate;
this.Server = server;
}

/// <summary>
Expand All @@ -36,6 +39,11 @@ public OpenApiOperationPathTemplate(OpenApiOperation operation, OpenApiPathTempl
/// </summary>
public OpenApiPathTemplate OpenApiPathTemplate { get; }

/// <summary>
/// Gets the server associated with this path template.
/// </summary>
public OpenApiServer? Server { get; }

/// <summary>
/// Builds the list of Open API parameters for this match.
/// </summary>
Expand All @@ -58,7 +66,18 @@ public IList<OpenApiParameter> BuildOpenApiParameters()
/// <returns>A dictionary of parameter names to values.</returns>
public IDictionary<string, object> BuildTemplateParameterValues(Uri requestUri)
{
return this.OpenApiPathTemplate.UriTemplate.GetParameters(requestUri);
IDictionary<string, object> pathParameters = this.OpenApiPathTemplate.UriTemplate.GetParameters(requestUri);

if (this.Server != null)
{
IDictionary<string, object> serverParameters = new UriTemplate(this.Server.Url).GetParameters(requestUri);
if (serverParameters != null)
{
pathParameters = pathParameters.Merge(serverParameters);
}
}

return pathParameters;
}
}
}
11 changes: 9 additions & 2 deletions Solutions/Menes.Abstractions/Menes/OpenApiPathTemplate.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,13 @@ public class OpenApiPathTemplate
/// </summary>
/// <param name="uriTemplate">The uri template.</param>
/// <param name="item">The path item.</param>
public OpenApiPathTemplate(string uriTemplate, OpenApiPathItem item)
/// <param name="document">The Open API document in which this is a path template.</param>
public OpenApiPathTemplate(string uriTemplate, OpenApiPathItem item, OpenApiDocument? document)
{
this.UriTemplate = new UriTemplate(uriTemplate);
this.PathItem = item;
this.Match = new Regex(UriTemplate.CreateMatchingRegex(uriTemplate), RegexOptions.Compiled);
this.Match = new Regex(UriTemplate.CreateMatchingRegex2(uriTemplate), RegexOptions.Compiled);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...so you've found the need to change to the 2 form so you must have spotted a substantial difference! But what?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"2"

this.Document = document;
}

/// <summary>
Expand All @@ -39,5 +41,10 @@ public OpenApiPathTemplate(string uriTemplate, OpenApiPathItem item)
/// Gets the <see cref="Regex"/> which provides a match.
/// </summary>
public Regex Match { get; }

/// <summary>
/// Gets the <see cref="OpenApiDocument"/> containing this path template.
/// </summary>
public OpenApiDocument? Document { get; }
}
}
3 changes: 2 additions & 1 deletion Solutions/Menes.Hosting.AspNetCore/Menes/Hosting/AspNetCore/HttpRequestExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ namespace Menes.Hosting.AspNetCore
using System.Threading.Tasks;
using Menes;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Extensions;
using Microsoft.AspNetCore.Mvc;

/// <summary>
Expand All @@ -23,7 +24,7 @@ public static class HttpRequestExtensions
/// <returns>The result of the request.</returns>
public static Task<IActionResult> HandleRequestAsync(this IOpenApiHost<HttpRequest, IActionResult> host, HttpRequest httpRequest, object parameters)
{
return host.HandleRequestAsync(httpRequest.Path, httpRequest.Method, httpRequest, parameters);
return host.HandleRequestAsync(httpRequest.GetDisplayUrl(), httpRequest.Method, httpRequest, parameters);
}
}
}
Loading