* standardize jsonc parsing, unmarshaling, and normalization on empty documents
Empty JSONC documents that had whitespace or comments were treated differently by
- jsonc.Normalize, which converted them to `{}` (an object)
- jsonc.Parse, which returned "" (which is invalid JSON)
- jsonc.Unmarshal, which silently returned without doing anything
Now, they all treat these empty JSONC documents the same (as though the JSONC document was `null`, i.e. JSON null, which is a valid JSON document).
Fixes https://github.com/sourcegraph/sourcegraph/issues/42000 because the validation (which used the jsonc.Normalize code path) now reports an error due to `null` not conforming to the JSON Schema (which expects the root to be an object).
* show nicer error message when attempting to save empty settings
User settings (and all other JSON configuration input) must be valid JSONC objects. An empty JSONC document, or one that consists solely of whitespace or comments, is converted to JSON `null`.
The previous commit fixed https://github.com/sourcegraph/sourcegraph/issues/42000. This commit presents a nicer error for that case: instead of `invalid settings: (root): Invalid type. Expected: object, given: null`, the user now sees `invalid settings: must be a JSON object (use {} for empty)`.
* remove confusing error return from JSON Schema validation functions
It would be easy for callers to assume a nil error means no problems. That assumption would be false, and it could lead to callers unintentionally accepting invalid JSON.
This removal of the error return makes the API impossible to use incorrectly.
* remove easy-to-misuse jsonc.Normalize
Our settings, site config, and external service configs are stored in the "JSONC" format, which is like JSON but with comments and trailing commas allowed. All code paths that actually read and write this format to/from the DB enforce "JSONC".
However, when validating settings and site config (but not external service config), we used an even more lenient format, which you might call "JSONC that skips past syntax errors". It allowed things like `{"a": 1, b}` (note the unquoted object key with no value).
The jsonc package confusingly supported both formats:
- jsonc.Parse and jsonc.Unmarshal supported "JSONC".
- jsonc.Normalize supported "JSONC that skips past syntax errors".
Why did we ever support "JSONC that skips past syntax errors"? I believe I implicitly made the decision to "support" it by not building a better, clearer API. (Also, VS Code supports it in its own JSON settings files. But it has a better reason for supporting it: you can edit those files to introduce syntax errors on your own disk using other editors. We, on the other hand, can prevent edits that are syntactically invalid.)
This commit remedies that past bad decision and removes jsonc.Normalize. We no longer support "JSONC that skips past syntax errors" anywhere. Because this lenient format was only used when producing validation notices, and not when actually reading/writing configuration in ways that have side effects, this should not cause problems.
The sole expected behavior difference is that if Sourcegraph is attempted to be run with syntactically invalid configuration (e.g., via `GLOBAL_SETTINGS_FILE`), then the error message printed will just report the syntax error and not other errors that can be ascertained if the syntax error is ignored. This behavior is actually preferable because the syntax error should be fixed first anyway.
Co-authored-by: Milan Freml <kopancek@users.noreply.github.com>
* standardize jsonc parsing, unmarshaling, and normalization on empty documents
Empty JSONC documents that had whitespace or comments were treated differently by
- jsonc.Normalize, which converted them to `{}` (an object)
- jsonc.Parse, which returned "" (which is invalid JSON)
- jsonc.Unmarshal, which silently returned without doing anything
Now, they all treat these empty JSONC documents the same (as though the JSONC document was `null`, i.e. JSON null, which is a valid JSON document).
Fixes https://github.com/sourcegraph/sourcegraph/issues/42000 because the validation (which used the jsonc.Normalize code path) now reports an error due to `null` not conforming to the JSON Schema (which expects the root to be an object).
* show nicer error message when attempting to save empty settings
User settings (and all other JSON configuration input) must be valid JSONC objects. An empty JSONC document, or one that consists solely of whitespace or comments, is converted to JSON `null`.
The previous commit fixed https://github.com/sourcegraph/sourcegraph/issues/42000. This commit presents a nicer error for that case: instead of `invalid settings: (root): Invalid type. Expected: object, given: null`, the user now sees `invalid settings: must be a JSON object (use {} for empty)`.
Now that we require go1.18, we can use a builtin type alias for
"interface{}": "any". I've updated all code except lib, which can be
imported by external modules. That is currently pinned at go1.16.
Additionally I had to rerun generate since rewriting generated go code
will fail CI.
find -name '*.go' | xargs gofmt -s -r 'interface{} -> any' -w
git checkout lib
go generate ./...
Test Plan: CI will exercise that the code still compiles. Otherwise this
is a noop.
Previously we were only redacting the secret key.
This change also alters our redaction code so that we can deal with keys
that are more than one level deep and updates some comments.
In this commit, along with moving to
github.com/cockroachdb/errors, we update a few tests and some
error checks in non-test code.
List of files that have code changes apart from changes in the
error package import:
enterprise/internal/batches/sources/gitlab.go
enterprise/internal/batches/sources/gitlab_test.go
internal/conf/client.go
internal/store/store_test.go
Co-authored-by: ᴜɴᴋɴᴡᴏɴ <joe@sourcegraph.com>
