Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions src/Kiota.Builder/CodeDOM/CodeMethod.cs
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ public void DeduplicateErrorMappings()
public bool HasUrlTemplateOverride => !string.IsNullOrEmpty(UrlTemplateOverride);

private ConcurrentDictionary<string, CodeTypeBase> errorMappings = new(StringComparer.OrdinalIgnoreCase);
private ConcurrentDictionary<string, string> errorDescriptions = new(StringComparer.OrdinalIgnoreCase);

/// <summary>
/// Mapping of the error code and response types for this method.
Expand All @@ -265,6 +266,17 @@ public IOrderedEnumerable<KeyValuePair<string, CodeTypeBase>> ErrorMappings
return errorMappings.OrderBy(static x => x.Key);
}
}

/// <summary>
/// Mapping of the error code and response descriptions from OpenAPI spec for this method.
/// </summary>
public IOrderedEnumerable<KeyValuePair<string, string>> ErrorDescriptions
{
get
{
return errorDescriptions.OrderBy(static x => x.Key);
}
}
public bool HasErrorMappingCode(string code)
{
ArgumentException.ThrowIfNullOrEmpty(code);
Expand Down Expand Up @@ -304,6 +316,7 @@ public object Clone()
Parent = Parent,
OriginalIndexer = OriginalIndexer,
errorMappings = new(errorMappings),
errorDescriptions = new(errorDescriptions),
AcceptedResponseTypes = new List<string>(AcceptedResponseTypes),
PagingInformation = PagingInformation?.Clone() as PagingInformation,
Documentation = (CodeDocumentation)Documentation.Clone(),
Expand All @@ -330,4 +343,20 @@ public void AddErrorMapping(string errorCode, CodeTypeBase type)
ArgumentException.ThrowIfNullOrEmpty(errorCode);
errorMappings.TryAdd(errorCode, type);
}

public void AddErrorMapping(string errorCode, CodeTypeBase type, string description)
{
ArgumentNullException.ThrowIfNull(type);
ArgumentException.ThrowIfNullOrEmpty(errorCode);
errorMappings.TryAdd(errorCode, type);
if (!string.IsNullOrEmpty(description))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the try add result be used in the condition here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it, it is quite a corner case where consumer expectations are hard to guess - for this to matter, you'd need duplicate error code mappings where one has a description and the other hasn't.
But you have seen more OpenAPI specs than I so if you say this actually happens and the current behavior is unexpected, then the answer is yes :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually I didn't notice but we're duplicating a lot of code with the existing method.
Please:

  • add the description parameter as an optional parameter to the existing method
  • move the condition to the existing method
  • update the condition to consider the try add, so we don't end up in a weird state where we have a description for an error type that doesn't exist
  • remove this new method
  • Update the GetErrorDescriptionMethod to always check whether a matching mapping is present before returning anything (same principle, safety first)

errorDescriptions.TryAdd(errorCode, description);
}

public string? GetErrorDescription(string errorCode)
{
ArgumentException.ThrowIfNullOrEmpty(errorCode);
errorDescriptions.TryGetValue(errorCode, out var description);
return description;
}
}
2 changes: 1 addition & 1 deletion src/Kiota.Builder/KiotaBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1292,7 +1292,7 @@ private void AddErrorMappingToExecutorMethod(OpenApiUrlTreeNode currentNode, Ope
{
if (!codeClass.IsErrorDefinition)
codeClass.IsErrorDefinition = true;
executorMethod.AddErrorMapping(errorCode, errorType);
executorMethod.AddErrorMapping(errorCode, errorType, response.Description ?? string.Empty);
}
else
logger.LogWarning("Could not create error type for {Error} in {Operation}", errorCode, operation.OperationId);
Expand Down
94 changes: 94 additions & 0 deletions src/Kiota.Builder/Refiners/CSharpRefiner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ public override Task RefineAsync(CodeNamespace generatedCode, CancellationToken
AbstractionsNamespaceName
);
AddConstructorsForDefaultValues(generatedCode, false);
AddConstructorsForErrorClasses(generatedCode);
AddMessageFactoryMethodForErrorClasses(generatedCode);
AddDiscriminatorMappingsUsingsToParentClasses(
generatedCode,
"IParseNode"
Expand Down Expand Up @@ -268,4 +270,96 @@ private void SetTypeAccessModifiers(CodeElement currentElement)

CrawlTree(currentElement, SetTypeAccessModifiers);
}

private static void AddConstructorsForErrorClasses(CodeElement currentElement)
{
if (currentElement is CodeClass codeClass && codeClass.IsErrorDefinition)
{
// Add parameterless constructor if not already present
if (!codeClass.Methods.Any(x => x.IsOfKind(CodeMethodKind.Constructor) && !x.Parameters.Any()))
{
var parameterlessConstructor = CreateConstructor(codeClass, "Instantiates a new {TypeName} and sets the default values.");
codeClass.AddMethod(parameterlessConstructor);
}

// Add message constructor if not already present
if (!codeClass.Methods.Any(x => x.IsOfKind(CodeMethodKind.Constructor) && x.Parameters.Any(p => p.Type.Name.Equals("string", StringComparison.OrdinalIgnoreCase))))
{
var messageConstructor = CreateConstructor(codeClass, "Instantiates a new {TypeName} with the specified error message.");

// Add message parameter
messageConstructor.AddParameter(new CodeParameter
{
Name = "message",
Type = new CodeType { Name = "string", IsExternal = true },
Optional = false,
Documentation = new()
{
DescriptionTemplate = "The error message"
}
});

codeClass.AddMethod(messageConstructor);
}
}
CrawlTree(currentElement, AddConstructorsForErrorClasses);
}

private static void AddMessageFactoryMethodForErrorClasses(CodeElement currentElement)
{
if (currentElement is CodeClass codeClass &&
codeClass.IsErrorDefinition &&
!codeClass.Methods.Any(m => m.Name == "CreateFromDiscriminatorValueWithMessage"))
{
var method = codeClass.AddMethod(new CodeMethod
{
Name = "CreateFromDiscriminatorValueWithMessage",
Kind = CodeMethodKind.Factory,
IsAsync = false,
IsStatic = true,
Documentation = new(new() {
{"TypeName", new CodeType {
IsExternal = false,
TypeDefinition = codeClass,
}}
})
{
DescriptionTemplate = "Creates a new instance of the appropriate class based on discriminator value with a custom error message.",
},
Access = AccessModifier.Public,
ReturnType = new CodeType
{
Name = codeClass.Name,
TypeDefinition = codeClass,
},
Parent = codeClass,
}).Single();

// Add parseNode parameter
method.AddParameter(new CodeParameter
{
Name = "parseNode",
Type = new CodeType { Name = "IParseNode", IsExternal = true },
Kind = CodeParameterKind.ParseNode,
Optional = false,
Documentation = new()
{
DescriptionTemplate = "The parse node to use to read the discriminator value and create the object"
}
});

// Add message parameter
method.AddParameter(new CodeParameter
{
Name = "message",
Type = new CodeType { Name = "string", IsExternal = true },
Optional = false,
Documentation = new()
{
DescriptionTemplate = "The error message to set on the created object"
}
});
}
CrawlTree(currentElement, AddMessageFactoryMethodForErrorClasses);
}
}
47 changes: 28 additions & 19 deletions src/Kiota.Builder/Refiners/CommonLanguageRefiner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -243,28 +243,37 @@ protected static void AddConstructorsForDefaultValues(CodeElement current, bool
currentClass.Properties.Any(static x => !string.IsNullOrEmpty(x.DefaultValue)) ||
addIfInherited && DoesAnyParentHaveAPropertyWithDefaultValue(currentClass)) &&
!currentClass.Methods.Any(x => x.IsOfKind(CodeMethodKind.ClientConstructor)))
currentClass.AddMethod(new CodeMethod
{
Name = "constructor",
Kind = CodeMethodKind.Constructor,
ReturnType = new CodeType
{
Name = "void"
},
IsAsync = false,
Documentation = new(new() {
{ "TypeName", new CodeType() {
IsExternal = false,
TypeDefinition = current,
}}
})
{
DescriptionTemplate = "Instantiates a new {TypeName} and sets the default values.",
},
});
currentClass.AddMethod(CreateConstructor(currentClass, "Instantiates a new {TypeName} and sets the default values."));
CrawlTree(current, x => AddConstructorsForDefaultValues(x, addIfInherited, forceAdd, classKindsToExclude));
}

