Skip to content

Pydantic, exclude_defaults=True re-add apiVersion and kind #114

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.

Sign up for GitHub

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

Merged
negz merged 1 commit into from
Jan 8, 2025
Merged

Pydantic, exclude_defaults=True re-add apiVersion and kind #114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion crossplane/function/resource.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,15 @@ def update(r: fnv1.Resource, source: dict | structpb.Struct | pydantic.BaseModel
"""
match source:
case pydantic.BaseModel():
r.resource.update(source.model_dump(exclude_defaults=True, warnings=False))
data = source.model_dump(exclude_defaults=True, warnings=False)
# In Pydantic, exclude_defaults=True in model_dump excludes fields
# that have their value equal to the default. If a field like
# apiVersion is set to its default value 's3.aws.upbound.io/v1beta2'
# (and not explicitly provided during initialization), it will be
# excluded from the serialized output.
data['apiVersion'] = source.apiVersion
data['kind'] = source.kind
r.resource.update(data)
Comment on lines +41 to +49
Copy link
Member

@negz negz Jan 6, 2025

Choose a reason for hiding this comment

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

I don't love this workaround but I can't think of a better option.

I used exclude_defaults to stop model_dump from serializing optional fields as explicit None values. We could use exclude_none instead, but I think apart from apiVersion and kind we really don't want to include defaults - since we want to leave the defaulting to server-side apply.

Copy link
Member

@negz negz Jan 6, 2025

Choose a reason for hiding this comment

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

Actually, how feasible would it be to do the following?

  1. Update the models to remove all default values (except for apiVersion and kind)
  2. Switch to exclude_none

Copy link
Member Author

@haarchri haarchri Jan 6, 2025

Choose a reason for hiding this comment

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

people will lose the information which default values are available - in lsp - mhm

Copy link
Member Author

@haarchri haarchri Jan 7, 2025

Choose a reason for hiding this comment

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

apiVersion and kind are special - so i don't think the solution at all is a problem

case structpb.Struct():
# TODO(negz): Use struct_to_dict and update to match other semantics?
r.resource.MergeFrom(source)
Expand Down
6 changes: 5 additions & 1 deletion tests/test_resource.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,11 @@ class TestCase:
),
want=fnv1.Resource(
resource=resource.dict_to_struct(
{"spec": {"forProvider": {"region": "us-west-2"}}}
{
"apiVersion": "s3.aws.upbound.io/v1beta1",
"kind": "Bucket",
"spec": {"forProvider": {"region": "us-west-2"}},
}
),
),
),
Expand Down
4 changes: 2 additions & 2 deletions tests/testdata/models/io/upbound/aws/s3/v1beta2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -759,11 +759,11 @@ class Status(BaseModel):


class Bucket(BaseModel):
apiVersion: Optional[str] = None
apiVersion: Optional[str] = 's3.aws.upbound.io/v1beta2'
"""
APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources
"""
kind: Optional[str] = None
kind: Optional[str] = 'Bucket'
"""
Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
"""
Expand Down
Loading