Skip to content

Version parsing from namespace bug fix #366

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

Closed
wants to merge 1 commit into from
Closed

Version parsing from namespace bug fix #366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/Common/Versioning/Conventions/VersionByNamespaceConvention.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ static string GetRawApiVersion( string @namespace )

// 'v' | 'V' : [<year> '-' <month> '-' <day>] : [<major[.minor]>] : [<status>]
// ex: v2018_04_01_1_1_Beta
const string Pattern = @"(?:^|\.)[vV](\d{4})?_?(\d{2})?_?(\d{2})?_?(\d+)?_?(\d*)_?([a-zA-Z][a-zA-Z0-9]*)?(?:$|\.)";
const string Pattern = @"(?:^|\.)[vV](\d{4})?_?(\d{2})?_?(\d{2})?_?(\d+)?_?(\d*)_?([a-zA-Z][a-zA-Z0-9]*)?(?=($|\.))";
Copy link
Collaborator

Choose a reason for hiding this comment

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

While it seems this pattern works, I'm not sure this is the full solution. This change only switches from a non-capture to a look-ahead captured as a group.

Alternative 1

I believe the fix can simply be done by requiring the status to have a leading _ as in:

(?:^|\.)[vV](\d{4})?_?(\d{2})?_?(\d{2})?_?(\d+)?_?(\d*)(_([a-zA-Z][a-zA-Z0-9]*))?(?:$|\.)

Alternative 2

The pattern can be more flexible to not require the leading _, but things get a lot uglier:

(?:^|\.)[vV]((\d{4})_?(\d{2})_?(\d{2})_?(\d+)_?(\d*)|(\d{4})_?(\d{2})_?(\d{2})|(\d+)_?(\d*))_?([a-zA-Z][a-zA-Z0-9]*)?(?:$|\.)

This would be the most flexible and comprehensive solution. Everything after v must be either date, major, and minor, date, or major and minor. Then, and only then, can it be followed by a status. The existing pattern is wrong because it ends up capturing ersioningSample as a status which isn't even valid by itself.

Conclusion

Either of these options will require some small amount of code changes because the grouping also changes.


var match = Regex.Match( @namespace, Pattern, Singleline );
var rawApiVersions = new List<string>();
Expand Down
2 changes: 2 additions & 0 deletions ...spNetCore.Mvc.Versioning.Tests/Versioning/Conventions/VersionByNamespaceConventionTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ public class VersionByNamespaceConventionTest
[InlineData( "Contoso.Api.v2018_04_01_Beta.Controllers", "2018-04-01-Beta" )]
[InlineData( "Contoso.Api.v2018_04_01_1_0_Beta.Controllers", "2018-04-01.1.0-Beta" )]
[InlineData( "MyRestaurant.Vegetarian.Food.v1_1.Controllers", "1.1" )]
[InlineData( "VersioningSample.V5.Controllers", "5.0" )]
public void apply_should_infer_supported_api_version_from_namespace( string @namespace, string versionText )
{
// arrange
Expand Down Expand Up @@ -56,6 +57,7 @@ public void apply_should_infer_supported_api_version_from_namespace( string @nam
[InlineData( "Contoso.Api.v2018_04_01_Beta.Controllers", "2018-04-01-Beta" )]
[InlineData( "Contoso.Api.v2018_04_01_1_0_Beta.Controllers", "2018-04-01.1.0-Beta" )]
[InlineData( "MyRestaurant.Vegetarian.Food.v1_1.Controllers", "1.1" )]
[InlineData( "VersioningSample.V5.Controllers", "5.0" )]
public void apply_should_infer_deprecated_api_version_from_namespace( string @namespace, string versionText )
{
// arrange
Expand Down