Skip to content
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.

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

Generic golang interface{} serialization gives an empty object for structs #1326

Closed
yuriy-yarosh opened this issue May 13, 2023 · 8 comments
Closed
Labels
bug Something isn't working closed-for-staleness Issue/PR was closed due to staleness effort/small 1 day tops priority/p1 Should be on near term plans

Comments

@yuriy-yarosh
Copy link

yuriy-yarosh commented May 13, 2023

Description of the bug:

I'm facing something similar to #1291 with Crossplane.

Crossplane CompositeResourceDefinition (XRD) CRD defines openAPIV3Schema as a type: object, which results in an empty object synthesis and plain interface{} generation.

So, for a crossplane XRD, this

Versions: &([]*xrd.CompositeResourceDefinitionSpecVersions{{
	Name:                     jsii.String("v1alpha1"),
	Referenceable:            jsii.Bool(true),
	Served:                   jsii.Bool(true),
	Schema: &xrd.CompositeResourceDefinitionSpecVersionsSchema{
		OpenApiv3Schema: &k8s.JsonSchemaProps{
			Type: jsii.String("Object"),
		},
	},
}}),

CRD codegen gives an interface{} for a type: object schema

properties:
  openAPIV3Schema:
    description: OpenAPIV3Schema is the OpenAPI v3 schema to use for validation and pruning.
    type: object
    x-kubernetes-preserve-unknown-fields: true
type CompositeResourceDefinitionSpecVersionsSchema struct {
	// OpenAPIV3Schema is the OpenAPI v3 schema to use for validation and pruning.
	OpenApiv3Schema interface{} `field:"optional" json:"openApiv3Schema" yaml:"openApiv3Schema"`
}

it synthesizes just an empty object {}, disregarding the contents

  versions:
    - name: v1alpha1
      referenceable: true
      schema:
        openAPIV3Schema: {}
      served: true

It should be possible to cast existing struct to interface{}, or at least convert it to mapstructure as map[string]interface{} using the respective yaml reflection tags out of the existing k8s JsonSchemaProps, to overcome this. But I'd like a bit more concise solution, or at least an exception During Synthesis, instead of an empty output.

Reproduction Steps:

  1. Import a CRD schema with type: object which would generate interface{}
  2. Pass a struct{} with something there
  3. Get empty output during synthesis

Environment:

  • github.com/cdk8s-team/cdk8s-core-go/cdk8s/v2 v2.7.59
  • github.com/aws/jsii-runtime-go v1.81.0
  • github.com/aws/constructs-go/constructs/v10 v10.2.20

This is 🐛 Bug Report

@yuriy-yarosh yuriy-yarosh added bug Something isn't working needs-triage Priority and effort undetermined yet labels May 13, 2023
@RomainMuller
Copy link
Member

Reviewed this case from a jsii perspective, and here are my findings (re-stating the obvious, too):

  • openAPIV3Schema is typed object in the underlying schema
  • This object type is rendered as any by cdk8s import. The any type passes values by-reference.
    /**
     * Schema describes the schema used for validation, pruning, and defaulting of this version of the defined composite resource. Fields required by all composite resources will be injected into this schema automatically, and will override equivalently named fields in this schema. Omitting this schema results in a schema that contains only the fields   required by all composite resources.
     *
     * @schema CompositeResourceDefinitionSpecVersionsSchema
     */
    export interface CompositeResourceDefinitionSpecVersionsSchema {
      /**
       * OpenAPIV3Schema is the OpenAPI v3 schema to use for validation and pruning.
       *
       * @schema CompositeResourceDefinitionSpecVersionsSchema#openAPIV3Schema
       */
      readonly openApiv3Schema?: any;
    
    }
  • In this case, k8s.JSONSchemaProps is not a known jsii type, so it is passed as an opaque reference (jsii Object with no properties or methods)
  • It appears That cdk8s proceeds to use JSON.stringify on that opaque value, and since it has no properties nor methods, this results in {}.

There is no straight-forward way to explicitly represent "object with a specified JSON representation" in the jsii type model today except for using Record<string, _> (aka { [key: string]: T }) which has "meh" semantics here.

I tend to think it would be better to model properties typed as "opaque" object in the underlying schema to the map type, so that users don't get caught in this particular trap. Alternatively, we may want to consider requesting new jsii features, such as ability to explicitly bind a toJSON implementation to a user-defined type, or introduction of a normalized "JSON-representable value".


Now - while this is not very elegant, you can work around this issue today by actually pointer here instead of k8s.JSONSchemaProps to achieve the desired synth output:

	xp.NewCompositeResourceDefinition(chart, jsii.String("CrossPlane"), &xp.CompositeResourceDefinitionProps{
		Spec: &xp.CompositeResourceDefinitionSpec{
			Group: jsii.String("group"),
			Names: &xp.CompositeResourceDefinitionSpecNames{
				Kind:   jsii.String("kind"),
				Plural: jsii.String("kinds"),
			},
			Versions: &([]*xp.CompositeResourceDefinitionSpecVersions{
				{
					Name:          jsii.String("v1alpha1"),
					Referenceable: jsii.Bool(true),
					Served:        jsii.Bool(true),
					Schema: &xp.CompositeResourceDefinitionSpecVersionsSchema{
						OpenApiv3Schema: &map[string]interface{}{
							"type": "object",
						},
					},
				},
			}),
		},
	})

