OSAC-1552: Reject creation when required editable field has no value

[tzvatot]

OSAC-1552: Reject creation when required editable field has no value

Jira: https://redhat.atlassian.net/browse/OSAC-1552

Summary

When a catalog item defines an editable field with no default value (e.g., pull_secret), and the user creates a cluster or compute instance without providing that field, the server now rejects the request immediately with InvalidArgument. Previously, the request was silently accepted and failed later during AAP provisioning with a confusing error.

Changes

• internal/servers/catalog_item_validation.go - In applyFieldDefinitions(), when an editable field has no default and the user provides no value, return InvalidArgument with a message identifying the missing field
• internal/servers/catalog_item_validation_test.go - Added 5 unit tests for applyFieldDefinitions covering: rejection of missing required fields, acceptance of user-provided values, default application, non-editable field override, and empty field definitions

Testing

• Unit tests: 5 new tests for applyFieldDefinitions behavioral contracts
• Integration tests: N/A - shared validation function, no component interaction changes
• Coverage: All new code paths covered through public interface tests

Acceptance Criteria

• Server rejects cluster/compute instance creation with InvalidArgument when a required editable field is missing
• Error message clearly identifies which field is missing
• Existing clusters with defaults or user-provided values are unaffected
• Unit tests cover: editable+no default+no user value (rejected), editable+no default+user provides value (accepted), editable+default+no user value (default applied)

Summary by CodeRabbit

• Bug Fixes
  • Improved validation to properly enforce required editable fields. Fields without default values now correctly return an error if not provided by the user, preventing silent failures.

[openshift-ci-robot]

@tzvatot: This pull request references OSAC-1552 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set.

Details

In response to this:

OSAC-1552: Reject creation when required editable field has no value

Jira: https://redhat.atlassian.net/browse/OSAC-1552

Summary

When a catalog item defines an editable field with no default value (e.g., pull_secret), and the user creates a cluster or compute instance without providing that field, the server now rejects the request immediately with InvalidArgument. Previously, the request was silently accepted and failed later during AAP provisioning with a confusing error.

Changes

• internal/servers/catalog_item_validation.go - In applyFieldDefinitions(), when an editable field has no default and the user provides no value, return InvalidArgument with a message identifying the missing field
• internal/servers/catalog_item_validation_test.go - Added 5 unit tests for applyFieldDefinitions covering: rejection of missing required fields, acceptance of user-provided values, default application, non-editable field override, and empty field definitions

Testing

• Unit tests: 5 new tests for applyFieldDefinitions behavioral contracts
• Integration tests: N/A - shared validation function, no component interaction changes
• Coverage: All new code paths covered through public interface tests

Acceptance Criteria

• Server rejects cluster/compute instance creation with InvalidArgument when a required editable field is missing
• Error message clearly identifies which field is missing
• Existing clusters with defaults or user-provided values are unaffected
• Unit tests cover: editable+no default+no user value (rejected), editable+no default+user provides value (accepted), editable+default+no user value (default applied)

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 9b67df92-05a9-474f-8983-4a3c0a71d7f3

📥 Commits

Reviewing files that changed from the base of the PR and between c05068c and 3062667.

📒 Files selected for processing (2)

• internal/servers/catalog_item_validation.go
• internal/servers/catalog_item_validation_test.go

---

Walkthrough

applyFieldDefinitions in catalog_item_validation.go gains a 4-line guard: when a field is editable, the user provided no value, and defaultVal is nil, the function now returns an InvalidArgument error instead of silently leaving the field unset. A new Ginkgo Describe block (99 lines) validates all branches of this logic.

Changes

Required-field enforcement in applyFieldDefinitions

Layer / File(s)

Summary

Required-field guard and test suite internal/servers/catalog_item_validation.go, internal/servers/catalog_item_validation_test.go

applyFieldDefinitions returns InvalidArgument when an editable field has no user-supplied value and no catalog default. The new Ginkgo suite covers rejection (missing value + no default), acceptance (user provides value), default application, non-editable field override, empty field definitions (nil), and multi-field missing error assertions including the field path pull_secret in the error message.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• osac-project/fulfillment-service#549: Modifies the same applyFieldDefinitions function in catalog_item_validation.go for editable field handling when user value and catalog default are both absent.

Suggested labels

lgtm

Suggested reviewers

• tzumainn
• akshaynadkarni

Poem

🔐 No default, no value? That's a CRITICAL gap!
The guard now fires before the field falls flat.
InvalidArgument raised — severity: high,
No silent nil escapes the validator's eye.
Editable fields must answer the call — supply a value, or fall. 🚨

🚥 Pre-merge checks | ✅ 11

✅ Passed checks (11 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: rejecting creation requests when required editable fields lack values, which matches the core logic modification in the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets	✅ Passed	No hardcoded secrets detected. All string values in the PR are test fixtures (my-secret, default-secret, admin-value, my-ssh-key) used only in unit test cases, not production credentials.
No-Weak-Crypto	✅ Passed	No weak cryptography patterns detected. PR contains only validation logic with no crypto operations, custom implementations, or insecure comparisons.
No-Injection-Vectors	✅ Passed	No injection vectors detected. Code uses safe error formatting (%s placeholders), no SQL/shell/eval/pickle/yaml operations, no unsafe deserialization, and field paths are safely processed via map n...
Container-Privileges	✅ Passed	PR modifies only Go source code files (catalog_item_validation.go and test file), not container/K8s manifests. Check for container privileges is not applicable.
No-Sensitive-Data-In-Logs	✅ Passed	PR logs only field names (e.g., 'pull_secret'), not sensitive values. Error messages at lines 102 and 118 reference the 'path' variable which contains field names from FieldDefinitions. Actual sens...
Ai-Attribution	✅ Passed	Commit properly attributes AI tool usage with "Assisted-by: Claude Code" and "Generated with" trailers; does not use Co-Authored-By for AI tools.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhernand, tzvatot

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [jhernand,tzvatot]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment