Skip to content

Conversation

@archerzz
Copy link
Member

  • change json coverter for InputModelProperty to set the DefaultValue property if the property type is a literal type
    • update ObjectTypeProperty so that DefaultValue is set if it's from literal type
  • change ModelTypeProviderFields:
    • exclude literal properties from constructor parameter list
    • literal properties are internal readonly
  • change ModelTypeProvider, update the criteria on properties to be serialized, now literal type property will be serialized regardless it's read-only
  • fix a issue in emitter that boolean literal type is not identified
  • minor refactoring
  • update test projects

resolve #3035

Description

Add your description here!

Checklist

To ensure a quick review and merge, please ensure:

  • The PR has a understandable title and description explaining the why and what.
  • The PR is opened in draft if not ready for review yet.
    • If opened in draft, please allocate sufficient time (24 hours) after moving out of draft for review
  • The branch is recent enough to not have merge conflicts upon creation.

Ready to Land?

  • Build is completely green
    • Submissions with test failures require tracking issue and approval of a CODEOWNER
  • At least one +1 review by a CODEOWNER
  • All -1 reviews are confirmed resolved by the reviewer
    • Override/Marking reviews stale must be discussed with CODEOWNERS first

- change json coverter for `InputModelProperty` to set the `DefaultValue` property if the property type is a literal type
  - update `ObjectTypeProperty` so that `DefaultValue` is set if it's from literal type
- change `ModelTypeProviderFields`:
  - exclude literal properties from constructor parameter list
  - literal properties are `internal readonly`
- change `ModelTypeProvider`, update the criteria on properties to be serialized, now literal type property will be serialized regardless it's read-only
- fix a issue in emitter that boolean literal type is not identified
- minor refactoring
- update test projects

resolve Azure#3035
Copy link
Member

@ArcturusZhang ArcturusZhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a InputConstant in our input types, which is really similar to the literal type.

Do we need to use two things to represent a very similar case?

Currently the InputConstant class is only used in InputParamters, but I believe this literal type has very few differences from the LiteralType.

This is what InputConstant and InputLiteralType looks like:

internal record InputConstant(object? Value, InputType Type);
internal record InputLiteralType(string Name, InputType LiteralValueType, object Value, bool IsNullable = false)

The Name in literal is constant Literal, and IsNullable makes no sense on literal types (you can only assign one value for it, how could it be null? if it could be null, it only longer only accepts one value).

Therefore I suggest we combine the InputConstant and InputLiteralType and things would align with how we deal with the same thing about ConstantSchema back using swaggers

- add simple value type check for `number` literal value in emitter, we only support int32 and float64
- change converter to support float64 which will be mapped to double in csharp
- drop converter support for int64 since it's not emitted in the emitter
- update test cases
- minor refactoring
- modify cadl definition to add test case for literal model property
- add a customization constructor of `CadlFirstTestClient` for test purpose
- add mock api
- add a property `ShouldSkipDeserialization` in `PropertySerliaization`
- update writer to skip generating deserialization codes for literal type properties
@archerzz
Copy link
Member Author

archerzz commented Jan 31, 2023

A couple of changes since last review:

  • Modify emitter to add basic support of differentiating integer from float. We'll not try to identify the full list of CADL number types. Instead, we just support int32 for integer value and double for float value, those two types are the most commonly used.
  • Remove literal type properties from generated deserialization codes
  • Add E2E test case

archerzz and others added 5 commits February 1, 2023 16:05
# Conflicts:
#	test/TestProjects/FirstTest-Cadl/Generated/Docs/CadlFirstTestClient.xml
#	test/TestProjects/FirstTest-Cadl/Generated/Models/Thing.cs
#	test/TestProjects/FirstTest-Cadl/Generated/cadl.json
# Conflicts:
#	test/TestProjects/FirstTest-Cadl/FirstTest-Cadl.cadl
#	test/TestProjects/FirstTest-Cadl/Generated/Docs/CadlFirstTestClient.xml
#	test/TestProjects/FirstTest-Cadl/Generated/Models/Thing.Serialization.cs
#	test/TestProjects/FirstTest-Cadl/Generated/Models/Thing.cs
#	test/TestProjects/FirstTest-Cadl/Generated/cadl.json
@archerzz
Copy link
Member Author

archerzz commented Feb 2, 2023

@m-nash I've committed change to treat literal type operation parameters as constant parameters (see the last 2nd commit). However, I think it's still better to handle that in the emitter, because:

  • By the meaning, it's a constant. It's more like a compiler optimization, v.s. type/name mapping which should be handled in generator.
  • Previously it's handled outside generator. When autorest passes in the parameters, their types are already decided.
  • We don't have a neat encapsulation on this issue. I have to change a few places in the codes, instead of one line code change in the emitter.
  • Another option is to write a Json converter to transform the Kind property. But that's almost equal to change in the emitter.

@archerzz archerzz merged commit c8280c0 into Azure:feature/v3 Feb 9, 2023
@archerzz archerzz deleted the cadl/literal-model-property branch February 9, 2023 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feat]: Support Literal Type Model Property

4 participants