Skip to content

Commit ad6f1e1

Browse files
negznegz
authored
Merge pull request #208 from negz/best-left-unset
Serialize models with exclude_unset, not exclude_defaults
2 parents 11b617c + d18cbbb commit ad6f1e1

10 files changed

Lines changed: 451 additions & 14 deletions

File tree

‎crossplane/function/resource.py‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,21 @@ def update(r: fnv1.Resource, source: dict | structpb.Struct | pydantic.BaseModel
4040
"""
4141
match source:
4242
case pydantic.BaseModel():
43-
data = source.model_dump(exclude_defaults=True, warnings=False)
44-
# In Pydantic, exclude_defaults=True in model_dump excludes fields
45-
# that have their value equal to the default. If a field like
46-
# apiVersion is set to its default value 's3.aws.upbound.io/v1beta2'
47-
# (and not explicitly provided during initialization), it will be
48-
# excluded from the serialized output.
43+
# exclude_unset emits only the fields the caller explicitly set.
44+
# Crossplane treats desired resources as server-side apply intent,
45+
# so a function should own exactly the fields it has an opinion
46+
# about and leave the rest to the API server.
47+
#
48+
# by_alias emits each field under its alias, which is its real
49+
# wire name. datamodel-code-generator aliases fields whose KRM
50+
# name collides with a Python keyword or builtin (e.g. it emits a
51+
# bool_ attribute aliased to bool, continue_ aliased to continue).
52+
# Without by_alias those fields serialize under the Python name and
53+
# don't match the resource's schema. It's a no-op for ordinary
54+
# fields, which have no alias.
55+
data = source.model_dump(exclude_unset=True, by_alias=True, warnings=False)
56+
# apiVersion and kind identify the resource but are rarely passed
57+
# as kwargs, so they're usually unset. Add them back explicitly.
4958
data["apiVersion"] = source.apiVersion
5059
data["kind"] = source.kind
5160
r.resource.update(data)
@@ -71,11 +80,12 @@ def update_status(
7180
status: The status to set, as a dictionary or Pydantic model.
7281
7382
Sets ``r.resource.status`` from the supplied status. When the status
74-
is a Pydantic model, fields set to their default value are excluded,
83+
is a Pydantic model, fields the caller didn't explicitly set are
84+
excluded and aliased fields are emitted under their wire names,
7585
matching the behavior of :func:`update`.
7686
"""
7787
if isinstance(status, pydantic.BaseModel):
78-
status = status.model_dump(exclude_defaults=True, warnings=False)
88+
status = status.model_dump(exclude_unset=True, by_alias=True, warnings=False)
7989
update(r, {"status": status})
8090

8191

‎tests/test_resource.py‎

Lines changed: 197 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,11 @@
2222

2323
import crossplane.function.proto.v1.run_function_pb2 as fnv1
2424
from crossplane.function import logging, resource
25-
from tests.testdata.models.io.upbound.aws.s3 import v1beta2
25+
from tests.testdata.models.io.k8s.api.resource import v1 as resourcev1
26+
from tests.testdata.models.io.upbound.aws.s3 import v1beta2 as s3v1beta2
27+
from tests.testdata.models.io.upbound.m.aws.iam.accountalias import (
28+
v1beta1 as accountaliasv1beta1,
29+
)
2630

2731

2832
class TestResource(unittest.TestCase):
@@ -59,13 +63,43 @@ class TestCase:
5963
{"apiVersion": "example.org", "kind": "XR"}
6064
),
6165
),
62-
status=v1beta2.ForProvider(region="us-west-2"),
66+
status=s3v1beta2.ForProvider(region="us-west-2"),
6367
want={
6468
"apiVersion": "example.org",
6569
"kind": "XR",
6670
"status": {"region": "us-west-2"},
6771
},
6872
),
73+
TestCase(
74+
reason="Fields the caller set should be kept, while unset "
75+
"fields are omitted.",
76+
r=fnv1.Resource(
77+
resource=resource.dict_to_struct(
78+
{"apiVersion": "example.org", "kind": "XR"}
79+
),
80+
),
81+
status=s3v1beta2.ForProvider(region="us-west-2", forceDestroy=False),
82+
want={
83+
"apiVersion": "example.org",
84+
"kind": "XR",
85+
"status": {"region": "us-west-2", "forceDestroy": False},
86+
},
87+
),
88+
TestCase(
89+
reason="Setting status from a Pydantic model with keyword-"
90+
"aliased fields should emit the fields under their aliases.",
91+
r=fnv1.Resource(
92+
resource=resource.dict_to_struct(
93+
{"apiVersion": "example.org", "kind": "XR"}
94+
),
95+
),
96+
status=resourcev1.DeviceAttribute(**{"bool": True}),
97+
want={
98+
"apiVersion": "example.org",
99+
"kind": "XR",
100+
"status": {"bool": True},
101+
},
102+
),
69103
TestCase(
70104
reason="Setting status on an empty resource should work.",
71105
r=fnv1.Resource(),
@@ -131,11 +165,16 @@ class TestCase:
131165
),
132166
),
133167
TestCase(
134-
reason="Updating from a Pydantic model should work.",
168+
# This model uses the default_factory form that older
169+
# datamodel-code-generator emits for fields with an object
170+
# default. providerConfigRef has such a default but isn't set
171+
# here, so it must not be emitted.
172+
reason="Updating from a Pydantic model with default_factory "
173+
"object defaults should omit unset fields.",
135174
r=fnv1.Resource(),
136-
source=v1beta2.Bucket(
137-
spec=v1beta2.Spec(
138-
forProvider=v1beta2.ForProvider(region="us-west-2"),
175+
source=s3v1beta2.Bucket(
176+
spec=s3v1beta2.Spec(
177+
forProvider=s3v1beta2.ForProvider(region="us-west-2"),
139178
),
140179
),
141180
want=fnv1.Resource(
@@ -148,6 +187,98 @@ class TestCase:
148187
),
149188
),
150189
),
190+
TestCase(
191+
# This model uses the validate_default=True form that newer
192+
# datamodel-code-generator emits for fields with an object
193+
# default. providerConfigRef has such a default but isn't set
194+
# here, so it must not be emitted.
195+
reason="Updating from a Pydantic model with validate_default "
196+
"object defaults should omit unset fields.",
197+
r=fnv1.Resource(),
198+
source=accountaliasv1beta1.AccountAlias(
199+
spec=accountaliasv1beta1.Spec(forProvider={"x": "y"}),
200+
),
201+
want=fnv1.Resource(
202+
resource=resource.dict_to_struct(
203+
{
204+
"apiVersion": "iam.aws.m.upbound.io/v1beta1",
205+
"kind": "AccountAlias",
206+
"spec": {"forProvider": {"x": "y"}},
207+
}
208+
),
209+
),
210+
),
211+
TestCase(
212+
# datamodel-code-generator can't name a field bool or int, so
213+
# it emits bool_ aliased to bool and int_ aliased to int. The
214+
# alias is the resource's real wire name, so update must emit
215+
# fields under their aliases.
216+
reason="Updating from a Pydantic model with keyword-aliased "
217+
"fields should emit the fields under their aliases.",
218+
r=fnv1.Resource(),
219+
source=resourcev1.ResourceSlice(
220+
spec=resourcev1.Spec(
221+
devices=[
222+
resourcev1.Device(
223+
name="gpu",
224+
attributes={
225+
"powered": resourcev1.DeviceAttribute(
226+
**{"bool": True},
227+
),
228+
"lanes": resourcev1.DeviceAttribute(
229+
**{"int": 16},
230+
),
231+
},
232+
),
233+
],
234+
),
235+
),
236+
want=fnv1.Resource(
237+
resource=resource.dict_to_struct(
238+
{
239+
"apiVersion": "resource.k8s.io/v1",
240+
"kind": "ResourceSlice",
241+
"spec": {
242+
"devices": [
243+
{
244+
"name": "gpu",
245+
"attributes": {
246+
"powered": {"bool": True},
247+
"lanes": {"int": 16},
248+
},
249+
},
250+
],
251+
},
252+
}
253+
),
254+
),
255+
),
256+
TestCase(
257+
# managementPolicies defaults to ["*"] and is set to ["*"]
258+
# here. A field the caller sets is one it has an opinion about
259+
# and should own, even when the value equals the default.
260+
reason="A field the caller explicitly set to its default value "
261+
"should be emitted.",
262+
r=fnv1.Resource(),
263+
source=accountaliasv1beta1.AccountAlias(
264+
spec=accountaliasv1beta1.Spec(
265+
forProvider={"x": "y"},
266+
managementPolicies=["*"],
267+
),
268+
),
269+
want=fnv1.Resource(
270+
resource=resource.dict_to_struct(
271+
{
272+
"apiVersion": "iam.aws.m.upbound.io/v1beta1",
273+
"kind": "AccountAlias",
274+
"spec": {
275+
"forProvider": {"x": "y"},
276+
"managementPolicies": ["*"],
277+
},
278+
}
279+
),
280+
),
281+
),
151282
]
152283

