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

Use struct tags to aid in static validation #43

Merged
merged 5 commits into from
Apr 5, 2024

Conversation

mbarnes
Copy link
Collaborator

@mbarnes mbarnes commented Apr 5, 2024

What this PR does

This adds a dependency on github.com/go-playground/validator, which is an interesting way to implement static validation through struct tags.

This doesn't fully cover static validation, but helps remove some of the potential tedium of hand-written validation checks.

This also beautifies cloud error responses due to validation errors. Here's an example from some test data:

$ curl -X PUT http://localhost:8443/subscriptions/SUBSCRIPTION/resourcegroups/RESOURCEGROUP/providers/Microsoft.RedHatOpenshift/hcpOpenShiftClusters/mbarnestest?api-version=2024-06-10-preview --json @cluster.json
{
    "error": {
        "code": "InvalidRequestContent",
        "message": "Content validation failed on one or more fields",
        "details": [
            {
                "code": "InvalidRequestContent",
                "message": "Invalid value 'invalid' for field 'provisioningState' (must be one of: Canceled Failed Succeeded)",
                "target": "properties.provisioningState"
            },
            {
                "code": "InvalidRequestContent",
                "message": "Invalid value 'this_is_not_a_cidr_address' for field 'podCidr' (must be a v4 CIDR address)",
                "target": "properties.spec.network.podCidr"
            },
            {
                "code": "InvalidRequestContent",
                "message": "Invalid value 'private_eyes_they're_watching_you' for field 'visibility' (must be one of: private public)",
                "target": "properties.spec.api.visibility"
            },
            {
                "code": "InvalidRequestContent",
                "message": "Invalid value 'this_is_not_a_url' for field 'issuerUrl' (must be a URL)",
                "target": "properties.spec.issuerUrl"
            }
        ]
    }
}

Jira: ARO-5736: Create static validation for frontend API Structs

(I started working on ARO-6347: Validate metadata from api structs but this ended up falling more into ARO-5736)

Special notes for your reviewer

Please read the individual commit messages, some of which have additional detail.

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

  • PR: The PR description is expressive enough and will help future contributors
  • Code: Write code that humans can understand and Keep it simple
  • Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
  • Upgrade: Impact of this change on upgrade flows was considered and addressed if required
  • Deployment: The deployment process was considered and addressed if required
  • Testing: New code requires new unit tests.
  • Documentation: Is the documentation updated? Either in the doc located in focus area, in the README or in the code itself.
  • Customers: Is this change affecting customers? Is the release plan considered?

Matthew Barnes added 4 commits April 5, 2024 10:24
This will aid in static validation by adding validation tags to
internal API structs.

Note, we have to be careful with this.  We cannot add validation
tags to versioned API structs because they are generated, so this
validation method is limited to things that will not change from
version to version.

For example string fields that expect a standardized syntax like
a URL or CIDR address can be tagged for validation.
An enum field will always be an enum field, but the set of valid
values may increase over future versions.

To account for this, each API version will own its own Validate
instance and register a custom alias for each enum type under a
well-known tag name.

So, for example, if an internal struct field is tagged with:

  validate:"enum_foo"

Then one API version can register the alias "enum_foo" as:

  "oneof=a b c"

And a subsequent API version could register the same alias as:

  "oneof=a b c d"

This change also reverts internal enum fields back to string types,
such that Normalize() methods will no longer fail but enum fields
may temporarily hold an invalid enum value prior to validation.
Writes an appropriate CloudError for JSON unmarshaling or static
validation errors to the given ResponseWriter.
s-amann
s-amann previously approved these changes Apr 5, 2024
mjlshen
mjlshen previously approved these changes Apr 5, 2024
@mbarnes mbarnes dismissed stale reviews from mjlshen and s-amann via 3385c7c April 5, 2024 17:19
@mjlshen mjlshen merged commit 2f99d3b into Azure:main Apr 5, 2024
3 checks passed
@mbarnes mbarnes deleted the tag-validation branch April 5, 2024 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants