-
Notifications
You must be signed in to change notification settings - Fork 292
Provide an error message from Open api description #6953
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
base: main
Are you sure you want to change the base?
Changes from 8 commits
bfc848a
1c3d3e8
b4c5f47
9bd10a8
3d40934
2131100
e287fd2
19491e5
159e96a
5856ca0
425f81c
353c1ba
c9b42ee
3e226d6
5c7a961
cee1363
e77d204
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,6 +107,8 @@ public override Task RefineAsync(CodeNamespace generatedCode, CancellationToken | |
| AbstractionsNamespaceName | ||
| ); | ||
| AddConstructorsForDefaultValues(generatedCode, false); | ||
| AddConstructorsForErrorClasses(generatedCode); | ||
| AddMessageFactoryMethodForErrorClasses(generatedCode); | ||
| AddDiscriminatorMappingsUsingsToParentClasses( | ||
| generatedCode, | ||
| "IParseNode" | ||
|
|
@@ -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)))) | ||
|
||
| { | ||
| 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); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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) | ||||||
|
|
@@ -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"); | ||||||
|
||||||
| 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"); |
Outdated
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.
@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.
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.
Why do you think this would be confusing for the 4XX/5XX case?
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.
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".
Outdated
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.
| currentMethod.Parameters.Any(p => p.IsOfKind(CodeParameterKind.ErrorMessage))) | |
| currentMethod.Parameters.Any(static p => p.IsOfKind(CodeParameterKind.ErrorMessage))) |
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.
There's probably a static analyser trick I'm missing here that would warn contributors about lambdas that can be static?
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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))); | ||
|
||
|
|
||
| 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 | ||
| } | ||
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.
should the try add result be used in the condition here?
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.
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 :)
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.
actually I didn't notice but we're duplicating a lot of code with the existing method.
Please: