Fix missing URI scheme in Swagger when using PollConsul service discovery #308
Conversation
|
Thanks for opening this pull request! If you have implemented new functions, write about them in the readme file. |
WalkthroughThe pull request introduces modifications to the Changes
Possibly related issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/MMLib.SwaggerForOcelot/ServiceDiscovery/SwaggerServiceDiscoveryProvider.cs (1)
141-142: Approved: Improved method formatting with a suggestionThe reformatting of
GetErrorMessageto use an expression-bodied member improves code conciseness and readability, which is a good practice in modern C#.Consider fixing the typo in the error message:
- $"Service with swagger documentation '{endPoint.Service.Name}' cann't be discovered"; + $"Service with swagger documentation '{endPoint.Service.Name}' cannot be discovered";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/MMLib.SwaggerForOcelot/Middleware/SwaggerForOcelotMiddleware.cs (4 hunks)
- src/MMLib.SwaggerForOcelot/ServiceDiscovery/SwaggerServiceDiscoveryProvider.cs (2 hunks)
🔇 Additional comments (6)
src/MMLib.SwaggerForOcelot/ServiceDiscovery/SwaggerServiceDiscoveryProvider.cs (3)
110-112: Excellent addition to handle missing URI schemeThis change directly addresses the PR objective of fixing the missing URI scheme issue when using PollConsul service discovery. The null check and fallback mechanism ensure that a scheme is always set, improving the robustness of the code.
128-137: Improved code formatting enhances readabilityThe reformatting of the
GetSchememethod improves code readability without changing its functionality. The use of pattern matching with the switch expression is maintained, which is a good practice in modern C#.
139-139: Approval of nullableServiceProviderTypepropertyThe modification of
ServiceProviderTypeto be nullable aligns with modern C# practices for better null handling. This change might be related to the PR objectives, potentially allowing for null values when the service provider type is not set.Could you please clarify the specific reason for making this property nullable? It would be helpful to understand how this change relates to the overall PR objectives.
src/MMLib.SwaggerForOcelot/Middleware/SwaggerForOcelotMiddleware.cs (3)
72-73: Improved code readabilityThe split of the tuple assignment into two lines enhances code readability without changing the functionality. This change aligns with good coding practices.
Line range hint
1-180: Summary of changes and their impactThe changes in this file successfully address the PR objective of fixing the missing URI scheme issue when using PollConsul service discovery. The modifications ensure that the Swagger content is not cleared when using PollConsul, which is consistent with the PR description.
Key points:
- The code readability has been improved by splitting a tuple assignment.
- The condition for handling different service discovery providers has been updated to include PollConsul.
These changes appear to solve the reported issue without introducing new problems. However, consider the suggested refactoring of the condition structure for better maintainability in the future.
Overall, the implementation looks solid and achieves the intended goal of the PR.
96-97: Approved change with suggestion for improvementThe addition of the PollConsul condition addresses the PR objective of fixing the missing URI scheme issue when using PollConsul service discovery. This change ensures that the Swagger content is not cleared when using PollConsul, which is consistent with the PR description.
However, I suggest refactoring the condition for better readability and maintainability:
-if (SwaggerServiceDiscoveryProvider.ServiceProviderType != "Consul" && - SwaggerServiceDiscoveryProvider.ServiceProviderType != "PollConsul") +if (!new[] { "Consul", "PollConsul" }.Contains(SwaggerServiceDiscoveryProvider.ServiceProviderType))This refactoring makes it easier to add or remove service provider types in the future without changing the condition structure.
To ensure this change doesn't have unintended consequences, let's verify the usage of
SwaggerServiceDiscoveryProvider.ServiceProviderTypeacross the codebase:This will help us identify any other places in the code that might need similar updates.
✅ Verification successful
Verified the refactoring suggestion
The search results confirm that
SwaggerServiceDiscoveryProvider.ServiceProviderTypeis only used in the reviewed condition withinSwaggerForOcelotMiddleware.cs. This ensures that the proposed refactoring will not affect other parts of the codebase.Approved the refactoring suggestion.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of SwaggerServiceDiscoveryProvider.ServiceProviderType # Search for all occurrences of SwaggerServiceDiscoveryProvider.ServiceProviderType echo "Occurrences of SwaggerServiceDiscoveryProvider.ServiceProviderType:" rg --type csharp "SwaggerServiceDiscoveryProvider\.ServiceProviderType" # Search for other conditions related to Consul or PollConsul echo "\nOther conditions related to Consul or PollConsul:" rg --type csharp "(Consul|PollConsul)"Length of output: 980
src/MMLib.SwaggerForOcelot/ServiceDiscovery/SwaggerServiceDiscoveryProvider.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/MMLib.SwaggerForOcelot/ServiceDiscovery/SwaggerServiceDiscoveryProvider.cs (2)
142-142: Approve new property and suggest improvementsThe addition of the
ServiceProviderTypeproperty is good and aligns with the PR objectives. However, there are a couple of improvements that can be made:
- Add XML documentation for the property to improve code documentation.
- Consider adding a
#nullable enabledirective at the file level to properly enable nullable reference types.Here's a suggested improvement:
/// <summary> /// Gets or sets the type of the service provider. /// </summary> public static string? ServiceProviderType { get; set; }Also, consider adding
#nullable enableat the top of the file if it's not already present.🧰 Tools
🪛 GitHub Check: build-and-test
[warning] 142-142:
The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
[warning] 142-142:
Missing XML comment for publicly visible type or member 'SwaggerServiceDiscoveryProvider.ServiceProviderType'
144-145: Approve formatting change and fix typoThe reformatting of the
GetErrorMessagemethod to use an expression-bodied member improves code conciseness. This is a good change.However, there's a small typo in the error message that should be corrected.
Please correct the typo in the error message:
- $"Service with swagger documentation '{endPoint.Service.Name}' cann't be discovered"; + $"Service with swagger documentation '{endPoint.Service.Name}' can't be discovered";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/MMLib.SwaggerForOcelot/ServiceDiscovery/SwaggerServiceDiscoveryProvider.cs (2 hunks)
🧰 Additional context used
🪛 GitHub Check: build-and-test
src/MMLib.SwaggerForOcelot/ServiceDiscovery/SwaggerServiceDiscoveryProvider.cs
[warning] 142-142:
The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
[warning] 142-142:
Missing XML comment for publicly visible type or member 'SwaggerServiceDiscoveryProvider.ServiceProviderType'
🔇 Additional comments (2)
src/MMLib.SwaggerForOcelot/ServiceDiscovery/SwaggerServiceDiscoveryProvider.cs (2)
131-140: Approve formatting changesThe reformatting of the
GetSchememethod using a switch expression improves code readability without changing the underlying logic. This is a good improvement.
Line range hint
1-145: Summary of changesThe changes in this file successfully address the PR objective of fixing the missing URI scheme issue in Swagger when using PollConsul service discovery. The code quality has been improved with better formatting and readability.
Key improvements:
- Added a check for null or empty scheme in the UriBuilder.
- Reformatted the
GetSchememethod for better readability.- Added a new
ServiceProviderTypeproperty to handle different service discovery provider types.- Reformatted the
GetErrorMessagemethod for conciseness.Minor suggestions for further improvement have been provided in the individual comments. Overall, these changes look good and should resolve the reported issue.
🧰 Tools
🪛 GitHub Check: build-and-test
[warning] 142-142:
The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
[warning] 142-142:
Missing XML comment for publicly visible type or member 'SwaggerServiceDiscoveryProvider.ServiceProviderType'
| if (builder.Scheme.IsNullOrEmpty()) | ||
| { | ||
| builder.Scheme = conf?.Scheme ?? "http"; | ||
| } | ||
| builder.Scheme = conf?.Scheme ?? "http"; | ||
|
|
There was a problem hiding this comment.
Approve changes and remove duplicate line
The added check for a null or empty scheme addresses the PR objective of fixing the missing URI scheme issue in Swagger. This change looks good and should resolve the reported problem.
However, there's a duplicate line that should be removed:
Please remove the duplicate line:
if (builder.Scheme.IsNullOrEmpty())
{
builder.Scheme = conf?.Scheme ?? "http";
}
- builder.Scheme = conf?.Scheme ?? "http";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (builder.Scheme.IsNullOrEmpty()) | |
| { | |
| builder.Scheme = conf?.Scheme ?? "http"; | |
| } | |
| builder.Scheme = conf?.Scheme ?? "http"; | |
| if (builder.Scheme.IsNullOrEmpty()) | |
| { | |
| builder.Scheme = conf?.Scheme ?? "http"; | |
| } |
|
close #307 |
This PR addresses an issue where the scheme is missing from the Swagger URI when using the PollConsul service discovery provider.
Changes:
SwaggerServiceDiscoveryProvider.GetSwaggerUri(...)to set the scheme if missing.ServiceProviderTypeisPollConsul.This fixes issue #307 .
Summary by CodeRabbit
New Features
Schemeproperty in the Swagger URI builder, improving configuration handling.Improvements
Bug Fixes
Schemeproperty is null or empty.