Skip to content
Draft
Show file tree
Hide file tree
Changes from 8 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
35 changes: 34 additions & 1 deletion src/Kiota.Builder/CodeDOM/CodeMethod.cs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,11 @@ public enum CodeMethodKind
/// <summary>
/// The override for the error message for the error/exception type.
/// </summary>
ErrorMessageOverride
ErrorMessageOverride,
/// <summary>
/// Factory method for error classes that accepts an error message parameter.
/// </summary>
FactoryWithErrorMessage,
}
public enum HttpMethod
{
Expand Down Expand Up @@ -254,6 +258,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 +270,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, StringComparer.Ordinal);
}
}
public bool HasErrorMappingCode(string code)
{
ArgumentException.ThrowIfNullOrEmpty(code);
Expand Down Expand Up @@ -304,6 +320,7 @@ public object Clone()
Parent = Parent,
OriginalIndexer = OriginalIndexer,
errorMappings = new(errorMappings),
errorDescriptions = new(errorDescriptions, StringComparer.Ordinal),
AcceptedResponseTypes = new List<string>(AcceptedResponseTypes),
PagingInformation = PagingInformation?.Clone() as PagingInformation,
Documentation = (CodeDocumentation)Documentation.Clone(),
Expand All @@ -330,4 +347,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
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
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
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;
}
}
4 changes: 4 additions & 0 deletions src/Kiota.Builder/CodeDOM/CodeParameter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ public enum CodeParameterKind
/// This is only used for languages that use static functions for the serialization as opposed to instance methods since the OOP inheritance correctly handles that case.
/// </summary>
SerializingDerivedType,
/// <summary>
/// Error message parameter for error/exception class constructors and factory methods.
/// </summary>
ErrorMessage,
}

public class CodeParameter : CodeTerminalWithKind<CodeParameterKind>, ICloneable, IDocumentedElement, IDeprecableElement
Expand Down
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
89 changes: 89 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,91 @@ private void SetTypeAccessModifiers(CodeElement currentElement)

CrawlTree(currentElement, SetTypeAccessModifiers);
}

private static CodeParameter CreateErrorMessageParameter(string descriptionTemplate = "The error message")
{
return new CodeParameter
{
Name = "message",
Type = new CodeType { Name = "string", IsExternal = true },
Kind = CodeParameterKind.ErrorMessage,
Optional = false,
Documentation = new()
{
DescriptionTemplate = descriptionTemplate
}
};
}

private static void AddConstructorsForErrorClasses(CodeElement currentElement)
{
if (currentElement is CodeClass codeClass && codeClass.IsErrorDefinition)
{
// Add parameterless constructor if not already present
if (!codeClass.Methods.Any(static 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(static x => x.IsOfKind(CodeMethodKind.Constructor) && x.Parameters.Any(static p => "string".Equals(p.Type.Name, StringComparison.OrdinalIgnoreCase))))
Copy link
Member

Choose a reason for hiding this comment

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

should filter on the new kind instead of the name

{
var messageConstructor = CreateConstructor(codeClass, "Instantiates a new {TypeName} with the specified error message.");
messageConstructor.AddParameter(CreateErrorMessageParameter());
codeClass.AddMethod(messageConstructor);
}
}
CrawlTree(currentElement, AddConstructorsForErrorClasses);
}

private static void AddMessageFactoryMethodForErrorClasses(CodeElement currentElement)
{
const string MethodName = "CreateFromDiscriminatorValueWithMessage";
if (currentElement is CodeClass codeClass &&
codeClass.IsErrorDefinition &&
!codeClass.Methods.Any(m => m.Name == MethodName))
{
var method = codeClass.AddMethod(new CodeMethod
{
Name = MethodName,
Kind = CodeMethodKind.FactoryWithErrorMessage,
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(CreateErrorMessageParameter("The error message to set on the created object"));
}
CrawlTree(currentElement, AddMessageFactoryMethodForErrorClasses);
}
}
46 changes: 27 additions & 19 deletions src/Kiota.Builder/Refiners/CommonLanguageRefiner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -243,28 +243,36 @@ 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,
},
};
}

protected static void ReplaceReservedModelTypes(CodeElement current, IReservedNamesProvider provider, Func<string, string> replacement) =>
ReplaceReservedNames(current,
provider,
Expand Down
27 changes: 26 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,11 @@ 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.IsOfKind(CodeMethodKind.FactoryWithErrorMessage))
{
var messageParam = codeElement.Parameters.FirstOrDefault(p => p.IsOfKind(CodeParameterKind.ErrorMessage)) ?? throw new InvalidOperationException($"FactoryWithErrorMessage should have a message parameter");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var messageParam = codeElement.Parameters.FirstOrDefault(p => p.IsOfKind(CodeParameterKind.ErrorMessage)) ?? throw new InvalidOperationException($"FactoryWithErrorMessage should have a message parameter");
var messageParam = codeElement.Parameters.FirstOrDefault(static p => p.IsOfKind(CodeParameterKind.ErrorMessage)) ?? throw new InvalidOperationException($"FactoryWithErrorMessage should have a message parameter");

writer.WriteLine($"return new {parentClass.GetFullName()}({messageParam.Name});");
}
else
writer.WriteLine($"return new {parentClass.GetFullName()}();");
}
Expand Down Expand Up @@ -401,7 +407,19 @@ 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 typeName = conventions.GetTypeString(errorMapping.Value, codeElement, false);
if (errorMapping.Value.AllTypes.FirstOrDefault()?.TypeDefinition is CodeClass { IsErrorDefinition: true } errorClass)
{
var errorDescription = codeElement.GetErrorDescription(errorMapping.Key);
var statusCodeAndDescription = !string.IsNullOrEmpty(errorDescription)
Copy link
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
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
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 +626,16 @@ 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.IsOfKind(CodeParameterKind.ErrorMessage)))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
currentMethod.Parameters.Any(p => p.IsOfKind(CodeParameterKind.ErrorMessage)))
currentMethod.Parameters.Any(static p => p.IsOfKind(CodeParameterKind.ErrorMessage)))

Copy link
Author

Choose a reason for hiding this comment

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

There's probably a static analyser trick I'm missing here that would warn contributors about lambdas that can be static?

{
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,83 @@ 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
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.FactoryWithErrorMessage));

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