C# client: fix default values for DateTime etc, numeric and boolean defaults were not applied#7414
C# client: fix default values for DateTime etc, numeric and boolean defaults were not applied#7414WolfgangHG wants to merge 28 commits intomicrosoft:mainfrom
Conversation
baywet
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
Added a comment to help drive this to completion.
In addition, could you please add unit tests for the code method writer?
@baywet The code that I changed is completely covered by Could you give me a hint what kind of testing you expect and where I could copy code? |
|
This is essentially an integration test. Thank you for initially adding it. Did you come across the default value tests in coemethodwritertests? I'm suggesting to add more there. Let me know if you have additional questions |
29dc124 to
e073db1
Compare
Perfect, that's what I actually was looking for but didn't find ;-). I added the test for "float value needs the suffix f" to Is the integration test (which parses the full json file) still required? I think so, because it brings the full demo file for this problem. |
|
Thanks for adding the unit tests! I think we're getting really close at this stage. |
|
I added another small commit. Some integration tests (including my new one) did not set I also experimented a bit (not committed) and added Roslyn compilation to the generated clients of the "Kiota.Builder.IntegrationTests" project. Later I found that there is a big integration tests suite in the "it" directory that is only run during Github pull request verification. Probably, there compilation is also done. But this big test probably does not cover the default value handling of this pull request. What do you think? Is the approach to compile the "Kiota.Builder.IntegrationTests" clients worth the effort? Or is this unnecessary? |
Not worth the time. Both in terms of setup, and then in terms of additional runtime of the tests. This is why we set this up as integration tests run by the CI, and not integration tests run as the local test suite. But thank you for considering it. |
|
@baywet I see that a lot of tests are failing, most in java clients. Is this caused by my code? Maybe because I detect default values for basic data types now and they are not handled properly when generating the java client? E.g. here: |
| else if (kind == CodePropertyKind.Custom && | ||
| propertySchema?.Default is JsonValue stringDefaultJsonValue2 && | ||
| !stringDefaultJsonValue2.IsJsonNullSentinel() && | ||
| (stringDefaultJsonValue2.GetValueKind() == JsonValueKind.Number || stringDefaultJsonValue2.GetValueKind() == JsonValueKind.True || stringDefaultJsonValue2.GetValueKind() == JsonValueKind.False)) | ||
| { | ||
| //Values not placed in quotes (number and boolean): just forwared the value. | ||
| prop.DefaultValue = stringDefaultJsonValue2.ToString(); | ||
| } |
There was a problem hiding this comment.
this is causing a regression for Java/Go/Dart.
Similar changes that were made for the CSharp writer need to be made for those languages before we can accept the PR.
There was a problem hiding this comment.
OK, I will take a look later.
|
I hope I fixed the java problems - a client created from my sample JSON file works again, and the client from The sample model "WeatherForecast" model looks like this: public WeatherForecast() {
this.setBoolValue(true);
this.setDateOnlyValue(LocalDate.parse("1900-01-01"));
this.setDateValue(OffsetDateTime.parse("1900-01-01T15:00:00+00:00"));
this.setDateValueLocalTime(java.time.LocalDateTime.parse("1900-01-01T00:00:00").atZone(java.time.ZoneId.systemDefault()).toOffsetDateTime());
this.setDecimalValue(25.5d);
this.setDoubleValue(25.5d);
this.setFloatValue(25.5f);
this.setGuidValue(UUID.fromString("00000000-0000-0000-0000-000000000000"));
this.setSummary("Test");
this.setTemperatureC(15);
this.setTimeValue(LocalTime.parse("00:00:00"));
}Specials compared to C#:
Update 2026-03-27: after thinking once more about the "datetime without timezone" problem, I understood that ISO8601 allows also datetime values without timezone - thats "local time (unqualified)" (https://en.wikipedia.org/wiki/ISO_8601). OffsetDateTime cannot parse them, it fails. Now to the other problems - as I don't know Go or Dart, this might be harder... |
|
About GO: Currently, it generates this: m.SetBoolValue(true)
m.SetDecimalValue(25.5)
m.SetDoubleValue(25.5)
m.SetFloatValue(25.5)...which results in errors like Following https://www.codegenes.net/blog/how-to-set-bool-pointer-to-true-in-struct-literal/, this could be done like this: m.SetBoolValue(func() *bool {
b := true
return &b
}())
m.SetDecimalValue(func() *float64 {
b := 25.5
return &b
}())
m.SetFloatValue(func() *float32 {
b := float32(25.5)
return &b
}())The float value literal causes a compilation error, and the float64 value would do the same if the default has no decimal point. Is there any suffix/prefix to define a float32/float64 literal, or must it be done with @baywet What do you think about my suggestion? By the way: due to my lack of knowledge of theses languages, I would prefer to ignore the date/time default handling hat I added for C#/Java, as they probably never were an issue with GO or Dart previously ;-). |
|
Also a PHP error: https://github.com/microsoft/kiota/actions/runs/22629867053/job/65577135791?pr=7414#step:17:139 The DART error: https://github.com/microsoft/kiota/actions/runs/22629867053/job/65577135767?pr=7414#step:17:75 |
|
Thanks for looking into this! For dart maybe we can ask help from @ricardoboss on this. For go, this is verbose, but I don't have a better solution unless we add short hands in the abstractions library. As for which types are supported, I believe we should strive for parity between stable languages here. |
|
Dart requires default values to be compile time constant, so you can't actually have a default parameter like |
|
Making some slow progress with Go and requesting feedback on the current result. This is the generated Go code for my "WeatherForecast" sample class. I verified that it works and all fields are assigned the expected values. func NewWeatherForecast()(*WeatherForecast) {
m := &WeatherForecast{
}
boolValueValue := true
m.SetBoolValue(&boolValueValue)
dateOnlyValueValue, _ := i878a80d2330e89d26896388a3f487eef27b0a0e6c010c493bf80be1452208f91.ParseDateOnly("1900-01-01")
m.SetDateOnlyValue(dateOnlyValueValue)
dateValueValue, _ := i336074805fc853987abe6f7fe3ad97a6a6f3077a16391fec744f671a015fbd7e.Parse(i336074805fc853987abe6f7fe3ad97a6a6f3077a16391fec744f671a015fbd7e.RFC3339, "1900-01-01T00:00:00+00:00")
m.SetDateValue(&dateValueValue)
decimalValueValue := float64(25.5)
m.SetDecimalValue(&decimalValueValue)
doubleValueValue := float64(25.5)
m.SetDoubleValue(&doubleValueValue)
floatValueValue := float32(25.5)
m.SetFloatValue(&floatValueValue)
guidValueValue, _ := i561e97a8befe7661a44c8f54600992b4207a3a0cf6770e5559949bc276de2e22.Parse("00000000-0000-0000-0000-000000000000")
m.SetGuidValue(&guidValueValue)
summaryValue := "Test"
m.SetSummary(&summaryValue)
temperatureCValue := int32(15)
m.SetTemperatureC(&temperatureCValue)
timeValueValue, _ := i878a80d2330e89d26896388a3f487eef27b0a0e6c010c493bf80be1452208f91.ParseTimeOnly("00:00:00")
m.SetTimeValue(timeValueValue)
return m
}Update 2026-03-31: for DateTime values, I now check the default value whether it contains a timezone or not. A value without timezone results fails to parse in RFC3339 format. But it works to do this (this automatically applies the local timezone to the DateTime): dateValueLocalTimeValue, _ := i33...d7e.ParseInLocation("2006-01-02T15:04:05", "1900-01-01T00:00:00", i33...d7e.Now().Location())
m.SetDateValueLocalTime(&dateValueLocalTimeValue)The first argument ( Attached is the full file: Question 1: is there anything reusable? I see that the generated |
|
Thank you for the additional information. Yes, I like this approach! It's verbose, but consistent at least. Q1: I don't think you'll be able to easily reuse the parse node infrastructure because that not only requires you to have access to the parse node factories, but also get bytes from string, and the representation might vary based on the mime type/available implementation. All of which add layers of complexity that calling a static factory is much simpler. Maybe it'd be worth refactoring things to make sure the parts that emit those lines are defined once and not duplicated. Q2: I've always found that part to be odd in Go's design. Essentially "constructors" don't typically return an error, but the way errors work, they require an extra return value. It does not fit well. Since we don't want to introduce breaking changes, this effectively leaves us with two choices:
My opinion is that failing to set a default because the spec is odd, or because the static factory fails to parse a value, is not a huge deal which should lead the application to crash. So panicking is probably too aggressive. Of course that's something that can always be updated later on if people have issues with errors being ignored. Let me know if you have any additional comments or questions. |
|
Here we go with the Go client. Hope I didn't break it - not having seen Go before three days ago... @baywet Thanks for the feedback about error handling. I agree completely - a parse error while applying the defaults should be avoided. If a user runs into a parse problem, she could still add error logging in the generated code. I am very confused that the Go test @baywet Could you start the Github CI test run so that I can see whether my changes fix the failing Java and Go tests? PHP and Dart are still on my TODO list, but not before next Tuesday. I also don't know those two languages ;-) |
2564a1b to
c96ed51
Compare
|
Are those GO errors also caused by me? For example https://github.com/microsoft/kiota/actions/runs/22796339086/job/66295047630?pr=7414#step:17:35:
|
I'd say yes since one of the files it's erroring with is local (not pulled from an external repo) and since this is only happening on this branch. Let us know if you have any additional comments or questions. |
|
Yes, it was me ;-). Previously, a default value was always set as With the latest commit, I brought back the old behavior: m.SetAdditionalData(make(map[string]any)) |
|
@baywet There is one failing Java test: https://github.com/microsoft/kiota/actions/runs/22796339086/job/66295047215?pr=7414#step:17:201 The error is: The reason is this snippet: "make_latest": {
"default": true,
"description": "Specifies whether this release should be set as the latest release for the repository. ....",
"enum": [
"true",
"false",
"legacy"
],
Shouldn't the default for the enum value be placed in quotes here? Before, it was not detected. Kiota now creates this java snippet: public WithReleasePatchRequestBody() {
this.setAdditionalData(new HashMap<>());
this.setMakeLatest(WithReleasePatchRequestBodyMakeLatest.forValue(true));
}
I could work around by adding quotes to enum default values if they are missing: if (propWithDefault.Type is CodeType propertyType && propertyType.TypeDefinition is CodeEnum enumDefinition)
{
if (!defaultValue.StartsWith('"'))
{
defaultValue = $"\"{defaultValue}\"";
}
defaultValue = $"{enumDefinition.Name}.forValue({defaultValue})";
}How to continue? Add the workaround? Or if my understanding is correct and the syntax is invalid, would it be worth a bug report? Could you also trigger another run of the tests to verify that the Go problems are fixed? |
|
In this example, the default value (in the OpenAPI description) is wrong IMHO. That's because the enum value is "true" (a string) but the default is I'd be ok suppressing the specific operation that's bringing this type in the configuration here for that reason. |
a159a9e to
ac2be7d
Compare
|
I created github/rest-api-description#6100. Seems they fixed one of the problems already. But I am a bit irritated because the file that Kiota downloads differs a lot from the github file. Did I look at the proper file ;-)? @baywet Why not use the approach of the C# client generator instead of calling a parsing method? kiota/src/Kiota.Builder/Writers/CSharp/CodeMethodWriter.cs Lines 256 to 259 in e19028c Currently generated Java code: public WithReleasePatchRequestBody() {
this.setAdditionalData(new HashMap<>());
this.setMakeLatest(WithReleasePatchRequestBodyMakeLatest.forValue("true"));
}The C# approach would result in this code: public WithReleasePatchRequestBody() {
this.setAdditionalData(new HashMap<>());
this.setMakeLatest(WithReleasePatchRequestBodyMakeLatest.True);
}Here, the generated code compiles without errors. Could we use this code? By the way: the "config.json" that you linked contain some suppressions for |
Yes, most descriptions come from the APIs.guru index. Which is often out of date sadly. This is a bit outside of the scope of this pull request but you could do a could of things:
In any case, if any changes are required in kiota's repo, please open a separate pull request to avoid mixing concerns/spead up the review process.
I'm not sure/can't remember why we'd have a difference in the approach between languages here. Could be as simple as the initial language implementers not coordinating. Happy to see things getting aligned assuming it doesn't introduce regressions.
yes, it's probably an oversight. Please do that in a separate PR though. Thanks for the continuous work here. Let me know if you have any additional comments or questions. |
…efaults were not applied
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
…num default from Github api, add ModelWithDefaultValues.json to CI tests
…ss, exclude invalid enum default from Github api only for single method
… test for ModelWithDefaultValues.json
… integration test
…DefaultValues.json
15639d7 to
2b79089
Compare
baywet
left a comment
There was a problem hiding this comment.
Thanks, we're making progress here. Left a few comments. You also will want to merge main in and handle the changelog conflict.
| adapter.setBaseUrl("http://127.0.0.1:1080"); | ||
| var client = new ApiClient(adapter); | ||
|
|
||
| //No request is executed. |
| $requestAdapter->setBaseUrl('http://127.0.0.1:1080'); | ||
| $client = new ApiClient($requestAdapter); | ||
|
|
||
| // Your test logic here |
There was a problem hiding this comment.
isn't the validation entirely missing here?
| if (!string.IsNullOrEmpty(erasableUsing)) | ||
| { | ||
| //Find all usings for this type (might occur multiple times) and set the using to "Erasable = false": | ||
| var erasableUsings = currentClass.Usings.Where(currentUsing => currentUsing.Name == erasableUsing); |
There was a problem hiding this comment.
use a string comparison instead (.Equals(bar, StringComparison.Ordinal))
This is an attempt to fix #7404
If the OpenAPI spec contains model fields with format
date-time,date,timeoruuidand default values, the generated client did not compile because the string value was assigned directly to the property.Default values for numeric types and boolean were not applied at all, because the code ignored all default values that did not start with a quote.
I also tried to add a test and placed my json file in "Kiota.Builder.IntegrationTests" and added a simple test to "GenerateSample.cs", but only for C#. Other languages apparently handle the default values differently.
This test just runs code generating. It might also make sense to test the generated file. But I don't have an idea how you would do it. It would probably perfect if the client code could be compiled using Roslyn and then the model class could be loaded from the dynamic assembly and the field values could be verified. Or I could take at least a look in the generated class, as e.g. test "GeneratesUritemplateHintsAsync" does. What do you think?