-
Notifications
You must be signed in to change notification settings - Fork 43
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
Resolve conflicts when two csdl names have similar suffixes #407
Conversation
test/Microsoft.OData.Cli.Tests/CodeGeneration/Artifacts/SampleServiceV4EnableInternalProxy.cs
Show resolved
Hide resolved
test/Microsoft.OData.Cli.Tests/CodeGeneration/Artifacts/SampleServiceV4EnableTrackingProxy.cs
Show resolved
Hide resolved
@@ -4683,7 +4683,10 @@ namespace <#= fullNamespace #> | |||
try | |||
{ | |||
var assembly = global::System.Reflection.Assembly.GetExecutingAssembly(); | |||
var resourcePath = global::System.Linq.Enumerable.Single(assembly.GetManifestResourceNames(), str => str.EndsWith(filePath)); | |||
// If multiple resource names end with the file name, select the shortest one. | |||
var resourcePath = global::System.Linq.Enumerable.First( |
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.
I feel like we should apply the filter first (i.e., .EndsWith(string)
), then order by length, then pick the first item
I feel like that's more efficient. We'll calculate length for fewer candidates (most times 1):
global::System.Linq.Enumerable.First(
global::System.Linq.Enumerable.OrderBy(
global::System.Linq.Enumerable.Where(
assembly.GetManifestResourceNames(),
str1 => str1.EndsWith(filePath)),
str2 => str2.Length))
There's also the question whether it'd be safer to use FirstOrDefault()
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.
I've made the change.
Fixes #405
We store the CSDL file as an embedded resource. When we want to load the and parse the CSDL in memory, we find the resource that matches the target CSDL file name. We use
resourceName.EndsWith(filePath)
to find the match. However, this causes a conflict when one CSDL name is a substring of the other, e.g.:OData ServiceCsdl.xml
Another OData ServiceCsdl.xml
Currently an exception is thrown when such a conflict is detected. This PR fixes this issue by resolving the conflict by selecting the shortest resource name that ends with the target filename.
This solves the problem for simple cases, and possibly all practical common scenarios. However, it would be problematic in the following cases:
OData ServiceCsdl.xml
in the same project and adds it as an embedded resource.I did not address the concerns because:
Csdl.xml
suffix the service name set in the connected service configuration when generating the CSDL file. We do not expect the user to create files with this exact naming convention manually.The main issue here is that we
EndsWith
to match the files and resources. Why don't we just check the full resource name? The full resource name does not match the file name, it's generated based on the assembly namespace and connected service folder. It as the format:<Namespace>.Connected_services.OData_Services.<filename>
. I am reluctant to generate a match based on the full resource name because I have not seen an official documentation of this resource naming convention. I am concerned about taking a dependency on a format that's not documented when there other easier alternatives. I don't know if there's a guarantee that this naming convention would not be changed in some future version of Visual Studio or .NET without our knowledge.About the tests
Frist, you many notice that while I have updated a lot of generated C# client code in the tests, I have not updated any test generated VB client code. This is because all of the tests in
ODataConnectedServiceTests
test against code that includes the CSDL in the reference code as a string, instead of as a separate file that's compiled as an embedded resource. That code path is not affected by the change in this PR. Only thatOData.Cli.Tests
test this code path. But we don't have any CLI tests for Visual Basic.That made me realize that the OData CLI does not actually support Visual Basic, despite the fact the documentation says it does. I've created a separate issue to track that: #408
Secondly, while we do have tests to verify that the generate code looks as expected and compiles without errors, we don't have tests to verify that the code we generate runs as expected. For example, testing that when you run the generated code with two schemas that have overlapping names does not throw an exception or that the correct schema is selected. We achieve that using manual testing, which is tedious and not enforced consistently. I've created a separate issue to track an E2E testing environment: #409