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

Added JSON input validation for CLI commands #1771

Merged
merged 15 commits into from
Oct 11, 2024

Conversation

andrewnester
Copy link
Contributor

@andrewnester andrewnester commented Sep 13, 2024

Changes

Added JSON input validation for CLI commands. Now when invalid JSON passed as a payload to CLI commands, CLI performs input normalisation and detects if there are any mismatches such as incorrect types, unknown fields and etc.

This diagnostic information is printed in standard error output and does not block command execution, so the change is backward compatible.

This PR also addresses issue when values which are considered "empty" in Go (such as false, 0, "" and etc.) were ignored and not passed to API request payload.

Fixes #1769 #1764 #1625 #1560

Tests

Added unit tests

andrew.nester@HFW9Y94129 ~ % databricks jobs create --json '{"seeti}'
Error: error decoding JSON at (inline):1:2: unexpected EOF


andrew.nester@HFW9Y94129 ~ % databricks jobs create --json '{"seeti": true}'
Warning: unknown field: seeti
  in (inline):1:9

Error: Job settings must be specified.

libs/flags/json_flag.go Show resolved Hide resolved
libs/dyn/jsonloader/json.go Outdated Show resolved Hide resolved
@@ -33,7 +35,27 @@ func (j *JsonFlag) Unmarshal(v any) error {
if j.raw == nil {
return nil
}
return json.Unmarshal(j.raw, v)

dv, err := jsonloader.LoadJSON(j.raw)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do the same for the YAML flag?

I believe that's the only reason we still depend on github.com/ghodss/yaml. This package uses the json tags in the types as opposed to yaml tags, which the upstream YAML package uses. Since we use the json tags as well in the dyn package it should be possible to replace it.

Not blocking for this PR, of course.

@andrewnester andrewnester changed the title [WIP] Added JSON input validation for CLI commands Added JSON input validation for CLI commands Oct 3, 2024
libs/dyn/convert/to_typed.go Show resolved Hide resolved
libs/dyn/convert/to_typed.go Outdated Show resolved Hide resolved
libs/dyn/convert/to_typed.go Show resolved Hide resolved
libs/dyn/jsonloader/json_test.go Show resolved Hide resolved
libs/dyn/jsonloader/locations.go Outdated Show resolved Hide resolved
libs/dyn/jsonloader/locations.go Outdated Show resolved Hide resolved
libs/flags/json_flag.go Outdated Show resolved Hide resolved
libs/flags/json_flag.go Outdated Show resolved Hide resolved
libs/flags/json_flag_test.go Outdated Show resolved Hide resolved
libs/flags/json_flag.go Show resolved Hide resolved
libs/dyn/convert/to_typed.go Outdated Show resolved Hide resolved
libs/dyn/convert/to_typed.go Show resolved Hide resolved
libs/dyn/jsonloader/json.go Show resolved Hide resolved
libs/dyn/jsonloader/json_test.go Show resolved Hide resolved
libs/cmdio/render.go Outdated Show resolved Hide resolved
libs/dyn/convert/to_typed.go Show resolved Hide resolved
libs/dyn/jsonloader/json.go Show resolved Hide resolved
libs/dyn/jsonloader/json.go Show resolved Hide resolved
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, can you TAL at the PR summary before merging?

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.

CLI "invalid character" error message when issuing valid jobs create command
2 participants