From 6a1617c4744ee19cdbdcad84a7e984d23d6eabac Mon Sep 17 00:00:00 2001
From: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Date: Fri, 7 Aug 2026 22:17:25 -0700
Subject: [PATCH] pkg/types: preserve qualified requirement text in field docs

Motivation:

Upjet-generated CRD field documentation strips Terraform argument
description text that clarifies when a field is actually required or
optional. getDescription() in pkg/types/field.go removed any
parenthesized text beginning with "(optional" or "(required"
(case-insensitive), not just the plain "(Optional)"/"(Required)"
markers Terraform docs use. For arguments whose requirement is
conditional, e.g. AWS RDS's username:

  (Required unless a snapshot_identifier or replicate_source_db is
  provided)

the entire parenthetical was discarded, leaving users with no
indication of the conditional requirement until they apply the
resource and it fails.

Approach:

Only strip a parenthetical when its trimmed, lowercased inner content
is exactly "optional" or "required". Plain markers such as
"(Optional)", "(required)", and "( Optional )" are still removed as
before; parentheticals carrying more specific requirement text are now
preserved verbatim in the generated field comment.

Validation:

Added a table-driven unit test, TestGetDescription, in the new
pkg/types/field_test.go covering a plain "(Optional)" marker, a plain
"(Required)" marker, a case-insensitive "(optional)" marker, and the
reported RDS username case. Confirmed the new ComplexRequirement case
fails against the pre-fix implementation (the whole parenthetical is
stripped) and passes against the fix.

Ran:
  go build ./...            # succeeds
  go test ./pkg/types/...   # all packages pass

This is a pure string-processing change confined to doc-comment
generation, so no e2e or dev-stack run was performed; none is required
by this repo's contribution guidelines for a change of this kind.

Report: https://github.com/crossplane/upjet/issues/457
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
---
 pkg/types/field.go      |  9 +++++--
 pkg/types/field_test.go | 52 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 2 deletions(-)
 create mode 100644 pkg/types/field_test.go

diff --git a/pkg/types/field.go b/pkg/types/field.go
index 079c60a9..3cff3ddb 100644
--- a/pkg/types/field.go
+++ b/pkg/types/field.go
@@ -468,10 +468,15 @@ func getDescription(s string) string {
 	// Remove dash
 	s = strings.TrimSpace(s)[strings.Index(s, "-")+1:]
 
-	// Remove 'Reqiured' || 'Optional' information
+	// Remove plain '(Required)' || '(Optional)' markers, but keep
+	// parenthesized content that further qualifies the specification
+	// requirement, e.g. "(Required unless a snapshot_identifier or
+	// replicate_source_db is provided)", since that information is not
+	// otherwise conveyed to the user.
 	matches := parentheses.FindAllString(s, -1)
 	for _, m := range matches {
-		if strings.HasPrefix(strings.ToLower(m), "(optional") || strings.HasPrefix(strings.ToLower(m), "(required") {
+		inner := strings.ToLower(strings.TrimSpace(strings.Trim(m, "()")))
+		if inner == "optional" || inner == "required" {
 			s = strings.ReplaceAll(s, m, "")
 		}
 	}
diff --git a/pkg/types/field_test.go b/pkg/types/field_test.go
new file mode 100644
index 00000000..ca6480d4
--- /dev/null
+++ b/pkg/types/field_test.go
@@ -0,0 +1,52 @@
+// SPDX-FileCopyrightText: 2023 The Crossplane Authors <https://crossplane.io>
+//
+// SPDX-License-Identifier: Apache-2.0
+
+package types
+
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+)
+
+func TestGetDescription(t *testing.T) {
+	cases := map[string]struct {
+		reason string
+		arg    string
+		want   string
+	}{
+		"PlainOptional": {
+			reason: "A simple '(Optional)' marker should be stripped entirely.",
+			arg:    "timezone - (Optional) Time zone of the DB instance.",
+			want:   "Time zone of the DB instance.",
+		},
+		"PlainRequired": {
+			reason: "A simple '(Required)' marker should be stripped entirely.",
+			arg:    "name - (Required) Name of the resource.",
+			want:   "Name of the resource.",
+		},
+		"ComplexRequirement": {
+			reason: "A parenthesized requirement that is more specific than " +
+				"'Optional'/'Required' must be preserved, since it conveys " +
+				"information not otherwise available to the user.",
+			arg: "username - (Required unless a snapshot_identifier or replicate_source_db is provided) " +
+				"Username for the master DB user. Cannot be specified for a replica.",
+			want: "(Required unless a snapshot_identifier or replicate_source_db is provided) " +
+				"Username for the master DB user. Cannot be specified for a replica.",
+		},
+		"CaseInsensitiveOptional": {
+			reason: "The '(optional)' marker check must be case-insensitive.",
+			arg:    "field - (optional) Some description.",
+			want:   "Some description.",
+		},
+	}
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			got := getDescription(tc.arg)
+			if diff := cmp.Diff(tc.want, got); diff != "" {
+				t.Errorf("\n%s\ngetDescription(...): -want, +got:\n%s", tc.reason, diff)
+			}
+		})
+	}
+}