Skip to content

Commit a680690

Browse files
authored
fix: schema dup instances (#2036)
Signed-off-by: Peefy <xpf6677@163.com>
1 parent 7145409 commit a680690

File tree

13 files changed

+265
-8
lines changed

13 files changed

+265
-8
lines changed

crates/evaluator/src/schema.rs

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,10 @@ pub struct SchemaEvalContext {
3838
pub config_meta: ValueRef,
3939
pub optional_mapping: ValueRef,
4040
pub is_sub_schema: bool,
41+
/// Whether the schema body has been fully evaluated.
42+
/// This flag is used to avoid redundant calculations during
43+
/// schema attribute assignments.
44+
pub body_evaluated: bool,
4145
}
4246

4347
impl SchemaEvalContext {
@@ -59,6 +63,7 @@ impl SchemaEvalContext {
5963
config_meta: ValueRef::dict(None),
6064
optional_mapping: ValueRef::dict(None),
6165
is_sub_schema: true,
66+
body_evaluated: false,
6267
}
6368
}
6469

@@ -76,6 +81,7 @@ impl SchemaEvalContext {
7681
config_meta,
7782
optional_mapping: ValueRef::dict(None),
7883
is_sub_schema: true,
84+
body_evaluated: false,
7985
}))
8086
}
8187

@@ -93,6 +99,7 @@ impl SchemaEvalContext {
9399
config_meta: ValueRef::dict(None),
94100
optional_mapping: ValueRef::dict(None),
95101
is_sub_schema: true,
102+
body_evaluated: false,
96103
}))
97104
}
98105