153284
for case in cases:
@@ -158,6 +289,66 @@ class TestCase:
158289
"-want, +got",
159290
)
160291

292+
def test_model_round_trip(self) -> None:
293+
# A function reads an observed resource (wire names), validates it into
294+
# a model, then writes it back via update. A field that goes in under
295+
# its wire name must come back out under the same wire name. This pins
296+
# the property the by_alias fix exists to guarantee: validation accepts
297+
# the alias, and serialization must emit the alias, not the Python
298+
# attribute name. It does not assert anything about fields the model
299+
# doesn't define (pydantic drops them) or value types (Struct coerces
300+
# numbers to float).
301+
@dataclasses.dataclass
302+
class TestCase:
303+
reason: str
304+
# The resource as it arrives from Crossplane, using wire names.
305+
observed: dict
306+
# The model type to validate the observed resource into.
307+
model: type[pydantic.BaseModel]
308+
309+
cases = [
310+
TestCase(
311+
reason="A model with keyword-aliased fields should round-trip "
312+
"through validation and update with its fields under the same "
313+
"wire names (bool, int) they arrived under.",
314+
observed={
315+
"apiVersion": "resource.k8s.io/v1",
316+
"kind": "ResourceSlice",
317+
"spec": {
318+
"devices": [
319+
{
320+
"name": "gpu",
321+
"attributes": {
322+
"powered": {"bool": True},
323+
"lanes": {"int": 16},
324+
"model": {"string": "h100"},
325+
},
326+
},
327+
],
328+
},
329+
},
330+
model=resourcev1.ResourceSlice,
331+
),
332+
TestCase(
333+
reason="A model with only ordinary fields should round-trip unchanged.",
334+
observed={
335+
"apiVersion": "s3.aws.upbound.io/v1beta2",
336+
"kind": "Bucket",
337+
"spec": {"forProvider": {"region": "us-west-2"}},
338+
},
339+
model=s3v1beta2.Bucket,
340+
),
341+
]
342+
343+
for case in cases:
344+
# Mimic the SDK flow: a function reads an observed resource (wire
345+
# names), validates it into a model, then writes it back out.
346+
m = case.model.model_validate(case.observed)
347+
r = fnv1.Resource()
348+
resource.update(r, m)
349+
got = resource.struct_to_dict(r.resource)
350+
self.assertEqual(case.observed, got, case.reason)
351+
161352
def test_get_condition(self) -> None:
162353
@dataclasses.dataclass
163354
class TestCase:

