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

Add validation for keys in PickProperties #5868

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dmnorc
Copy link

@dmnorc dmnorc commented Feb 5, 2025

Keys Validation for PickProperties

Description

This PR introduces key validation for the pickProperties decorator in the TypeSpec compiler. The changes ensure that only valid keys are used when picking properties from a model. If an invalid key is provided, a diagnostic error will be emitted.

Changes

  • Updated the decorators.ts file to include the validation checks.
  • Added corresponding tests in decorators.test.ts to verify the new validation logic.

Related Issues:

#4369

@microsoft-github-policy-service microsoft-github-policy-service bot added the compiler:core Issues for @typespec/compiler label Feb 5, 2025
@dmnorc
Copy link
Author

dmnorc commented Feb 5, 2025

@dmnorc please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

@dmnorc dmnorc marked this pull request as ready for review February 5, 2025 15:45
@dmnorc dmnorc changed the title Added validation for keys in PickProperties Add validation for keys in PickProperties Feb 5, 2025
@dmnorc dmnorc force-pushed the feature/pickProperties-key-validation branch from b65e1d8 to 279d57e Compare February 18, 2025 10:57
@dmnorc dmnorc force-pushed the feature/pickProperties-key-validation branch from 279d57e to 6cd0012 Compare February 18, 2025 11:00
Copy link
Member

@witemple-msft witemple-msft left a comment

Choose a reason for hiding this comment

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

This seems like a good idea.

Could you run pnpm -w change add to create a change record for this (and fill in a small message describing the change)? We use this to create release notes etc.

I have some comments on the implementation and some things that should be accounted for.

// Remove all properties not picked
filterModelPropertiesInPlace(target, (prop) => pickedNames.has(prop.name));
};

function validatePropertyName(context: DecoratorContext, target: Model, name: string) {
const source = target.templateMapper?.args[0] as Model;
if (source && !source.properties?.has(name)) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to account for and test for correct handling of inheritance here. The property might not be a property of the template argument itself, but one of its ancestors.

model X {
  a: string;
}

model Y extends X {}

model Test is PickProperties<Y, "a">

{
code: "unexpected-property",
message:
"Object value may only specify known properties, and 'notMee' does not exist in type 'OriginalModel'.",
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the right error message to give. We use this diagnostic internally when we are checking the assignability of objects, like this:

const a: {} = #{ b: "abc" };

Object value may only specify known properties, and 'a' does not exist in type '{}'.

I think we need to add a better/shorter message for this case. Something like

Property 'notMee' does not exist in type 'OriginalModel'.

propertyName: name,
type: getTypeName(source),
},
target: context.decoratorTarget,
Copy link
Member

Choose a reason for hiding this comment

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

We need to be careful here, because this will put the error on the invocation of the decorator, which is on the template. We would want to put it at least on the second template argument of the template, and best if we can put it specifically on the string that isn't a property.

@dmnorc dmnorc requested a review from witemple-msft February 21, 2025 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:core Issues for @typespec/compiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants