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

bug: Unable to set default on scalar definition #1646

Closed
tmeckel opened this issue Feb 14, 2023 · 8 comments
Closed

bug: Unable to set default on scalar definition #1646

tmeckel opened this issue Feb 14, 2023 · 8 comments
Assignees
Labels
design:needed A design request has been raised that needs a proposal
Milestone

Comments

@tmeckel
Copy link

tmeckel commented Feb 14, 2023

The following screenshot shows a valid OpenAPI YAML definition for a component which derives from a string and has a default value assigned. The default value is inherited when, the component definition is (re-)used as reference inside another component (model) definition:

8lvo8SMyhj

With CADL 0.4.0 the following code creates a compile time error:

import "@cadl-lang/rest";

using Cadl.Http;
@service({
  title: "CADL Test Service",
  version: "1.0.0",
})
namespace CADLTestService;

@format("uuid")
scalar Id extends string = "b285d632-5603-471f-90a9-b836fa3fce7b";

model Item {
  id: Id;
  value: bytes
}
Cadl compiler v0.40.0

Diagnostics were reported during compilation:

/home/user/cadl-test/main.cadl:11:26 - error token-expected: Statement expected.
> 11 | scalar Id extends string = "b285d632-5603-471f-90a9-b836fa3fce7b";
     |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Found 1 error.

It is possible though to define/assign a default value to the id field of model Item when the field id is marked optional. But this is definitely not the same behavior as with the initially mentioned OpenAPI definition.

import "@cadl-lang/rest";

using Cadl.Http;
@service({
  title: "CADL Test Service",
  version: "1.0.0",
})
namespace CADLTestService;

@format("uuid")
scalar Id extends string;

model Item {
  id?: Id = "b285d632-5603-471f-90a9-b836fa3fce7b";
  value: bytes
}
@markcowl markcowl added the design:needed A design request has been raised that needs a proposal label Feb 21, 2023
@markcowl markcowl added this to the Backlog milestone Feb 21, 2023
@bterlson
Copy link
Member

bterlson commented Sep 1, 2023

@markcowl what is the design discussion needed here? Can we schedule this?

@tmeckel
Copy link
Author

tmeckel commented Sep 27, 2023

@bterlson @markcowl any progress here? The issue lingers around for a while now? 😕

@bterlson
Copy link
Member

There is a related issue now (#2359, for general declarations not just scalars). Sorry for the delay and I agree this issue has been around for a quite a while and is something we should support, I'll try to get something on the design agenda for next week (and will scope the issue to just scalars in case model defaults is a bridge too far for folks). /cc @markcowl

@tmeckel
Copy link
Author

tmeckel commented Sep 27, 2023

@bterlson great news! This issue is the only thing preventing me from converting all OpenAPI definitions to TypeSpec at a customer engagement. Can't wait 😊👌🏼

@bterlson
Copy link
Member

@tmeckel we managed to discussed this briefly today, but did not come to any conclusion for scalar defaults yet. However, it was noted that the code above, which previously threw an error, does not throw! In particular, this should work fine today:

model Item {
  // id is required, this is allowed
  id: Id = "b285d632-5603-471f-90a9-b836fa3fce7b";
  value: bytes
}

Perhaps this is a viable workaround. It does mean any usages of Id will have to specify their own defaults, though. To that end, another possible workaround is to encapsulate the entire property in a model:

model ItemId {
  id: Id = "b285d632-5603-471f-90a9-b836fa3fce7b";
}

model Item {
  ... ItemId;
  value: bytes;
}

Lastly, it would be helpful to understand the underlying motivation behind the need for scalar defaults - is it about not repeating the default everywhere it is used, matching an existing OpenAPI document, etc.

@tmeckel
Copy link
Author

tmeckel commented Oct 5, 2023

@bterlson Many thanks for the detailed answer! I'll give both workarounds a shot. See which might work best for me.

About the reasons you asked:

  • Firstly it's very convenient to have a default on a scalar because regardless where you use (reference) it in a component type, the property of the component type will inherit the default value. This is less error prone than repeating the default value over and over again. The second workaround (via a model) and use the spread operator to expand the model into another model gets as closest as it could I think. But it feels kinda awkward. I'm a purist in that way 😁

  • Secondly the OpenAPI specs that I try to migrate to TypeSpec mostly apply this pattern i.e. defining a default value on scalars. By the way, this was the reason why I opened this issue in the first place.

@markcowl markcowl modified the milestones: Backlog, [2023] December Nov 6, 2023
@markcowl
Copy link
Contributor

markcowl commented Nov 7, 2023

@markcowl
Copy link
Contributor

closing in favor of #2359

@markcowl markcowl closed this as not planned Won't fix, can't repro, duplicate, stale Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design:needed A design request has been raised that needs a proposal
Projects
None yet
Development

No branches or pull requests

3 participants