Bugfix #1124 When CSDL filter and http resource supplied for HIDI input, it fails because ApplyFilter expects a local file but not a URL address#1125
Bugfix #1124 When CSDL filter and http resource supplied for HIDI input, it fails because ApplyFilter expects a local file but not a URL address#1125MazZzDaI wants to merge 1 commit intomicrosoft:vnextmicrosoft/OpenAPI.NET:vnextfrom
Conversation
…because ApplyFilter expects a local file but not a URL address microsoft#1124
|
@microsoft-github-policy-service agree [company="Olk inc."] |
|
@microsoft-github-policy-service agree company="Olk inc." |
baywet
left a comment
There was a problem hiding this comment.
Thanks for the contribution, here are a couple of comments to help improve it
| { | ||
| XslCompiledTransform transform = GetFilterTransform(); | ||
| stream = ApplyFilter(csdl, csdlFilter, transform); | ||
| stream = csdl.StartsWith("http") ? ApplyFilter(stream, csdlFilter, transform) : ApplyFilter(csdl, csdlFilter, transform); |
There was a problem hiding this comment.
| stream = csdl.StartsWith("http") ? ApplyFilter(stream, csdlFilter, transform) : ApplyFilter(csdl, csdlFilter, transform); | |
| stream = csdl.StartsWith("http", StringComparison.OrdinalIgnoreCase) ? ApplyFilter(new StreamReader(stream), csdlFilter, transform) : ApplyFilter(new StreamReader(csdl), csdlFilter, transform); |
There was a problem hiding this comment.
The original code here seems wrong. GetStream does all the logic to determine if stream is coming from a file or via HTTP. Currently the code is getting the stream and then ignoring it if there is a filter. The solution should be to change ApplyFilter to just accept the stream that comes from GetStream. And the case-insensitive check needs to be added to GetStream.
I'm not sure why the OP closed the PR. This seems like a good thing to fix.
| private static Stream ApplyFilter(string csdl, string entitySetOrSingleton, XslCompiledTransform transform) | ||
| { | ||
| StreamReader inputReader = new(csdl); | ||
| return ApplyFilterForStreamReader(inputReader, entitySetOrSingleton, transform); | ||
| } | ||
|
|
||
| private static Stream ApplyFilter(Stream csdlStream, string entitySetOrSingleton, XslCompiledTransform transform) | ||
| { | ||
| StreamReader inputReader = new(csdlStream); | ||
| return ApplyFilterForStreamReader(inputReader, entitySetOrSingleton, transform); | ||
| } | ||
|
|
There was a problem hiding this comment.
| private static Stream ApplyFilter(string csdl, string entitySetOrSingleton, XslCompiledTransform transform) | |
| { | |
| StreamReader inputReader = new(csdl); | |
| return ApplyFilterForStreamReader(inputReader, entitySetOrSingleton, transform); | |
| } | |
| private static Stream ApplyFilter(Stream csdlStream, string entitySetOrSingleton, XslCompiledTransform transform) | |
| { | |
| StreamReader inputReader = new(csdlStream); | |
| return ApplyFilterForStreamReader(inputReader, entitySetOrSingleton, transform); | |
| } |
| } | ||
|
|
||
| private static Stream ApplyFilter(string csdl, string entitySetOrSingleton, XslCompiledTransform transform) | ||
| private static Stream ApplyFilterForStreamReader(StreamReader inputReader, string entitySetOrSingleton, XslCompiledTransform transform) |
There was a problem hiding this comment.
| private static Stream ApplyFilterForStreamReader(StreamReader inputReader, string entitySetOrSingleton, XslCompiledTransform transform) | |
| private static Stream ApplyFilter(StreamReader inputReader, string entitySetOrSingleton, XslCompiledTransform transform) |
| { | ||
| StreamReader inputReader = new(csdlStream); | ||
| return ApplyFilterForStreamReader(inputReader, entitySetOrSingleton, transform); | ||
| } |
Check warning
Code scanning / CodeQL
Missing Dispose call on local IDisposable
Check warning
Code scanning / CodeQL
Missing Dispose call on local IDisposable
|
@MazZzDaI is there any specific reason why you closed that PR? |
Fix of the #1124 issue.
Overload added for Microsoft.OpenApi.Hidi/OpenApiService.cs/ApplyFilter to accept both string and stream variables.