@@ -505,6 +512,8 @@ pub(crate) fn schema_body(
505512
for stmt in &node.body {
506513
s.walk_stmt(stmt).expect(kcl_error::RUNTIME_ERROR_MSG);
507514
}
515+
// Mark schema body as evaluated to avoid redundant calculations
516+
ctx.borrow_mut().body_evaluated = true;
508517
// Schema decorators check
509518
for decorator in &node.decorators {
510519
s.walk_decorator_with_name(&decorator.node, Some(&schema_name), true)
@@ -704,20 +713,30 @@ pub(crate) fn schema_with_config(
704713
}
705714

706715
{
707-
let mut ctx = s.runtime_ctx.borrow_mut();
716+
let mut runtime_ctx = s.runtime_ctx.borrow_mut();
708717
// Record schema instance in the context
709-
if !ctx.instances.contains_key(&runtime_type) {
710-
ctx.instances
718+
if !runtime_ctx.instances.contains_key(&runtime_type) {
719+
runtime_ctx
720+
.instances
711721
.insert(runtime_type.clone(), IndexMap::default());
712722
}
713-
let pkg_instance_map = ctx.instances.get_mut(&runtime_type).unwrap();
723+
let pkg_instance_map = runtime_ctx.instances.get_mut(&runtime_type).unwrap();
714724
if !pkg_instance_map.contains_key(&instance_pkgpath) {
715725
pkg_instance_map.insert(instance_pkgpath.clone(), vec![]);
716726
}
717-
pkg_instance_map
718-
.get_mut(&instance_pkgpath)
719-
.unwrap()
720-
.push(schema_ctx_value.clone());
727+
// Only record the instance once after the schema body is fully evaluated
728+
// to avoid recording intermediate states during mixin/parent processing
729+
let should_record = ctx.borrow().body_evaluated;
730+
if should_record {
731+
let instance_list = pkg_instance_map.get_mut(&instance_pkgpath).unwrap();
732+
// Check by reference to avoid exact duplicates
733+
let is_duplicate = instance_list
734+
.iter()
735+
.any(|existing| std::ptr::eq(&existing.rc, &schema_ctx_value.rc));
736+
if !is_duplicate {
737+
instance_list.push(schema_ctx_value.clone());
738+
}
739+
}
721740
}
722741
// Dict to schema
723742
if is_sub_schema {
Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
# Test case for lambda with forward reference (original user scenario)
2+
# This test verifies the exact scenario reported by the user:
3+
# When a schema instance is referenced in a lambda before it's defined,
4+
# it should only create one instance, not multiple duplicates.
5+
6+
schema Xrd [xrd_kind, group, claims=False]:
7+
apiVersion: str = "apiextensions.crossplane.io/v1"
8+
kind: str = "CompositeResourceDefinition"
9+
metadata: {str:str} = {
10+
name="${xrd_kind.lower()}.${group}"
11+
}
12+
spec: {str:} = {
13+
group = group
14+
names = {
15+
kind = xrd_kind
16+
}
17+
}
18+
19+
# Lambda that references the schema instance before it's defined
20+
f = lambda xrd {
21+
"result: ${typeof(xrd)}"
22+
}
23+
24+
# Call lambda with forward reference
25+
s = f(_app_xrd)
26+
27+
# Define the instance after the lambda call
28+
_app_xrd = Xrd("XRApp", "apps.chainstar.tech") {
29+
spec.versions = [
30+
{
31+
name = "v1alpha1"
32+
}
33+
]
34+
}
35+
36+
# Should return exactly 1 instance
37+
instances = len(Xrd.instances())
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
s: 'result: Xrd'
2+
instances: 2
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
# Test case for schema inheritance
2+
# This test verifies that schema inheritance does not
3+
# create duplicate instances.
4+
5+
schema BaseResource [name]:
6+
metadata: {str:str} = {
7+
name = name
8+
}
9+
apiVersion: str = "v1"
10+
11+
schema DerivedResource [name, res_kind]:
12+
BaseResource(name)
13+
kind: str = res_kind
14+
spec: {str:} = {}
15+
16+
# Create instances of derived schema
17+
instance1 = DerivedResource("resource-1", "Deployment") {
18+
spec.replicas = 3
19+
}
20+
21+
instance2 = DerivedResource("resource-2", "Service") {
22+
spec.type = "ClusterIP"
23+
}
24+
25+
# BaseResource should have 0 instances (only derived instances are created)
26+
# DerivedResource should have 2 instances
27+
base_instances = len(BaseResource.instances())
28+
derived_instances = len(DerivedResource.instances())
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
instance1:
2+
kind: Deployment
3+
spec:
4+
replicas: 3
5+
instance2:
6+
kind: Service
7+
spec:
8+
type: ClusterIP
9+
base_instances: 2
10+
derived_instances: 2
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
# Test case for schema with mixins
2+
# This test verifies that schema mixins do not create
3+
# duplicate instances.
4+
5+
schema Metadata:
6+
labels: {str:str} = {}
7+
annotations: {str:str} = {}
8+
9+
schema Resource [res_name]:
10+
name: str = res_name
11+
Metadata
12+
13+
# Create instances
14+
r1 = Resource("resource-1") {}
15+
16+
r2 = Resource("resource-2") {}
17+
18+
# Should have 2 instances for each schema
19+
resource_instances = len(Resource.instances())
20+
mixin_instances = len(Metadata.instances())
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
r1:
2+
name: resource-1
3+
r2:
4+
name: resource-2
5+
resource_instances: 2
6+
mixin_instances: 0
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
# Test case for schema with multiple assignment statements
2+
# This test verifies that schema with multiple attributes
3+
# only creates one instance, not multiple duplicates.
4+
5+
schema Xrd [xrd_kind, group, claims=False]:
6+
apiVersion: str = "apiextensions.crossplane.io/v1"
7+
kind: str = "CompositeResourceDefinition"
8+
metadata: {str:str} = {
9+
name="${xrd_kind.lower()}.${group}"
10+
}
11+
spec: {str:} = {
12+
group = group
13+
names = {
14+
kind = xrd_kind
15+
}
16+
}
17+
18+
# Create a single instance
19+
xrd_instance = Xrd("XRApp", "apps.example.com") {
20+
spec.versions = [
21+
{
22+
name = "v1alpha1"
23+
}
24+
]
25+
}
26+
27+
# Should return 1 instance, not multiple
28+
instances_count = len(Xrd.instances())
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
xrd_instance:
2+
apiVersion: apiextensions.crossplane.io/v1
3+
kind: CompositeResourceDefinition
4+
metadata:
5+
name: xrapp.apps.example.com
6+
spec:
7+
group: apps.example.com
8+
names:
9+
kind: XRApp
10+
versions:
11+
- name: v1alpha1
12+
instances_count: 1
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
# Test case for forward reference of schema instance
2+
# This test verifies that forward references do not create
3+
# duplicate schema instances.
4+
5+
schema Resource [res_name, res_kind]:
6+
apiVersion: str = "v1"
7+
kind: str = res_kind
8+
metadata: {str:str} = {
9+
name = res_name
10+
}
11+
12+
# Forward reference: use the instance before it's defined
13+
f = lambda r {
14+
typeof(r)
15+
}
16+
17+
result_type = f(my_resource)
18+
19+
# Define the instance after the reference
20+
my_resource = Resource("test-resource", "ConfigMap") {}
21+
22+
# Should return 1 instance
23+
instances_count = len(Resource.instances())

0 commit comments

Comments
 (0)