Math transform schema validation error when multiplying integer with integer (integer vs float64)

[chlunde]

What happened?

Transform failed when upgrading to crossplane 1.14.3:

        - fromFieldPath: spec.parameters.diskSize
          toFieldPath: spec.forProvider.maxAllocatedStorage
          transforms:
            - type: math
              math:
                type: Multiply
                multiply: 2

Log message:

Warning: Composition "xxxx" invalid for schema-aware validation: spec.resources[1].patches[8].transforms: Invalid value: []v1.Transform{v1.Transform{Type:"math", Math:(*v1.MathTransform)(0xc0148796b0), Map:(*v1.MapTransform)(nil), Match:(*v1.MatchTransform)(nil), String:(*v1.StringTransform)(nil), Convert:(*v1.ConvertTransform)(nil)}}: the provided transforms do not output a type compatible with the toFieldPath according to the schema: integer != integer

Reproducer unit test:

diff --git a/pkg/validation/apiextensions/v1/composition/patches.go b/pkg/validation/apiextensions/v1/composition/patches.go
index be5af468..d7c2ee4a 100644
--- a/pkg/validation/apiextensions/v1/composition/patches.go
+++ b/pkg/validation/apiextensions/v1/composition/patches.go
@@ -311,7 +311,6 @@ func validateIOTypesWithTransforms(transforms []v1.Transform, fromType, toType x
        if fieldErr != nil {
                return fieldErr
        }

        if transformsOutputType == "" || toType == "" || xpschema.FromTransformIOType(transformsOutputType).IsEquivalent(toType) {
                return nil
        }
@@ -319,6 +318,7 @@ func validateIOTypesWithTransforms(transforms []v1.Transform, fromType, toType x
        if len(transforms) == 0 {
                return field.Required(field.NewPath("transforms"), fmt.Sprintf("the fromFieldPath does not have a type compatible with the toFieldPath according to their schemas and no transforms were provided: %s != %s", fromType, toType))
        }
+       fmt.Println(fromType, toType, transformsOutputType)
        return field.Invalid(field.NewPath("transforms"), transforms, fmt.Sprintf("the provided transforms do not output a type compatible with the toFieldPath according to the schema: %s != %s", fromType, toType))
 }
 
diff --git a/pkg/validation/apiextensions/v1/composition/patches_test.go b/pkg/validation/apiextensions/v1/composition/patches_test.go
index d4a7c67f..1d405154 100644
--- a/pkg/validation/apiextensions/v1/composition/patches_test.go
+++ b/pkg/validation/apiextensions/v1/composition/patches_test.go
@@ -275,6 +275,28 @@ func TestValidateTransforms(t *testing.T) {
                                toType:   "",
                        },
                },
+               "MathTransformIntegerOutput": {
+                       reason: "int * int = int",
+                       want:   want{},
+                       args: args{
+                               transforms: []v1.Transform{
+                                       {
+                                               Type: v1.TransformTypeMath,
+                                               Math: &v1.MathTransform{
+                                                       Multiply: ptr.To[int64](2),
+                                               },
+                                       },/* Does not work without this:
+                                       {
+                                               Type: v1.TransformTypeConvert,
+                                               Convert: &v1.ConvertTransform{
+                                                       ToType: "int64",
+                                               },
+                                       },*/
+                               },
+                               fromType: "integer",
+                               toType:   "integer",
+                       },
+               },
                "RejectNoToTypeInvalidInputType": {
                        reason: "Should reject if there is no type spec for the output, but input is specified and transforms are wrong",
                        want: want{err: &field.Error{

How can we reproduce it?

Add a transform that multiplies two integers and store it into an integer, see above

I think it would also be helpful to log transformsOutputType as the current message integer != integer is confusing :)

What environment did it happen in?

Crossplane version: 1.14.3

[phisco]

Hi! Thanks for reporting this and for the analysis @chlunde! 🙏

The error should definitely use transformsOutputType instead of fromType, as what we care about is the output of the transformations and the type expected by the destination according to the schema, good catch!

return field.Invalid(field.NewPath("transforms"), transforms, fmt.Sprintf("the provided transforms do not output a type compatible with the toFieldPath according to the schema: %s != %s", transformsOutputType, toType))

In this case, we assume all outputs of Math transforms to be floats64 here, which is then mapped to a number by FromTransformIOType and compared to the expected output that should be integer, which we are rejecting as patching a number into an integer could indeed lead to issues. I don't think we could be more specific in FromTransformIOType without actually running the transform though 🤔

[phisco]

Actually we could have inferred it, if Transform.GetOutputType took in consideration the type of math transform at hand.

Multiply: the multiplier is always an integer, so we should decide the output type according to the same logic: integer*integer=integer and number*integer=number.

ClampMin/Max: it's a bit trickier, in this case the implementation always returns an integer if the input number is below/above the specified value, otherwise it returns the original input type. So in case of integer as input, we know it's going to be an integer in output, but in case of a number, the safest assumption would be to return no type at all, but we could also return a number as potentially it could result into one and result in an implicit lossy casting in the end. 🤔

[github-actions]

Crossplane does not currently have enough maintainers to address every issue and pull request. This issue has been automatically marked as stale because it has had no activity in the last 90 days. It will be closed in 14 days if no further activity occurs. Leaving a comment starting with /fresh will mark this issue as not stale.

[phisco]

/fresh

[github-actions]

Crossplane does not currently have enough maintainers to address every issue and pull request. This issue has been automatically marked as stale because it has had no activity in the last 90 days. It will be closed in 14 days if no further activity occurs. Leaving a comment starting with /fresh will mark this issue as not stale.