-
Notifications
You must be signed in to change notification settings - Fork 316
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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"); | ||||||
|
koen-lee marked this conversation as resolved.
Outdated
|
||||||
| writer.WriteLine($"return new {parentClass.GetFullName()}({messageParam.Name});"); | ||||||
| } | ||||||
| else | ||||||
| writer.WriteLine($"return new {parentClass.GetFullName()}();"); | ||||||
| } | ||||||
|
|
@@ -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) | ||||||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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("};"); | ||||||
| } | ||||||
|
|
@@ -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))) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||||||
|
|
||||||
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: