Fix processing & linking of alias types #6

coro · 2021-01-07T16:03:41Z

Prior to this change, alias types would be added to the processor.Types
map twice, in the case where an API defines both the alias type, and
another struct which uses either a pointer to, or an array / slice of,
an alias type. Depending on which element is processed first, the alias
type would be added to the map as an AliasKind, or a
SliceKind/ArrayKind/PointerKind. This means that where templates attempt
to render an underlying type of an Alias using IsAlias, the alias may
be registered as a non-alias type.

In addition, alias types were treated as Basic in the renderer if their
underlying types were basic. This means in the above case, links are not
generated to the alias type in the generated documentation, despite them
being a valid type as part of the API.

Here's an example API definition file that displays the problematic behaviour with the Plugin alias:

package v1beta1

import (
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

type ExampleFakeCluster struct {
	metav1.TypeMeta   `json:",inline"`
	metav1.ObjectMeta `json:"metadata,omitempty"`

	Plugins []Plugin `json:"plugins,omitempty"`
}

type Plugin string

// +kubebuilder:object:root=true

type ExampleFakeClusterList struct {
	metav1.TypeMeta `json:",inline"`
	metav1.ListMeta `json:"metadata,omitempty"`
	Items           []ExampleFakeCluster `json:"items"`
}

func init() {
	SchemeBuilder.Register(&ExampleFakeCluster{}, &ExampleFakeClusterList{})
}

Here is the diff in generated output:

--- "before.asciidoc"	2021-01-07 15:59:11.596313100 +0000
+++ "after.asciidoc"	2021-01-07 15:57:58.595765400 +0000
@@ -1,71 +1,71 @@
 // Generated documentation. Please do not edit.
 :anchor_prefix: k8s-api
 
 [id="{p}-api-reference"]
 == API Reference
 
 .Packages
 - xref:{anchor_prefix}-rabbitmq-com-v1beta1[$$rabbitmq.com/v1beta1$$]
 
 
 [id="{anchor_prefix}-rabbitmq-com-v1beta1"]
 === rabbitmq.com/v1beta1
 
 Package v1beta1 contains API Schema definitions for the rabbitmq v1beta1 API group
 
 .Resource Types
 - xref:{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-examplefakeclusterlist[$$ExampleFakeClusterList$$]
 
 
 
 [id="{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-examplefakecluster"]
 ==== ExampleFakeCluster 
 
 
 
 .Appears In:
 ****
 - xref:{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-examplefakeclusterlist[$$ExampleFakeClusterList$$]
 ****
 
 [cols="25a,75a", options="header"]
 |===
 | Field | Description
 | *`TypeMeta`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.20/#typemeta-v1-meta[$$TypeMeta$$]__ | 
 | *`metadata`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.20/#objectmeta-v1-meta[$$ObjectMeta$$]__ | Refer to Kubernetes API documentation for fields of `metadata`.
 
-| *`plugins`* __Plugin array__ | 
+| *`plugins`* __xref:{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-plugin[$$Plugin$$] array__ | 
 |===
 
 
 [id="{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-examplefakeclusterlist"]
 ==== ExampleFakeClusterList 
 
 
 
 
 
 [cols="25a,75a", options="header"]
 |===
 | Field | Description
 | *`apiVersion`* __string__ | `rabbitmq.com/v1beta1`
 | *`kind`* __string__ | `ExampleFakeClusterList`
 | *`TypeMeta`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.20/#typemeta-v1-meta[$$TypeMeta$$]__ | 
 | *`metadata`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.20/#listmeta-v1-meta[$$ListMeta$$]__ | Refer to Kubernetes API documentation for fields of `metadata`.
 
 | *`items`* __xref:{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-examplefakecluster[$$ExampleFakeCluster$$]__ | 
 |===
 
 
 [id="{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-plugin"]
-==== Plugin 
+==== Plugin (string) 
 
 
 
 .Appears In:
 ****
 - xref:{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-examplefakecluster[$$ExampleFakeCluster$$]
 ****

This also fixes another issue where types will be missing their description if they are only ever included in other structs directly, rather than as elements in arrays or pointed to. Here's a repro:

package v1beta1

import (
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

type ExampleFakeCluster struct {
	metav1.TypeMeta   `json:",inline"`
	metav1.ObjectMeta `json:"metadata,omitempty"`

	Array   []ArrayItemStruct `json:"array"`
	Pointer *PointeredStruct  `json:"pointer"`
	Field   FieldStruct       `json:"field"`
}

// The ArrayItemStruct is included in a array in the parent struct.
type ArrayItemStruct struct {
	Name string `json:"name"`
}

// The PointeredStruct is included only be address in a parent struct.
type PointeredStruct struct {
	Name string `json:"name"`
}

// The FieldStruct is directly included in a parent struct.
type FieldStruct struct {
	Name string `json:"name"`
}

// +kubebuilder:object:root=true

type ExampleFakeClusterList struct {
	metav1.TypeMeta `json:",inline"`
	metav1.ListMeta `json:"metadata,omitempty"`
	Items           []ExampleFakeCluster `json:"items"`
}

func init() {
	SchemeBuilder.Register(&ExampleFakeCluster{}, &ExampleFakeClusterList{})
}

--- "before.asciidoc"	2021-01-07 16:20:23.590964600 +0000
+++ "after.asciidoc"	2021-01-07 16:20:43.736994300 +0000
@@ -1,112 +1,112 @@
 // Generated documentation. Please do not edit.
 :anchor_prefix: k8s-api
 
 [id="{p}-api-reference"]
 == API Reference
 
 .Packages
 - xref:{anchor_prefix}-rabbitmq-com-v1beta1[$$rabbitmq.com/v1beta1$$]
 
 
 [id="{anchor_prefix}-rabbitmq-com-v1beta1"]
 === rabbitmq.com/v1beta1
 
 Package v1beta1 contains API Schema definitions for the rabbitmq v1beta1 API group
 
 .Resource Types
 - xref:{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-examplefakeclusterlist[$$ExampleFakeClusterList$$]
 
 
 
 [id="{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-arrayitemstruct"]
 ==== ArrayItemStruct 
 
-
+The ArrayItemStruct is included in a array in the parent struct.
 
 .Appears In:
 ****
 - xref:{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-examplefakecluster[$$ExampleFakeCluster$$]
 ****
 
 [cols="25a,75a", options="header"]
 |===
 | Field | Description
 | *`name`* __string__ | 
 |===
 
 
 [id="{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-examplefakecluster"]
 ==== ExampleFakeCluster 
 
 
 
 .Appears In:
 ****
 - xref:{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-examplefakeclusterlist[$$ExampleFakeClusterList$$]
 ****
 
 [cols="25a,75a", options="header"]
 |===
 | Field | Description
 | *`TypeMeta`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.20/#typemeta-v1-meta[$$TypeMeta$$]__ | 
 | *`metadata`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.20/#objectmeta-v1-meta[$$ObjectMeta$$]__ | Refer to Kubernetes API documentation for fields of `metadata`.
 
 | *`array`* __xref:{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-arrayitemstruct[$$ArrayItemStruct$$] array__ | 
 | *`pointer`* __xref:{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-pointeredstruct[$$PointeredStruct$$]__ | 
 | *`field`* __xref:{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-fieldstruct[$$FieldStruct$$]__ | 
 |===
 
 
 [id="{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-examplefakeclusterlist"]
 ==== ExampleFakeClusterList 
 
 
 
 
 
 [cols="25a,75a", options="header"]
 |===
 | Field | Description
 | *`apiVersion`* __string__ | `rabbitmq.com/v1beta1`
 | *`kind`* __string__ | `ExampleFakeClusterList`
 | *`TypeMeta`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.20/#typemeta-v1-meta[$$TypeMeta$$]__ | 
 | *`metadata`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.20/#listmeta-v1-meta[$$ListMeta$$]__ | Refer to Kubernetes API documentation for fields of `metadata`.
 
 | *`items`* __xref:{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-examplefakecluster[$$ExampleFakeCluster$$]__ | 
 |===
 
 
 [id="{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-fieldstruct"]
 ==== FieldStruct 
 
 The FieldStruct is directly included in a parent struct.
 
 .Appears In:
 ****
 - xref:{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-examplefakecluster[$$ExampleFakeCluster$$]
 ****
 
 [cols="25a,75a", options="header"]
 |===
 | Field | Description
 | *`name`* __string__ | 
 |===
 
 
 [id="{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-pointeredstruct"]
 ==== PointeredStruct 
 
-
+The PointeredStruct is included only be address in a parent struct.
 
 .Appears In:
 ****
 - xref:{anchor_prefix}-github-com-rabbitmq-cluster-operator-api-v1beta1-examplefakecluster[$$ExampleFakeCluster$$]
 ****
 
 [cols="25a,75a", options="header"]
 |===
 | Field | Description
 | *`name`* __string__ | 
 |===

Note that the descriptions of several included structs were previously missing.

Prior to this change, alias types would be added to the `processor.Types` map twice, in the case where an API defines both the alias type, and another struct which uses either a pointer to, or an array / slice of, an alias type. Depending on which element is processed first, the alias type would be added to the map as an AliasKind, or a SliceKind/ArrayKind/PointerKind. This means that where templates attempt to render an underlying type of an Alias using `IsAlias`, the alias may be registered as a non-alias type. In addition, alias types were treated as Basic in the renderer if their underlying types were basic. This means in the above case, links are not generated to the alias type in the generated documentation, despite them being a valid type as part of the API.

cla-checker-service · 2021-01-07T16:03:44Z

💚 CLA has been signed

coro · 2021-01-07T16:24:34Z

I believe I've signed the CLA now. Not sure how to re-trigger the check.

I've also updated the PR to include details of a second issue that this fixes in passing.

charith-elastic

Thank you for catching this problem and submitting a patch. It looks good to me. Before I merge it in, can you please fix test/expected.asciidoc. (You can run test.sh to run the test.)

coro · 2021-01-11T15:50:12Z

Apologies for missing that. I've updated the tests, and added a test for the case that this PR fixes.

charith-elastic

LGTM.

thbkrkr requested a review from charith-elastic January 7, 2021 16:37

charith-elastic reviewed Jan 11, 2021

View reviewed changes

Add & fix unit test for alias array

594a01a

charith-elastic approved these changes Jan 12, 2021

View reviewed changes

charith-elastic merged commit 851eb91 into elastic:master Jan 12, 2021

thbkrkr added the bug Something isn't working label Jan 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix processing & linking of alias types #6

Fix processing & linking of alias types #6

coro commented Jan 7, 2021 •

edited

Loading

cla-checker-service bot commented Jan 7, 2021 •

edited

Loading

coro commented Jan 7, 2021

charith-elastic left a comment

coro commented Jan 11, 2021

charith-elastic left a comment

Fix processing & linking of alias types #6

Fix processing & linking of alias types #6

Conversation

coro commented Jan 7, 2021 • edited Loading

cla-checker-service bot commented Jan 7, 2021 • edited Loading

coro commented Jan 7, 2021

charith-elastic left a comment

Choose a reason for hiding this comment

coro commented Jan 11, 2021

charith-elastic left a comment

Choose a reason for hiding this comment

coro commented Jan 7, 2021 •

edited

Loading

cla-checker-service bot commented Jan 7, 2021 •

edited

Loading