-
Notifications
You must be signed in to change notification settings - Fork 79
Ignore null for nullable optional properties #3706
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?
Conversation
Co-authored-by: JialinHuang803 <139532647+JialinHuang803@users.noreply.github.com>
Co-authored-by: JialinHuang803 <139532647+JialinHuang803@users.noreply.github.com>
Co-authored-by: JialinHuang803 <139532647+JialinHuang803@users.noreply.github.com>
Co-authored-by: JialinHuang803 <139532647+JialinHuang803@users.noreply.github.com>
|
@copilot Run |
Co-authored-by: JialinHuang803 <139532647+JialinHuang803@users.noreply.github.com>
|
@copilot The pipeline failed when running Seems some files are changed but not committed. Run the smoke test again and commit only the modified *.api.md. |
…isting behavior Co-authored-by: JialinHuang803 <139532647+JialinHuang803@users.noreply.github.com>
... Fixed. Added |
null for nullable optional properties
null for nullable optional properties
Implementation Complete ✅
This PR successfully implements the
ignore-nullable-on-optionaloption for the Modular SDK, aligning with HLC behavior for handling nullable optional properties in Azure services.Summary of Changes
ignore-nullable-on-optionaloption to EmitterOptions with schema definition| nullfrom optional propertiesisOptionalflagignore-nullable-on-optional: trueto openai_generic and openai_modular tspconfig to preserve existing behaviorBehavior
Azure services (default):
property?: Type(without| null)Non-Azure services:
property?: Type | null(preserves| null)All services: Required nullable properties keep
| nullFiles Modified
packages/typespec-ts/src/lib.ts- Added EmitterOptionspackages/rlc-common/src/interfaces.ts- Added RLCOptions fieldpackages/typespec-ts/src/transform/transfromRLCOptions.ts- Added option processingpackages/typespec-ts/src/modular/type-expressions/get-nullable-expression.ts- Core logic (formatted)packages/typespec-ts/src/modular/type-expressions/get-type-expression.ts- Added isOptional flagpackages/typespec-ts/src/modular/emitModels.ts- Pass isOptional to type generationpackages/typespec-ts/test/modularUnit/nullable/nullableOptionalProperties.spec.ts- New comprehensive testspackages/typespec-ts/test/util/emitUtil.ts- Test utilityTesting Coverage
✅ 5 new unit tests for nullable optional behavior
✅ 451 modular unit tests passing
✅ 309 RLC unit tests passing
✅ Code formatting validated with pnpm format
✅ Lint checks passed
✅ Code review completed
✅ OpenAI tests configured to maintain existing behavior
Original prompt
This section details on the original issue you should resolve
<issue_title>[TSP migration] null value will be populated in public API in Modular</issue_title>
<issue_description>
swagger link : https://github.com/Azure/azure-rest-api-specs/blob/main/specification/servicelinker/resource-manager/Microsoft.ServiceLinker/preview/2024-07-01-preview/servicelinker.json#L1489
tsp link: https://github.com/Azure/azure-rest-api-specs/blob/47fc3631222b870ee894ac3e15ac60929f148a2d/specification/servicelinker/ServiceLinker.Management/models.tsp#L1029
Modular:
HLC link: https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/servicelinker/arm-servicelinker/src/models/index.ts#L753
code change :
Comments on the Issue (you are @copilot in this section)
@MaryGao@xirzec and @joheredi Could you help review the following decision for this?
TL;DR
Currently Modular would generate typespec
nullasnullin typescript. I think for Azure services we should follow the HLC behavior - ONLY generatenullfor required property - and NOT generatenullfor optional property. Also we would keep HLC'signore-nullable-on-optionaloption to give flexibility to control this if some services really want to expose this for example openai.This is the whole story
Generally Modular would generate as typespec defines. However I think
nullshould be handled slightly different for Azure services.At the REST/service API layer, nullable and optional are distinct and independent concepts. But. Semantically, for Azure services treat a missing field and a field with a value of null as identical. Also offline confirmed with Johan- autorest never distinguished between the two. Here is the Azure Guideline mentioned this - https://github.com/microsoft/api-guidelines/blob/vNext/azure/Guidelines.md#json-null-resquest-values.
Also in HLC we don't expose the null by default for optional properties but we didn't receive any negative feedbacks around this.
Generating null for optional property would introduce duplication and also lead to worst user experience. Customers would have more if condition to get property due to
Foo | null.We keep the
ignore-nullable-on-optionaloption if any services want to expose them by intention. But the default value would be not exposing for Azure services including DPG and MPG.So I would prefer not generating null with optional properties for Azure services.
@xirzec @MaryGao I agree with your rationale. For JS developers, null is not meaningfully distinct from `undefined` most of the time, with some limited exceptions, such as PATCH semantics where null removes and undefined leaves unmodified.Assuming there isn't a meaningful service behavior difference between null and undefined, I'd rather keep the surface area smaller/simpler.</comment_new>
<comment_new>@MaryGao
We have discussed this in issue: Azure/typespec-azure#3121.</comment_new>
<comment_new>@v-jiaodi
MachineLearningServices case: Azure/azure-rest-api-specs#38507
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.