-
Notifications
You must be signed in to change notification settings - Fork 374
fix: FilterVisibility now applies visibility filter to inherited types (extends) #11043
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?
Changes from all commits
5b2001c
dd75523
3e53a63
a7de7ad
b682d25
1e6c9a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| changeKind: fix | ||
| packages: | ||
| - "@typespec/compiler" | ||
| --- | ||
|
|
||
| `FilterVisibility` now applies the visibility filter to inherited types (via `extends`), filtering out invisible properties from base models in the inheritance chain. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1596,6 +1596,208 @@ | |
| ok(!arrA.properties.has("invisible")); | ||
| }); | ||
|
|
||
| it("applies visibility filter to base model (extends)", async () => { | ||
| const { UserCreate, program } = await Tester.compile(t.code` | ||
| model Base { | ||
| @visibility(Lifecycle.Read) id: string; | ||
| } | ||
|
|
||
| model Auditable extends Base { | ||
| @visibility(Lifecycle.Read) created_at: utcDateTime; | ||
| @visibility(Lifecycle.Read) updated_at: utcDateTime; | ||
| } | ||
|
|
||
| model User extends Auditable { | ||
| name: string; | ||
| } | ||
|
|
||
| @test model ${t.model("UserCreate")} is FilterVisibility<User, #{ all: #[Lifecycle.Create] }, "{name}Create">; | ||
| `); | ||
|
|
||
| ok(UserCreate); | ||
|
|
||
| // User's own property 'name' has default visibility (all), so it should be visible | ||
| ok(UserCreate.properties.has("name")); | ||
|
|
||
| // The base model should be transformed (Auditable -> AuditableCreate) | ||
| const auditableCreate = UserCreate.baseModel; | ||
| ok(auditableCreate); | ||
| strictEqual(auditableCreate.name, "AuditableCreate"); | ||
|
|
||
| // Auditable's read-only properties should be filtered out | ||
| ok(!auditableCreate.properties.has("created_at")); | ||
| ok(!auditableCreate.properties.has("updated_at")); | ||
|
|
||
| // The base of Auditable (Base) should also be transformed | ||
| const baseCreate = auditableCreate.baseModel; | ||
| ok(baseCreate); | ||
| strictEqual(baseCreate.name, "BaseCreate"); | ||
|
|
||
| // Base's read-only property should be filtered out | ||
| ok(!baseCreate.properties.has("id")); | ||
| }); | ||
|
|
||
| it("does not transform base model when all properties are visible", async () => { | ||
| const { ChildFiltered, program } = await Tester.compile(t.code` | ||
| model Parent { | ||
| name: string; | ||
| } | ||
|
|
||
| model Child extends Parent { | ||
| @visibility(Lifecycle.Read) id: string; | ||
| description: string; | ||
| } | ||
|
|
||
| @test model ${t.model("ChildFiltered")} is FilterVisibility<Child, #{ all: #[Lifecycle.Create] }, "{name}Create">; | ||
| `); | ||
|
|
||
| ok(ChildFiltered); | ||
|
|
||
| // Child's 'description' is visible (default visibility), 'id' is not | ||
| ok(ChildFiltered.properties.has("description")); | ||
| ok(!ChildFiltered.properties.has("id")); | ||
|
|
||
| // Parent has no properties with restricted visibility, so it should remain unchanged | ||
| const base = ChildFiltered.baseModel; | ||
| ok(base); | ||
| strictEqual(base.name, "Parent"); | ||
| ok(base.properties.has("name")); | ||
| }); | ||
|
|
||
| it("applies visibility filter across multiple layers where base is unchanged", async () => { | ||
| const { Result, program } = await Tester.compile(t.code` | ||
| model GrandParent { | ||
| gp_all: string; | ||
| } | ||
|
|
||
| model Parent extends GrandParent { | ||
| parent_all: string; | ||
| } | ||
|
|
||
| model Child extends Parent { | ||
| @visibility(Lifecycle.Read) child_read: string; | ||
| child_all: string; | ||
| } | ||
|
|
||
| @test model ${t.model("Result")} is FilterVisibility<Child, #{ all: #[Lifecycle.Create] }, "{name}Create">; | ||
| `); | ||
|
|
||
| ok(Result); | ||
|
|
||
| // Child's properties | ||
| ok(Result.properties.has("child_all")); | ||
| ok(!Result.properties.has("child_read")); | ||
|
|
||
| // Neither Parent nor GrandParent have visibility-restricted properties, | ||
| // so the entire base chain should remain unchanged | ||
| const parentBase = Result.baseModel; | ||
| ok(parentBase); | ||
| strictEqual(parentBase.name, "Parent"); | ||
| ok(parentBase.properties.has("parent_all")); | ||
|
|
||
| const gpBase = parentBase.baseModel; | ||
| ok(gpBase); | ||
| strictEqual(gpBase.name, "GrandParent"); | ||
| ok(gpBase.properties.has("gp_all")); | ||
| }); | ||
|
|
||
| it("applies visibility filter across multiple inheritance layers", async () => { | ||
| const { Result, program } = await Tester.compile(t.code` | ||
| model GrandParent { | ||
| @visibility(Lifecycle.Read) gp_read: string; | ||
| gp_all: string; | ||
| } | ||
|
|
||
| model Parent extends GrandParent { | ||
| parent_all: string; | ||
| } | ||
|
|
||
| model Child extends Parent { | ||
| @visibility(Lifecycle.Read) child_read: string; | ||
| child_all: string; | ||
| } | ||
|
|
||
| @test model ${t.model("Result")} is FilterVisibility<Child, #{ all: #[Lifecycle.Create] }, "{name}Create">; | ||
| `); | ||
|
|
||
| ok(Result); | ||
|
|
||
| // Child's properties | ||
| ok(Result.properties.has("child_all")); | ||
| ok(!Result.properties.has("child_read")); | ||
|
|
||
| // Parent is transformed because its base (GrandParent) is transformed | ||
| const parentFiltered = Result.baseModel; | ||
| ok(parentFiltered); | ||
| strictEqual(parentFiltered.name, "ParentCreate"); | ||
| ok(parentFiltered.properties.has("parent_all")); | ||
|
|
||
| // GrandParent needs transformation (has read-only property) | ||
| const gpFiltered = parentFiltered.baseModel; | ||
| ok(gpFiltered); | ||
| strictEqual(gpFiltered.name, "GrandParentCreate"); | ||
| ok(gpFiltered.properties.has("gp_all")); | ||
| ok(!gpFiltered.properties.has("gp_read")); | ||
| }); | ||
|
|
||
| it("applies visibility filter to base model via applyVisibilityFilter", async () => { | ||
| const { User, program } = await Tester.compile(t.code` | ||
| model Base { | ||
| @visibility(Lifecycle.Read) id: string; | ||
| } | ||
|
|
||
| model ${t.model("User")} extends Base { | ||
| name: string; | ||
| } | ||
| `); | ||
|
|
||
| const fnContext = { program } satisfies Pick<FunctionContext, "program">; | ||
| const lifecycle = getLifecycleVisibilityEnum(program); | ||
| const filtered = applyVisibilityFilter( | ||
| fnContext, | ||
| User, | ||
| anyFilter(lifecycle.members.get("Create")!), | ||
| "{name}Create", | ||
| ); | ||
|
|
||
| ok(filtered); | ||
| strictEqual(filtered.name, "UserCreate"); | ||
| ok(filtered.properties.has("name")); | ||
|
|
||
| // Base model should be transformed | ||
| const baseFiltered = filtered.baseModel; | ||
| ok(baseFiltered); | ||
| strictEqual(baseFiltered.name, "BaseCreate"); | ||
| ok(!baseFiltered.properties.has("id")); | ||
| }); | ||
|
|
||
| it("does not produce infinite regress for self-referential models", async () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not quite what I meant. Here I mean a test in which the target test model extends a model that has a self-referential property.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the test so |
||
| const { Result } = await Tester.compile(t.code` | ||
| model LinkedNode { | ||
| value: string; | ||
| @visibility(Lifecycle.Read) id: string; | ||
| next?: LinkedNode; | ||
| } | ||
|
|
||
| model Child extends LinkedNode { | ||
| extra: string; | ||
| } | ||
|
|
||
| @test model ${t.model("Result")} is FilterVisibility<Child, #{ all: #[Lifecycle.Create] }, "{name}Create">; | ||
| `); | ||
|
|
||
| ok(Result); | ||
| ok(Result.properties.has("extra")); | ||
|
|
||
| // Base model (LinkedNode) should be filtered and renamed | ||
| const base = Result.baseModel; | ||
| ok(base); | ||
| strictEqual(base.name, "LinkedNodeCreate"); | ||
| ok(base.properties.has("value")); | ||
| ok(base.properties.has("next")); | ||
| ok(!base.properties.has("id")); | ||
| }); | ||
|
|
||
| it("does not duplicate encodedName metadata", async () => { | ||
| const diagnostics = await Tester.diagnose(` | ||
| model SomeModel { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that there is a test over a self referential model, for example, if some type ihas s efl link, it should not produce infinite regress.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test for a self-referential model (
LinkedNodewithnext?: LinkedNodeproperty) that verifies no infinite regress occurs. ThecachedMutateSubgraphcache and the internalseenmap inmutateSubgraphWorkerprevent infinite recursion. Commit: a7de7ad.