Fix for type collisions for types referenced from OData actions #1215
Conversation
|
Kudos, SonarCloud Quality Gate passed! |
| type = CreateModelDeclarationAndType(currentNode, schema?.Items, operation, codeNamespace); | ||
| if (type == null) | ||
| { | ||
| var targetNamespace = schema?.Items == null ? codeNamespace : GetShortestNamespace(codeNamespace, schema.Items); |
There was a problem hiding this comment.
We evaluate this case similar to objects so that if a collection item has a schema, we will look at the same place(namespace) and avoid duplicating models in different namespaces.
| @@ -820,7 +823,7 @@ private CodeTypeBase CreateModelDeclarations(OpenApiUrlTreeNode currentNode, Ope | |||
| var parentSchema = referencedAllOfs.FirstOrDefault(); | |||
| if(parentSchema != null) { | |||
| var parentClassNamespace = GetShortestNamespace(currentNamespace, parentSchema); | |||
| inheritsFrom = AddModelDeclarationIfDoesntExit(currentNode, parentSchema, parentSchema.GetSchemaTitle(), parentClassNamespace, null, true) as CodeClass; | |||
| inheritsFrom = AddModelDeclarationIfDoesntExist(currentNode, parentSchema, parentSchema.GetSchemaTitle(), parentClassNamespace, null, !parentSchema.IsReferencedSchema()) as CodeClass; | |||
There was a problem hiding this comment.
Only set to true if we don't have schema. The parentClassNamespace should ideally contain the type if the schema is referenced as the namespace is consistently evaluated from the schema (if it exists).
With this change, we know which namespace the type should be if we have a reference schema so no need to search all namespaces.
| @@ -851,7 +854,7 @@ private CodeTypeBase CreateModelDeclarations(OpenApiUrlTreeNode currentNode, Ope | |||
| var shortestNamespaceName = GetModelsNamespaceNameFromReferenceId(propertyDefinitionSchema.Reference?.Id); | |||
| var targetNamespace = string.IsNullOrEmpty(shortestNamespaceName) ? ns : | |||
| (rootNamespace.FindNamespaceByName(shortestNamespaceName) ?? rootNamespace.AddNamespace(shortestNamespaceName)); | |||
| definition = AddModelDeclarationIfDoesntExit(currentNode, propertyDefinitionSchema, className, targetNamespace, null, true); | |||
| definition = AddModelDeclarationIfDoesntExist(currentNode, propertyDefinitionSchema, className, targetNamespace, null, !propertyDefinitionSchema.IsReferencedSchema()); | |||
There was a problem hiding this comment.
Only set to true if we don't have schema. The targetNamespace should ideally contain the type if the schema is referenced as the namespace is consistently evaluated from the schema (if it exists).
Leaving to true results to flaky generation of Types as sometimes it might resolve something like Microsoft.Graph.Group vs Microsoft.Graph.TermStore.Group depending on which namespace is dug up first(if set to true).
With this change, we know which namespace the type should be if we have a reference schema so no need to search all namespaces.
andrueastman
requested review from
baywet,
darrelmiller,
zengin,
MIchaelMainer,
ddyett,
peombwa,
nikithauc and
ramsessanchez
as code owners
andrueastman
added
the
generator
|
Great work rootcausing this issue! Thanks for the detailed explanation.
You have more trust in the code I wrote than I do 🤣 Jokes aside the "namespace and naming calculation code" has been built iteratively, by trial and error following increasing complexity of the different descriptions I was testing with, and this might show in the current structure (compared to a fully designed ahead approach). I'm going to disable the auto merge and merge as admin because the CLA check doesn't seem to be working at the moment. |
This PR closes #1198
Changes include
Evaluate the
targetNamespacein the event an array has a reference schema (similar to how objects are handled).This fixes the duplication of types models in other namespaces and now will be placed in the namespace as expected from the schema. (E.g we would see
Messagemodel in the default models namespace and in a function/action namespace and this would type collisions if someone imported both)Eliminate the explicit setting of the
checkInAllNameSpacesto true when callingAddModelDeclarationIfDoesntExitas this produces flaky generation results.This has now been limited to scenarios when the schema in use does not have a reference (essentially when the schema is inline?). This is because the namespace resolution seems solid and the type/model should be found in a consistent manner in the event there is a reference. Explicitly setting to true results in property types changing on different runs and models missing/or showing up in different runs. because of sometimes resolving Microsoft.Graph.Group instead of Microsoft.Graph.TermStore.Group(and vice versa based on random conditions lol).
Refactor typo in method name
AddModelDeclarationIfDoesntExistNotes