protected static CodeMethod CreateConstructor(CodeClass parentClass, string descriptionTemplate, AccessModifier access = AccessModifier.Public)
{
return new CodeMethod
{
Name = "constructor",
Kind = CodeMethodKind.Constructor,
ReturnType = new CodeType
{
Name = "void",
IsExternal = true
},
IsAsync = false,
IsStatic = false,
Access = access,
Documentation = new(new() {
{ "TypeName", new CodeType() {
IsExternal = false,
TypeDefinition = parentClass,
}}
})
{
DescriptionTemplate = descriptionTemplate,
},
Parent = parentClass,
};
}

protected static void ReplaceReservedModelTypes(CodeElement current, IReservedNamesProvider provider, Func<string, string> replacement) =>
ReplaceReservedNames(current,
provider,
Expand Down
34 changes: 33 additions & 1 deletion src/Kiota.Builder/Writers/CSharp/CodeMethodWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using Kiota.Builder.OrderComparers;

namespace Kiota.Builder.Writers.CSharp;

public class CodeMethodWriter : BaseElementWriter<CodeMethod, CSharpConventionService>
{
public CodeMethodWriter(CSharpConventionService conventionService) : base(conventionService)
Expand Down Expand Up @@ -203,6 +204,15 @@ private void WriteFactoryMethodBody(CodeMethod codeElement, CodeClass parentClas
WriteFactoryMethodBodyForUnionModel(codeElement, parentClass, parseNodeParameter, writer);
else if (parentClass.DiscriminatorInformation.ShouldWriteDiscriminatorForIntersectionType)
WriteFactoryMethodBodyForIntersectionModel(codeElement, parentClass, parseNodeParameter, writer);
else if (codeElement.Name == "CreateFromDiscriminatorValueWithMessage" && parentClass.IsErrorDefinition)
{
// Special case: CreateFromDiscriminatorValueWithMessage for error classes
var messageParam = codeElement.Parameters.FirstOrDefault(p => p.Name == "message");
if (messageParam != null)
writer.WriteLine($"return new {parentClass.GetFullName()}({messageParam.Name});");
else
writer.WriteLine($"return new {parentClass.GetFullName()}();");
}
else
writer.WriteLine($"return new {parentClass.GetFullName()}();");
}
Expand Down Expand Up @@ -401,7 +411,21 @@ protected void WriteRequestExecutorBody(CodeMethod codeElement, RequestParams re
writer.StartBlock();
foreach (var errorMapping in codeElement.ErrorMappings.Where(errorMapping => errorMapping.Value.AllTypes.FirstOrDefault()?.TypeDefinition is CodeClass))
{
writer.WriteLine($"{{ \"{errorMapping.Key.ToUpperInvariant()}\", {conventions.GetTypeString(errorMapping.Value, codeElement, false)}.CreateFromDiscriminatorValue }},");
var errorClass = errorMapping.Value.AllTypes.FirstOrDefault()?.TypeDefinition as CodeClass;
var typeName = conventions.GetTypeString(errorMapping.Value, codeElement, false);

if (errorClass?.IsErrorDefinition == true)
{
var errorDescription = codeElement.GetErrorDescription(errorMapping.Key);
var statusCodeAndDescription = !string.IsNullOrEmpty(errorDescription)
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@baywet I'm having second thoughts on this - I think we shouldn't include the status code key here because it may be confusing in the 4XX/5XX case.
So, just the description when provided and the old behavior when not.
The actual specific response status code has value though; #6951 would help to provide that without conflicting.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you think this would be confusing for the 4XX/5XX case?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 5XX is not part of the API designer provided error message and the actual response code provides more detailed info. I don't see added value in an ApiException that says "400 4XX Your mistake" over "400 Your mistake".

? $"{errorMapping.Key} {errorDescription}"
: errorMapping.Key;
writer.WriteLine($"{{ \"{errorMapping.Key.ToUpperInvariant()}\", (parseNode) => {typeName}.CreateFromDiscriminatorValueWithMessage(parseNode, \"{statusCodeAndDescription}\") }},");
}
else
{
writer.WriteLine($"{{ \"{errorMapping.Key.ToUpperInvariant()}\", {typeName}.CreateFromDiscriminatorValue }},");
}
}
writer.CloseBlock("};");
}
Expand Down Expand Up @@ -608,9 +632,17 @@ private static string GetBaseSuffix(bool isConstructor, bool inherits, CodeClass
}
return " : base()";
}
// For error classes with message constructor, pass the message to base constructor
else if (isConstructor && parentClass.IsErrorDefinition &&
currentMethod.Parameters.Any(p => p.Name.Equals("message", StringComparison.OrdinalIgnoreCase) &&
p.Type.Name.Equals("string", StringComparison.OrdinalIgnoreCase)))
{
return " : base(message)";
}

return string.Empty;
}

