-
Notifications
You must be signed in to change notification settings - Fork 96
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.
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
Refactor Swagger Endpoint Validation and Implement Custom Options Handling #315
base: master
Are you sure you want to change the base?
Changes from all commits
c71cd65
b3d41e6
9486732
68e34af
da22005
3d16d55
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
<Project Sdk="Microsoft.NET.Sdk.Web"> | ||
|
||
<PropertyGroup> | ||
<TargetFramework>net8.0</TargetFramework> | ||
<Nullable>enable</Nullable> | ||
<ImplicitUsings>enable</ImplicitUsings> | ||
</PropertyGroup> | ||
|
||
<ItemGroup> | ||
<PackageReference Include="Microsoft.AspNetCore.OpenApi" Version="8.0.4"/> | ||
<PackageReference Include="Swashbuckle.AspNetCore" Version="6.5.0" /> | ||
</ItemGroup> | ||
|
||
<ItemGroup> | ||
<ProjectReference Include="..\..\..\src\MMLib.ServiceDiscovery.Consul\MMLib.ServiceDiscovery.Consul.csproj" /> | ||
</ItemGroup> | ||
|
||
</Project> |
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,50 @@ | ||||||||||||||||
using MMLib.ServiceDiscovery.Consul.DependencyInjection; | ||||||||||||||||
|
||||||||||||||||
var builder = WebApplication.CreateBuilder(args); | ||||||||||||||||
|
||||||||||||||||
// Add services to the container. | ||||||||||||||||
// Learn more about configuring Swagger/OpenAPI at https://aka.ms/aspnetcore/swashbuckle | ||||||||||||||||
builder.Services.AddEndpointsApiExplorer(); | ||||||||||||||||
builder.Services.AddSwaggerGen(); | ||||||||||||||||
|
||||||||||||||||
builder.AddConsulAutoServiceDiscovery("http://localhost:8500"); //This line added | ||||||||||||||||
|
||||||||||||||||
var app = builder.Build(); | ||||||||||||||||
|
||||||||||||||||
// Configure the HTTP request pipeline. | ||||||||||||||||
if (app.Environment.IsDevelopment()) | ||||||||||||||||
{ | ||||||||||||||||
app.UseSwagger(); | ||||||||||||||||
app.UseSwaggerUI(); | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
app.UseHttpsRedirection(); | ||||||||||||||||
|
||||||||||||||||
app.UseSwaggerForOcelotUI(); //This line added | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Limit Swagger UI exposure to development environment The Proposed change: 15 if (app.Environment.IsDevelopment())
16 {
17 app.UseSwagger();
18 app.UseSwaggerUI();
+ app.UseSwaggerForOcelotUI(); // Moved inside the development check
19 }
- app.UseSwaggerForOcelotUI(); //This line removed 📝 Committable suggestion
Suggested change
|
||||||||||||||||
|
||||||||||||||||
var summaries = new[] | ||||||||||||||||
{ | ||||||||||||||||
"Freezing", "Bracing", "Chilly", "Cool", "Mild", "Warm", "Balmy", "Hot", "Sweltering", "Scorching" | ||||||||||||||||
}; | ||||||||||||||||
|
||||||||||||||||
app.MapGet("/weatherforecast", () => | ||||||||||||||||
{ | ||||||||||||||||
var forecast = Enumerable.Range(1, 5).Select(index => | ||||||||||||||||
new WeatherForecast | ||||||||||||||||
( | ||||||||||||||||
DateOnly.FromDateTime(DateTime.Now.AddDays(index)), | ||||||||||||||||
Random.Shared.Next(-20, 55), | ||||||||||||||||
summaries[Random.Shared.Next(summaries.Length)] | ||||||||||||||||
)) | ||||||||||||||||
.ToArray(); | ||||||||||||||||
return forecast; | ||||||||||||||||
}) | ||||||||||||||||
.WithName("GetWeatherForecast") | ||||||||||||||||
.WithOpenApi(); | ||||||||||||||||
|
||||||||||||||||
app.Run("http://localhost:7002"); | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Avoid hardcoding the application URL in Hardcoding the application URL directly in Proposed change: - app.Run("http://localhost:7002");
+ app.Run(); Then, in your {
"profiles": {
"AutoDiscoveryApi": {
"commandName": "Project",
"launchBrowser": true,
"applicationUrl": "http://localhost:7002",
"environmentVariables": {
"ASPNETCORE_ENVIRONMENT": "Development"
}
}
}
} |
||||||||||||||||
|
||||||||||||||||
record WeatherForecast(DateOnly Date, int TemperatureC, string? Summary) | ||||||||||||||||
{ | ||||||||||||||||
public int TemperatureF => 32 + (int)(TemperatureC / 0.5556); | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Use standard formula for Celsius to Fahrenheit conversion The current calculation for Proposed change: - public int TemperatureF => 32 + (int)(TemperatureC / 0.5556);
+ public int TemperatureF => 32 + (int)(TemperatureC * 9 / 5); 📝 Committable suggestion
Suggested change
|
||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
"Logging": { | ||
"LogLevel": { | ||
"Default": "Information", | ||
"Microsoft.AspNetCore": "Warning" | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
{ | ||
"Logging": { | ||
"LogLevel": { | ||
"Default": "Information", | ||
"Microsoft.AspNetCore": "Warning" | ||
} | ||
}, | ||
"AllowedHosts": "*" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
<Project Sdk="Microsoft.NET.Sdk.Web"> | ||
|
||
<PropertyGroup> | ||
<TargetFramework>net8.0</TargetFramework> | ||
<Nullable>enable</Nullable> | ||
<ImplicitUsings>enable</ImplicitUsings> | ||
</PropertyGroup> | ||
|
||
<ItemGroup> | ||
<PackageReference Include="Microsoft.AspNetCore.OpenApi" Version="8.0.4"/> | ||
<PackageReference Include="Swashbuckle.AspNetCore" Version="6.4.0"/> | ||
</ItemGroup> | ||
|
||
<ItemGroup> | ||
<ProjectReference Include="..\..\..\src\MMLib.SwaggerForOcelot\MMLib.SwaggerForOcelot.csproj" /> | ||
</ItemGroup> | ||
rabdulatif marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
using Ocelot.DependencyInjection; | ||
using Ocelot.Provider.Consul; | ||
using Ocelot.Middleware; | ||
|
||
var builder = WebApplication.CreateBuilder(args); | ||
builder.Services.AddEndpointsApiExplorer(); | ||
|
||
builder.Configuration | ||
.SetBasePath(builder.Environment.ContentRootPath) | ||
.AddJsonFile("ocelot.json", optional: false, reloadOnChange: true) | ||
.AddEnvironmentVariables(); | ||
|
||
builder.Services | ||
.AddOcelot(builder.Configuration) | ||
.AddConsul(); | ||
|
||
builder.Services.AddSwaggerForOcelot(builder.Configuration); | ||
|
||
var app = builder.Build(); | ||
|
||
app.UseSwaggerForOcelotUI(); | ||
await app.UseOcelot(); | ||
|
||
app.MapGet("/", () => "Hello World!").WithOpenApi(); | ||
app.Run("http://0.0.0.0:7001"); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
"Logging": { | ||
"LogLevel": { | ||
"Default": "Information", | ||
"Microsoft.AspNetCore": "Warning" | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
{ | ||
"Logging": { | ||
"LogLevel": { | ||
"Default": "Information", | ||
"Microsoft.AspNetCore": "Warning" | ||
} | ||
}, | ||
"AllowedHosts": "*" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
{ | ||
"Routes": [], | ||
"SwaggerEndPoints": [], | ||
"GlobalConfiguration": { | ||
"ServiceDiscoveryProvider": { | ||
"Scheme": "http", | ||
"Host": "localhost", | ||
"Port": 8500, | ||
"Type": "PollConsul", | ||
"PollingInterval": 100 | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,10 +1,10 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||
using Microsoft.AspNetCore.Builder; | ||||||||||||||||||||||||||||||||||||||||||||||||||
using Microsoft.AspNetCore.Hosting; | ||||||||||||||||||||||||||||||||||||||||||||||||||
using Microsoft.AspNetCore.Mvc.ApiExplorer; | ||||||||||||||||||||||||||||||||||||||||||||||||||
using Microsoft.Extensions.Configuration; | ||||||||||||||||||||||||||||||||||||||||||||||||||
using Microsoft.Extensions.DependencyInjection; | ||||||||||||||||||||||||||||||||||||||||||||||||||
using Microsoft.Extensions.Options; | ||||||||||||||||||||||||||||||||||||||||||||||||||
using Microsoft.Extensions.PlatformAbstractions; | ||||||||||||||||||||||||||||||||||||||||||||||||||
using MMLib.ServiceDiscovery.Consul.DependencyInjection; | ||||||||||||||||||||||||||||||||||||||||||||||||||
using Swashbuckle.AspNetCore.SwaggerGen; | ||||||||||||||||||||||||||||||||||||||||||||||||||
using System.IO; | ||||||||||||||||||||||||||||||||||||||||||||||||||
using System.Reflection; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -67,6 +67,7 @@ public void ConfigureServices(IServiceCollection services) | |||||||||||||||||||||||||||||||||||||||||||||||||
// integrate xml comments | ||||||||||||||||||||||||||||||||||||||||||||||||||
options.IncludeXmlComments(XmlCommentsFilePath); | ||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
/// <summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -79,15 +80,34 @@ public void Configure(IApplicationBuilder app, IApiVersionDescriptionProvider pr | |||||||||||||||||||||||||||||||||||||||||||||||||
app.UseRouting(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
app.UseEndpoints(endpoints => { endpoints.MapControllers(); }); | ||||||||||||||||||||||||||||||||||||||||||||||||||
app.UseSwagger(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
_ = app.UseSwaggerUI( | ||||||||||||||||||||||||||||||||||||||||||||||||||
options => | ||||||||||||||||||||||||||||||||||||||||||||||||||
// _ = app.UseSwaggerUI( | ||||||||||||||||||||||||||||||||||||||||||||||||||
// options => | ||||||||||||||||||||||||||||||||||||||||||||||||||
// { | ||||||||||||||||||||||||||||||||||||||||||||||||||
// // build a swagger endpoint for each discovered API version | ||||||||||||||||||||||||||||||||||||||||||||||||||
// foreach (ApiVersionDescription description in provider.ApiVersionDescriptions) | ||||||||||||||||||||||||||||||||||||||||||||||||||
// { | ||||||||||||||||||||||||||||||||||||||||||||||||||
// options.SwaggerEndpoint($"/swagger/{description.GroupName}/swagger.json", description.GroupName.ToUpperInvariant()); | ||||||||||||||||||||||||||||||||||||||||||||||||||
// } | ||||||||||||||||||||||||||||||||||||||||||||||||||
// }); | ||||||||||||||||||||||||||||||||||||||||||||||||||
app.UseSwaggerUI(c => | ||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||
c.RoutePrefix = "swagger"; | ||||||||||||||||||||||||||||||||||||||||||||||||||
foreach (var description in provider.ApiVersionDescriptions) | ||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||
c.SwaggerEndpoint($"/swagger/{description.GroupName}/swagger.json", $"Version {description.GroupName}"); | ||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+92
to
+99
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Swagger JSON files not found for all API versions. The search identified Swagger JSON files in:
Ensure that Swagger JSON files are generated for each API version as per the PR objectives. 🔗 Analysis chainVerify the Swagger UI configuration for multiple API versions The new configuration sets the To confirm that the Swagger endpoints are correctly configured for all API versions, you can check for the presence of generated Swagger JSON files for each version. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify that Swagger JSON files exist for each API version.
# Test: List all swagger.json files. Expect: Files for each API version.
fd --extension json 'swagger' ./wwwroot/swagger
Length of output: 152 Script: #!/bin/bash
# Description: Search for all swagger.json files in the project to verify Swagger endpoints for all API versions.
fd --extension json 'swagger\.json'
Length of output: 146 |
||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
// Separate endpoints that contain only one version | ||||||||||||||||||||||||||||||||||||||||||||||||||
foreach (var description in provider.ApiVersionDescriptions) | ||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||
app.UseSwaggerUI(c => | ||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||
// build a swagger endpoint for each discovered API version | ||||||||||||||||||||||||||||||||||||||||||||||||||
foreach (ApiVersionDescription description in provider.ApiVersionDescriptions) | ||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||
options.SwaggerEndpoint($"/swagger/{description.GroupName}/swagger.json", description.GroupName.ToUpperInvariant()); | ||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||
c.RoutePrefix = $"swagger/{description.GroupName}"; | ||||||||||||||||||||||||||||||||||||||||||||||||||
c.SwaggerEndpoint($"/swagger/{description.GroupName}/swagger.json", $"Version {description.GroupName}"); | ||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+101
to
+110
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid multiple calls to Calling Suggested Refactor: You can merge the configurations to handle multiple API versions within a single app.UseSwaggerUI(c =>
{
c.RoutePrefix = "swagger";
foreach (var description in provider.ApiVersionDescriptions)
{
c.SwaggerEndpoint($"/swagger/{description.GroupName}/swagger.json", $"Version {description.GroupName}");
+ // Optionally, add additional settings per version here if needed
}
+ // Remove the loop that calls app.UseSwaggerUI multiple times
});
-// Separate endpoints that contain only one version
-foreach (var description in provider.ApiVersionDescriptions)
-{
- app.UseSwaggerUI(c =>
- {
- c.RoutePrefix = $"swagger/{description.GroupName}";
- c.SwaggerEndpoint($"/swagger/{description.GroupName}/swagger.json", $"Version {description.GroupName}");
- });
-} This adjustment ensures that 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
static string XmlCommentsFilePath | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
using Microsoft.AspNetCore.Builder; | ||
using Microsoft.Extensions.DependencyInjection; | ||
using Swashbuckle.AspNetCore.SwaggerUI; | ||
|
||
namespace MMLib.ServiceDiscovery.Consul.DependencyInjection; | ||
|
||
/// <summary> | ||
/// | ||
/// </summary> | ||
public static class ConfigureExtensions | ||
{ | ||
/// <summary> | ||
/// | ||
/// </summary> | ||
/// <param name="builder"></param> | ||
/// <returns></returns> | ||
public static IApplicationBuilder UseSwaggerForOcelotUI(this WebApplication builder) | ||
{ | ||
builder.ConfigureConsulConnection(); | ||
builder.MapHealthChecks("/api/health"); | ||
|
||
return builder; | ||
} | ||
Comment on lines
+17
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Revise method name and implementation to match functionality The
Here's a suggested implementation: public static WebApplication ConfigureConsulAndHealthChecks(this WebApplication app)
{
app.ConfigureConsulConnection();
app.MapHealthChecks("/api/health");
return app;
} If Swagger UI setup is needed, consider adding it to this method or creating a separate method for it. |
||
|
||
/// <summary> | ||
/// | ||
/// </summary> | ||
/// <param name="builder"></param> | ||
/// <returns></returns> | ||
public static IApplicationBuilder ConfigureConsulConnection(this IApplicationBuilder builder) | ||
{ | ||
var consul = builder.ApplicationServices.GetService<IConsulConnectionService>(); | ||
consul?.Start(); | ||
|
||
return builder; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Avoid hardcoding the Consul service discovery URL
Hardcoding the Consul service URL reduces flexibility and complicates deployment to different environments. It's recommended to retrieve the URL from configuration files or environment variables. This approach enhances maintainability and allows easy configuration changes without modifying code.
Proposed change:
Ensure that the
ConsulConfig:ConsulUrl
setting is defined in your configuration files (e.g.,appsettings.json
):