results in the following output:

apiVersion: apiextensions.crossplane.io/v1
kind: CompositeResourceDefinition
metadata:
  name: test-chart-crossplane-c893e157
spec:
  group: group
  names:
    kind: kind
    plural: kinds
  versions:
    - name: v1alpha1
      referenceable: true
      schema:
        openAPIV3Schema:
          type: object
      served: true

@RomainMuller
Copy link
Member

I'm still going to verify if it'd be feasible to detect "exported" (aka public) properties of these structs and appropriately register them upon proxy object creation. This might be enough to make such scenarios work.

@RomainMuller
Copy link
Member

I was able to confirm that registering properties correctly here results in the expected behavior. I'll make a PR into the jsii runtime for Go with this soon (I need to write a couple of tests first).

RomainMuller added a commit to aws/jsii that referenced this issue May 15, 2023
When passing arbitrary structs through an `interface{}` type, the JS
value had no properties or methods, leading to it having an empty object
JSON representation.

In order to improve the situation, discover exported properties of such
structs, and register them as property callbacks, more closely matching
the behavior of JS in the same configuration.

The callbacks use the same naming transformation as the standard JSON
serialization mechanism in go: using the `json` tag if present with the
same semantics as `encoding/json`, and fall back to the raw field name
otherwise (with no case conversion applied).

Related: cdk8s-team/cdk8s#1326
@RomainMuller
Copy link
Member

Submitted aws/jsii#4104, which I believe addresses the issue at hand here (I mean - it does in the tests I ran with your original code snippet above).

@iliapolo iliapolo added effort/small 1 day tops priority/p1 Should be on near term plans and removed needs-triage Priority and effort undetermined yet labels May 16, 2023
@iliapolo
Copy link
Member

@RomainMuller thanks for diving into this!

mergify bot pushed a commit to aws/jsii that referenced this issue May 16, 2023
When passing arbitrary structs through an `interface{}` type, the JS value had no properties or methods, leading to it having an empty object JSON representation.

In order to improve the situation, discover exported properties of such structs, and register them as property callbacks, more closely matching the behavior of JS in the same configuration.

The callbacks use the same naming transformation as the standard JSON serialization mechanism in go: using the `json` tag if present with the same semantics as `encoding/json`, and fall back to the raw field name otherwise (with no case conversion applied).

Related: cdk8s-team/cdk8s#1326



---

By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license].

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
@yuriy-yarosh
Copy link
Author

yuriy-yarosh commented Jun 4, 2023

@RomainMuller I don't really get what had been patched

OpenApiv3Schema: &map[string]interface{}{
   "type": "object",
}

Worked previously anyway, but

OpenApiv3Schema: &k8s.JsonSchemaProps{ ... }

Gives me Module 'k8s' not found as per jsii/#4097.
So, I'm still stuck with map[string]interface{}'s.

Also OpenApiv3Schema: object for tekton charts had been missing, as well.
Most tekton CRD's are plain unstructured objects (ugh)

And instead

type PipelineProps struct {
	Metadata *cdk8s.ApiObjectMetadata `field:"optional" json:"metadata" yaml:"metadata"`
}

I'd like to get

type PipelineProps struct {
	Metadata *cdk8s.ApiObjectMetadata `field:"optional" json:"metadata" yaml:"metadata"`
	PipelineSpec *PipelineSpec `field:"optional" json:"metadata" yaml:"spec"`
}

type PipelineSpec struct {
  Schema *PipelineSchema  `field:"optional" json:"schema" yaml:"schema"`
}

type PipelineSchema struct {
  OpenApiv3Schema  interface{} `field:"optional" json:"openApiv3Schema" yaml:"openApiv3Schema"`
}

or similar...

It skips OpenApiv3Schema entirely for some reason.

So, I'm stuck with manual patching with CDK8S escape hatches.

I'm using latest

"devDependencies": {
    "cdk8s": "~2.7.77",
    "cdk8s-cli": "~2.2.51",
    "constructs": "~10.2.35",
    "jsii": "~5.1.0",
    "@jsii/runtime": "~1.82.0"
  }

with jsii-runtime-go@v1.82.0

@yuriy-yarosh
Copy link
Author

func ToMap[T any](obj T) map[string]interface{} {
	result := make(map[string]interface{})

	marshalled, _ := json.Marshal(obj)
	err := json.Unmarshal(marshalled, &result)
	if err != nil {
		log.Error().Err(err).Msg("can't convert to map[string]interface{}")
	}

	return result
}

does the trick, most of the time...

Copy link
Contributor

github-actions bot commented Jun 4, 2024

This issue has not received any attention in 1 year and will be closed soon. If you want to keep it open, please leave a comment below @mentioning a maintainer.

@github-actions github-actions bot added the closing-soon Issue/PR will be closing soon if no response is provided label Jun 4, 2024
@github-actions github-actions bot added closed-for-staleness Issue/PR was closed due to staleness and removed closing-soon Issue/PR will be closing soon if no response is provided labels Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working closed-for-staleness Issue/PR was closed due to staleness effort/small 1 day tops priority/p1 Should be on near term plans
Projects
None yet
Development

No branches or pull requests

3 participants