try-to-fix-the-non-required-default-value #26920
Conversation
qiaozha
requested review from
xirzec,
jeremymeng and
deyaaeldeen
as code owners
|
API change check API changes are not detected in this pull request. |
| }, | ||
| }; | ||
|
|
||
| const serialized = Serializer.serialize( |
There was a problem hiding this comment.
need a deserialize test rather than a serialize one
There was a problem hiding this comment.
Thanks for pointing it out :)
silly me 😂
There was a problem hiding this comment.
It took me a little bit to notice as well 😄
There was a problem hiding this comment.
I found if I change the serialize into deserialize and revert all the other changes back, the test will fail, it seems like we have some issues when handling flatten? Sorry I haven't get time to look into the details yet.
Also, as flatten will also need special serialization / deserialization in Modular, and I heard other language DPGs are using TypeSpec alias to do the flatten, I might need to investigate it a little first.
| @@ -239,7 +239,7 @@ class SerializerImpl implements Serializer { | |||
| responseBody = []; | |||
| } | |||
| // specifically check for undefined as default value can be a falsey value `0, "", false, null` | |||
| if (mapper.defaultValue !== undefined) { | |||
| if (mapper.required && mapper.defaultValue !== undefined) { | |||
There was a problem hiding this comment.
I feel like this is a pretty big change in runtime behavior. To ensure we didn't break we'd have to audit every use of defaultValue in a mapper for all SDKs we've shipped.
|
@xirzec I didn't mean to fix this issue either, I was going to create a draft PR just to see the impact of if we choose to fix it, then I had a wrong click when creating this PR :( Actually I am wondering if we should keep this deserialize logic for default value in our Modular. Previously, I have this PR Azure/autorest.typescript#1977 to remove the default value logic because of the reasons in this comment Azure/autorest.typescript#1975 (comment). now we only add default value for constant type in Modular. But if we have the deserialization in the current core-client, it will be another breaking when we switch from HLC to Modular, especially for mgmt plane ? |
I think it's okay to do something different in the new codegen since it won't affect previously shipped clients.
This is a good question, but perhaps part of the conversion process can be us searching for any non-constant usage of defaultValue and evaluating if it matters or not. If we find there are cases where it is important to include the default value, we could potentially add an emitter option to make the behavior opt-in on a per SDK basis. Does that sound like a reasonable approach to you? |
|
close it as no need to fix it in the current core. |
Packages impacted by this PR
Issues associated with this PR
Describe the problem that is addressed by this PR
What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
Are there test cases added in this PR? (If not, why?)
Provide a list of related PRs (if any)
Command used to generate this PR:**(Applicable only to SDK release request PRs)
Checklists