‎tests/testdata/models/io/k8s/api/__init__.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
# generated by datamodel-codegen:
2+
# filename: <stdin>

‎tests/testdata/models/io/k8s/api/resource/__init__.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
# generated by datamodel-codegen:
2+
# filename: <stdin>

‎tests/testdata/models/io/k8s/api/resource/v1.py‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
# generated by datamodel-codegen:
2+
# filename: <stdin>
3+
4+
from __future__ import annotations
5+
6+
from typing import Literal
7+
8+
from pydantic import BaseModel, Field
9+
10+
from ...apimachinery.pkg.apis.meta import v1
11+
12+
13+
class DeviceAttribute(BaseModel):
14+
bool_: bool | None = Field(None, alias='bool')
15+
"""
16+
BoolValue is a true/false value.
17+
"""
18+
int_: int | None = Field(None, alias='int')
19+
"""
20+
IntValue is a number.
21+
"""
22+
string: str | None = None
23+
"""
24+
StringValue is a string.
25+
"""
26+
version: str | None = None
27+
"""
28+
VersionValue is a semantic version according to semver.org spec 2.0.0.
29+
"""
30+
31+
32+
class Device(BaseModel):
33+
name: str
34+
"""
35+
Name is a unique identifier among all devices managed by the driver.
36+
"""
37+
attributes: dict[str, DeviceAttribute] | None = None
38+
"""
39+
Attributes defines the set of attributes for this device.
40+
"""
41+
42+
43+
class Spec(BaseModel):
44+
devices: list[Device] | None = None
45+
"""
46+
Devices lists the devices in this resource slice.
47+
"""
48+
49+
50+
class ResourceSlice(BaseModel):
51+
apiVersion: Literal['resource.k8s.io/v1'] | None = 'resource.k8s.io/v1'
52+
"""
53+
APIVersion defines the versioned schema of this representation of an object.
54+
"""
55+
kind: Literal['ResourceSlice'] | None = 'ResourceSlice'
56+
"""
57+
Kind is a string value representing the REST resource this object represents.
58+
"""
59+
metadata: v1.ObjectMeta | None = None
60+
"""
61+
Standard object's metadata.
62+
"""
63+
spec: Spec
64+
"""
65+
Contents of the ResourceSlice.
66+
"""

‎tests/testdata/models/io/upbound/m/__init__.py‎

Whitespace-only changes.

‎tests/testdata/models/io/upbound/m/aws/__init__.py‎

Whitespace-only changes.

‎tests/testdata/models/io/upbound/m/aws/iam/__init__.py‎

Whitespace-only changes.

‎tests/testdata/models/io/upbound/m/aws/iam/accountalias/__init__.py‎

Whitespace-only changes.

0 commit comments

Comments
 (0)