Skip to content

Commit 19017f2

Browse files
TechassiTechassi
committed
fix(stackable-versioned): Fix edge-cases uncovered by UI tests
1 parent 4009271 commit 19017f2

File tree

3 files changed

+154
-76
lines changed

3 files changed

+154
-76
lines changed

‎crates/stackable-versioned-macros/src/codegen/container/struct/conversion.rs

Lines changed: 46 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use std::{borrow::Cow, cmp::Ordering, ops::Not as _};
1+
use std::{borrow::Cow, cmp::Ordering};
22

33
use indoc::formatdoc;
44
use itertools::Itertools as _;
@@ -44,14 +44,6 @@ impl Struct {
4444
return None;
4545
}
4646

47-
if !mod_gen_ctx
48-
.kubernetes_options
49-
.experimental_conversion_tracking
50-
.is_present()
51-
{
52-
return None;
53-
}
54-
5547
let next_version = ver_ctx.next_version;
5648
let version = ver_ctx.version;
5749

@@ -69,29 +61,49 @@ impl Struct {
6961
Direction::Downgrade => (&version.idents.module, &next_version.idents.module),
7062
};
7163

64+
// NOTE (@Techassi): This if statement can be removed once experimental_conversion_tracking
65+
// is gone.
66+
let from_inner = if mod_gen_ctx.kubernetes_options.experimental_conversion_tracking.is_present() {
67+
quote! {
68+
// The status is optional. The be able to track changes in nested sub structs it needs
69+
// to be initialized with a default value.
70+
let mut status = #from_struct_parameter_ident.status.unwrap_or_default();
71+
72+
// Convert the spec and track values in the status
73+
let spec =
74+
<#for_module_ident::#spec_struct_ident as #versioned_path::TrackingFrom<_, _>>::tracking_from(
75+
#from_struct_parameter_ident.spec,
76+
&mut status,
77+
"",
78+
);
79+
80+
// Construct the final object by copying over the metadata, setting the status and
81+
// using the converted spec.
82+
Self {
83+
metadata: #from_struct_parameter_ident.metadata,
84+
status: Some(status),
85+
spec,
86+
}
87+
}
88+
} else {
89+
let status = spec_gen_ctx.kubernetes_arguments.status
90+
.as_ref()
91+
.map(|_| quote! { status: #from_struct_parameter_ident.status,});
92+
93+
quote! {
94+
Self {
95+
metadata: #from_struct_parameter_ident.metadata,
96+
spec: #from_struct_parameter_ident.spec.into(),
97+
#status
98+
}
99+
}
100+
};
101+
72102
quote! {
73103
#automatically_derived
74104
impl ::std::convert::From<#from_module_ident::#object_struct_ident> for #for_module_ident::#object_struct_ident {
75105
fn from(#from_struct_parameter_ident: #from_module_ident::#object_struct_ident) -> Self {
76-
// The status is optional. The be able to track changes in nested sub structs it needs
77-
// to be initialized with a default value.
78-
let mut status = #from_struct_parameter_ident.status.unwrap_or_default();
79-
80-
// Convert the spec and track values in the status
81-
let spec =
82-
<#for_module_ident::#spec_struct_ident as #versioned_path::TrackingFrom<_, _>>::tracking_from(
83-
#from_struct_parameter_ident.spec,
84-
&mut status,
85-
"",
86-
);
87-
88-
// Construct the final object by copying over the metadata, setting the status and
89-
// using the converted spec.
90-
Self {
91-
metadata: #from_struct_parameter_ident.metadata,
92-
status: Some(status),
93-
spec,
94-
}
106+
#from_inner
95107
}
96108
}
97109
}
@@ -126,10 +138,7 @@ impl Struct {
126138

127139
// Only add the #[automatically_derived] attribute only if this impl is used
128140
// outside of a module (in standalone mode).
129-
let automatically_derived = mod_gen_ctx
130-
.add_attributes
131-
.not()
132-
.then(|| quote! {#[automatically_derived]});
141+
let automatically_derived = mod_gen_ctx.automatically_derived_attr();
133142

134143
let fields = |direction: Direction| -> TokenStream {
135144
self.fields
@@ -169,8 +178,8 @@ impl Struct {
169178
where
170179
S: #versioned_path::TrackingStatus + ::core::default::Default
171180
{
172-
#[allow(unused)]
173181
fn tracking_from(#from_struct_ident: #from_module_ident::#struct_ident, status: &mut S, parent: &str) -> Self {
182+
// TODO (@Techassi): Only emit this if any of the fields below need it
174183
use #versioned_path::TrackingInto as _;
175184

176185
#json_paths
@@ -275,10 +284,6 @@ impl Struct {
275284
}
276285

277286
fn generate_json_paths(&self, next_version: &VersionDefinition) -> Option<TokenStream> {
278-
// if !self.needs_tracking(next_version) {
279-
// return None;
280-
// }
281-
282287
let json_paths = self
283288
.fields
284289
.iter()
@@ -585,6 +590,10 @@ impl Struct {
585590
mod_gen_ctx: ModuleGenerationContext<'_>,
586591
spec_gen_ctx: &SpecGenerationContext<'_>,
587592
) -> Option<TokenStream> {
593+
if mod_gen_ctx.skip.try_convert.is_present() || self.common.options.skip_try_convert {
594+
return None;
595+
}
596+
588597
let variant_data_ident = &spec_gen_ctx.kubernetes_idents.parameter;
589598
let variant_idents = &spec_gen_ctx.variant_idents;
590599

‎crates/stackable-versioned-macros/src/codegen/container/struct/mod.rs

Lines changed: 91 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ impl Container {
4545
// Ensure that the struct name includes the 'Spec' suffix.
4646
if kubernetes_data.is_some() && !idents.original.as_str().ends_with("Spec") {
4747
return Err(Error::custom(
48-
"struct name needs to include the `Spec` suffix if Kubernetes features are enabled via `#[versioned(k8s())]`"
48+
"struct name needs to include the `Spec` suffix if CRD features are enabled via `#[versioned(crd())]`"
4949
).with_span(&idents.original.span()));
5050
}
5151

@@ -207,7 +207,7 @@ impl Struct {
207207
fn generate_kube_attribute(
208208
&self,
209209
ver_ctx: VersionContext<'_>,
210-
gen_ctx: ModuleGenerationContext<'_>,
210+
mod_gen_ctx: ModuleGenerationContext<'_>,
211211
spec_gen_ctx: &SpecGenerationContext<'_>,
212212
) -> Option<TokenStream> {
213213
// Required arguments
@@ -234,27 +234,37 @@ impl Struct {
234234
.as_ref()
235235
.map(|p| quote! { , plural = #p });
236236

237-
let crates = gen_ctx.crates;
237+
let crates = mod_gen_ctx.crates;
238238

239239
let namespaced = spec_gen_ctx
240240
.kubernetes_arguments
241241
.namespaced
242242
.is_present()
243243
.then_some(quote! { , namespaced });
244244

245+
// NOTE (@Techassi): What an abomination
245246
let status = match (
246-
gen_ctx
247+
mod_gen_ctx
247248
.kubernetes_options
248249
.experimental_conversion_tracking
249250
.is_present(),
250251
&spec_gen_ctx.kubernetes_arguments.status,
251252
) {
252-
(true, _) => {
253-
let status_ident = &spec_gen_ctx.kubernetes_idents.status;
254-
Some(quote! { , status = #status_ident })
253+
(true, status_path) => {
254+
if (mod_gen_ctx.skip.merged_crd.is_present() || self.common.options.skip_merged_crd)
255+
&& (mod_gen_ctx.skip.try_convert.is_present()
256+
|| self.common.options.skip_try_convert)
257+
{
258+
status_path
259+
.as_ref()
260+
.map(|status_path| quote! { , status = #status_path })
261+
} else {
262+
let status_ident = &spec_gen_ctx.kubernetes_idents.status;
263+
Some(quote! { , status = #status_ident })
264+
}
255265
}
256-
(_, Some(status_ident)) => Some(quote! { , status = #status_ident }),
257-
(_, _) => None,
266+
(false, Some(status_path)) => Some(quote! { , status = #status_path }),
267+
_ => None,
258268
};
259269

260270
let shortnames: TokenStream = spec_gen_ctx
@@ -306,14 +316,7 @@ impl Struct {
306316
) -> TokenStream {
307317
let enum_ident = &spec_gen_ctx.kubernetes_idents.kind;
308318

309-
// Only generate merged_crd associated function if not opted out
310-
let merged_crd_fn =
311-
if !mod_gen_ctx.skip.merged_crd.is_present() && !self.common.options.skip_merged_crd {
312-
Some(self.generate_merged_crd_fn(mod_gen_ctx, spec_gen_ctx))
313-
} else {
314-
None
315-
};
316-
319+
let merged_crd_fn = self.generate_merged_crd_fn(mod_gen_ctx, spec_gen_ctx);
317320
let try_convert_fn = self.generate_try_convert_fn(versions, mod_gen_ctx, spec_gen_ctx);
318321
let from_json_value_fn = self.generate_from_json_value_fn(mod_gen_ctx, spec_gen_ctx);
319322
let into_json_value_fn = self.generate_into_json_value_fn(mod_gen_ctx, spec_gen_ctx);
@@ -421,22 +424,84 @@ impl Struct {
421424
return None;
422425
}
423426

424-
if !mod_gen_ctx
425-
.kubernetes_options
426-
.experimental_conversion_tracking
427-
.is_present()
428-
{
429-
return None;
430-
}
431-
432427
let next_version = ver_ctx.next_version;
433428
let version = ver_ctx.version;
434429

435430
next_version.map(|next_version| {
436-
self.generate_tracking_from_impl(direction, version, next_version, mod_gen_ctx)
431+
if mod_gen_ctx
432+
.kubernetes_options
433+
.experimental_conversion_tracking
434+
.is_present()
435+
{
436+
self.generate_tracking_from_impl(direction, version, next_version, mod_gen_ctx)
437+
} else {
438+
self.generate_plain_from_impl(direction, version, next_version, mod_gen_ctx)
439+
}
437440
})
438441
}
439442

443+
fn generate_plain_from_impl(
444+
&self,
445+
direction: Direction,
446+
version: &VersionDefinition,
447+
next_version: &VersionDefinition,
448+
mod_gen_ctx: ModuleGenerationContext<'_>,
449+
) -> TokenStream {
450+
// TODO (@Techassi): A bunch this stuff is duplicated in self.generate_tracking_from_impl.
451+
// Ideally we remove that duplication.
452+
let from_struct_ident = &self.common.idents.parameter;
453+
let struct_ident = &self.common.idents.original;
454+
455+
// Include allow(deprecated) only when this or the next version is
456+
// deprecated. Also include it, when a field in this or the next
457+
// version is deprecated.
458+
let allow_attribute = (version.deprecated.is_some()
459+
|| next_version.deprecated.is_some()
460+
|| self.is_any_field_deprecated(version)
461+
|| self.is_any_field_deprecated(next_version))
462+
.then(|| quote! { #[allow(deprecated)] });
463+
464+
// Only add the #[automatically_derived] attribute only if this impl is used
465+
// outside of a module (in standalone mode).
466+
let automatically_derived = mod_gen_ctx.automatically_derived_attr();
467+
468+
let fields = |direction: Direction| -> TokenStream {
469+
self.fields
470+
.iter()
471+
.filter_map(|f| {
472+
f.generate_for_from_impl(direction, version, next_version, from_struct_ident)
473+
})
474+
.collect()
475+
};
476+
477+
let (fields, for_module_ident, from_module_ident) = match direction {
478+
direction @ Direction::Upgrade => {
479+
let from_module_ident = &version.idents.module;
480+
let for_module_ident = &next_version.idents.module;
481+
482+
(fields(direction), for_module_ident, from_module_ident)
483+
}
484+
direction @ Direction::Downgrade => {
485+
let from_module_ident = &next_version.idents.module;
486+
let for_module_ident = &version.idents.module;
487+
488+
(fields(direction), for_module_ident, from_module_ident)
489+
}
490+
};
491+
492+
quote! {
493+
#automatically_derived
494+
#allow_attribute
495+
impl ::core::convert::From<#from_module_ident::#struct_ident> for #for_module_ident::#struct_ident {
496+
fn from(#from_struct_ident: #from_module_ident::#struct_ident) -> Self {
497+
Self {
498+
#fields
499+
}
500+
}
501+
}
502+
}
503+
}
504+
440505
/// Returns whether any field is deprecated in the provided `version`.
441506
fn is_any_field_deprecated(&self, version: &VersionDefinition) -> bool {
442507
// First, iterate over all fields. The `any` function will return true

‎crates/stackable-versioned-macros/src/codegen/item/field.rs

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -331,16 +331,19 @@ impl VersionedField {
331331
Direction::Upgrade => {
332332
let next_change = changes.get_expect(&next_version.inner);
333333

334-
let ident = &self.ident;
335-
let json_path_ident = format_ident!("__sv_{}_path", &self.ident.as_ident());
336-
337334
match next_change {
338-
ItemStatus::NotPresent | ItemStatus::NoChange { .. } => None,
339-
_ => Some(quote! {
340-
field_name if field_name == #json_path_ident => {
341-
spec.#ident = serde_yaml::from_value(value).unwrap();
342-
},
343-
}),
335+
// NOTE (@Techassi): We currently only support tracking added fields. As such
336+
// we only need to generate code if the next change is "Addition".
337+
ItemStatus::Addition { ident, .. } => {
338+
let json_path_ident = format_ident!("__sv_{}_path", ident.as_ident());
339+
340+
Some(quote! {
341+
field_name if field_name == #json_path_ident => {
342+
spec.#ident = serde_yaml::from_value(value).unwrap();
343+
},
344+
})
345+
}
346+
_ => None,
344347
}
345348
}
346349
Direction::Downgrade => None,
@@ -369,14 +372,15 @@ impl VersionedField {
369372
let next_change = changes.get_expect(&next_version.inner);
370373

371374
match next_change {
372-
ItemStatus::NoChange { .. } | ItemStatus::NotPresent => None,
373-
_ => {
374-
let field_ident = format_ident!("__sv_{}_path", &self.ident.as_ident());
375-
let child_string = &self.ident.to_string();
375+
ItemStatus::Addition { ident, .. } => {
376+
let field_ident = format_ident!("__sv_{}_path", ident.as_ident());
377+
let child_string = ident.to_string();
378+
376379
Some(quote! {
377380
let #field_ident = ::stackable_versioned::jthong_path(parent, #child_string);
378381
})
379382
}
383+
_ => None,
380384
}
381385
}
382386
}

0 commit comments

Comments
 (0)