Skip to content
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

model extends/is record<> #330

Merged
merged 20 commits into from
Oct 24, 2023
Merged

model extends/is record<> #330

merged 20 commits into from
Oct 24, 2023

Conversation

msyyc
Copy link
Member

@msyyc msyyc commented May 19, 2023

Cadl Ranch Contribution Checklist:

  • I have written a scenario spec
  • I have meaningful @scenario names. Someone can look at the list of scenarios and understand what I'm covering.
  • I have written a mock API
  • I have used @scenarioDocs for extra scenario description and to tell people how to pass my mock api check.

@changeset-bot
Copy link

changeset-bot bot commented May 19, 2023

🦋 Changeset detected

Latest commit: 47e84d8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@azure-tools/cadl-ranch-specs Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@lmazuel lmazuel left a comment

Choose a reason for hiding this comment

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

  • I would not put this in inheritance. TSP uses "extends", but it's the concept of "additional properties". Inheritance is misleading here.
  • I need a GET as well

@timotheeguerin
Copy link
Member

timotheeguerin commented May 19, 2023

note also extends vs is is different, you probably still want is. Extends will say i am inheriting a Record, is will just say I am a Record<string>

See the differently generated openapi
https://cadlplayground.z22.web.core.windows.net/?c=bW9kZWwgTcQGUmVjb3JkVW5rbm93biBleHRlbmRzIMYWPHXGFz4gewogIG5hbWU6IHN0cmluZzsKfQoK2EYyIGnfQsZC

I don't know how that affect generated client however but I could imagine something like that


class WithExtends extends Dictionary<string, string> {

  public Name {get; set;}
}


class WithIs implements IDictionary<string, string> {
  ... need to imploement dictionary interface
  public Name {get; set;}
}

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented May 22, 2023

On swagger output of

model ModelRecordUnknown2 is Record<unknown> {
  name: string;
}
    ModelRecordUnknown2:
      type: object
      additionalProperties: {}

It does not have anything about "name"?


PS, Java uses composition (it is almost always a bad practice to inherit Map in Java).
E.g.

class WithWhatever() {
  string name;
  Map<String, Object> additionalProperties; // this does not serialize to JSON, but as a composition for properties except "name"
}

@msyyc
Copy link
Member Author

msyyc commented May 22, 2023

note also extends vs is is different, you probably still want is. Extends will say i am inheriting a Record, is will just say I am a Record<string>

See the differently generated openapi https://cadlplayground.z22.web.core.windows.net/?c=bW9kZWwgTcQGUmVjb3JkVW5rbm93biBleHRlbmRzIMYWPHXGFz4gewogIG5hbWU6IHN0cmluZzsKfQoK2EYyIGnfQsZC

I don't know how that affect generated client however but I could imagine something like that


class WithExtends extends Dictionary<string, string> {

  public Name {get; set;}
}


class WithIs implements IDictionary<string, string> {
  ... need to imploement dictionary interface
  public Name {get; set;}
}

@timotheeguerin As weidong said, we shall encourage users extends instead of is otherwise users can't get expected output.

@msyyc
Copy link
Member Author

msyyc commented May 22, 2023

  • I would not put this in inheritance. TSP uses "extends", but it's the concept of "additional properties". Inheritance is misleading here.
  • I need a GET as well

I add new folder record under type/model and also add GET case

@tadelesh
Copy link
Member

tadelesh commented May 22, 2023

@msyyc I prefer to add it under to type/property named with additional-properties. @timotheeguerin I have a question for Record<unknown>. From TSP type relations, it seems custom model with properties are derived from Record. It that mean all custom models are already support additional properties? What's the difference for an explicit Model extends Record<unknown>?

@timotheeguerin
Copy link
Member

Hhm yeah seems like we have a bug in autorest and openapi3 emitter, those properties should not be dropped. I think the idea was there though that with is you have additionalProperties on the schema itself. Vs with extends it use an allOf.

I think the takeaway is mostly both can happen and I think we should recommend people to use the is variant. If you generate the same thing then maybe its ok

@timotheeguerin
Copy link
Member

timotheeguerin commented May 22, 2023

@msyyc I prefer to add it under to type/property named with additional-properties. @timotheeguerin I have a question for Record. From TSP type relations, it seems custom model with properties are derived from Record. It that mean all custom models are already support additional properties? What's the difference for an explicit Model extends Record?

there is a discussion on Teams about that

short answer, no models do not have additional properties in the TypeSpec system(I made a wrong statement before), we might need to update the type graph a little bit

model Foo { } !== model Foo is Record<unknown> {}

@msyyc
Copy link
Member Author

msyyc commented May 23, 2023

@msyyc Yuchao Yan FTE I prefer to add it under to type/property named with additional-properties. @timotheeguerin Timothee Guerin FTE I have a question for Record<unknown>. From TSP type relations, it seems custom model with properties are derived from Record. It that mean all custom models are already support additional properties? What's the difference for an explicit Model extends Record<unknown>?

@tadelesh Make sense. I will move it to type/property/additional-properties

@msyyc msyyc changed the title model extends record model extends/is record<> May 23, 2023
@msyyc
Copy link
Member Author

msyyc commented May 23, 2023

@timotheeguerin @lmazuel @tadelesh @weidongxu-microsoft
Updated:
I understand that language emitter shall have same behavior for extends Record<> or is Record<>, I add test for both scenario.

Copy link
Member

@tadelesh tadelesh left a comment

Choose a reason for hiding this comment

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

some minor comment

Copy link
Member

@tadelesh tadelesh left a comment

Choose a reason for hiding this comment

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

some minor comment

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft left a comment

Choose a reason for hiding this comment

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

Make sure you've tested the mockapi in python.

Copy link
Member

@qiaozha qiaozha left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@tadelesh tadelesh left a comment

Choose a reason for hiding this comment

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

Though seems a little bit weird to give both is and extends example, but should all be valid cases for non-branding.

Copy link
Member

@lmazuel lmazuel left a comment

Choose a reason for hiding this comment

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

Was approved by enough smart people to remove my "require change" :)

@msyyc msyyc merged commit 9081f95 into main Oct 24, 2023
7 checks passed
@msyyc msyyc deleted the extends-record branch October 24, 2023 16:59
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.

None yet

7 participants