private void WriteMethodPrototype(CodeMethod code, CodeClass parentClass, LanguageWriter writer, string returnType, bool inherits, bool isVoid)
{
var staticModifier = code.IsStatic ? "static " : string.Empty;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -961,5 +961,84 @@ public async Task SetTypeAccessModifierAsync(AccessModifier accessModifier)
Assert.Equal(codeClass.Access, accessModifier);
Assert.Equal(codeEnum.Access, accessModifier);
}

[Fact]
public async Task AddsMessageConstructorToErrorClasses()
{
// Given
var errorClass = root.AddClass(new CodeClass
{
Name = "Error401",
IsErrorDefinition = true
}).First();

// When
await ILanguageRefiner.RefineAsync(new GenerationConfiguration { Language = GenerationLanguage.CSharp }, root);

// Then
var messageConstructor = errorClass.Methods
.FirstOrDefault(m => m.IsOfKind(CodeMethodKind.Constructor) &&
m.Parameters.Any(p => p.Type.Name.Equals("string", StringComparison.OrdinalIgnoreCase) && p.Name.Equals("message", StringComparison.OrdinalIgnoreCase)));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should test on the new kind that was introduced (multiple instances)


Assert.NotNull(messageConstructor);
Assert.Single(messageConstructor.Parameters);
Assert.Equal("message", messageConstructor.Parameters.First().Name);
Assert.Equal("string", messageConstructor.Parameters.First().Type.Name);
Assert.False(messageConstructor.Parameters.First().Optional);
}

[Fact]
public async Task DoesNotAddMessageConstructorToNonErrorClasses()
{
// Given
var regularClass = root.AddClass(new CodeClass
{
Name = "RegularModel",
IsErrorDefinition = false
}).First();

// When
await ILanguageRefiner.RefineAsync(new GenerationConfiguration { Language = GenerationLanguage.CSharp }, root);

// Then
var messageConstructor = regularClass.Methods
.FirstOrDefault(m => m.IsOfKind(CodeMethodKind.Constructor) &&
m.Parameters.Any(p => p.Type.Name.Equals("string", StringComparison.OrdinalIgnoreCase) && p.Name.Equals("message", StringComparison.OrdinalIgnoreCase)));

Assert.Null(messageConstructor);
}

[Fact]
public async Task AddsMessageFactoryMethodToErrorClasses()
{
// Given
var errorClass = root.AddClass(new CodeClass
{
Name = "Error401",
IsErrorDefinition = true
}).First();

// When
await ILanguageRefiner.RefineAsync(new GenerationConfiguration { Language = GenerationLanguage.CSharp }, root);

// Then
var messageFactoryMethod = errorClass.Methods
.FirstOrDefault(m => m.IsOfKind(CodeMethodKind.Factory) &&
m.Name.Equals("CreateFromDiscriminatorValueWithMessage", StringComparison.OrdinalIgnoreCase));

Assert.NotNull(messageFactoryMethod);
Assert.Equal(2, messageFactoryMethod.Parameters.Count());

var parseNodeParam = messageFactoryMethod.Parameters.FirstOrDefault(p => p.Name.Equals("parseNode", StringComparison.OrdinalIgnoreCase));
Assert.NotNull(parseNodeParam);
Assert.Equal("IParseNode", parseNodeParam.Type.Name);

var messageParam = messageFactoryMethod.Parameters.FirstOrDefault(p => p.Name.Equals("message", StringComparison.OrdinalIgnoreCase));
Assert.NotNull(messageParam);
Assert.Equal("string", messageParam.Type.Name);

Assert.True(messageFactoryMethod.IsStatic);
Assert.Equal(AccessModifier.Public, messageFactoryMethod.Access);
}
#endregion
}
Loading