graphqlbackend: Introduce JSONCString scalar type

[mrnugget]

This is the first step towards implementing #6135.

In order to keep the pull requests and diffs as small was possible, I split this out, since it's generic and can already be merged.

I manually tested editing the site configuration, critical config in the management console, user and org settings. External service configuration is tested by e2e tests.

[codecov]

Codecov Report

Merging #6209 into master will increase coverage by 0.14%.
The diff coverage is 18.51%.

@@            Coverage Diff             @@
##           master    #6209      +/-   ##
==========================================
+ Coverage   39.18%   39.33%   +0.14%     
==========================================
  Files        1178     1170       -8     
  Lines       59520    58868     -652     
  Branches     5743     5743              
==========================================
- Hits        23324    23155     -169     
+ Misses      34051    33578     -473     
+ Partials     2145     2135      -10

Impacted Files	Coverage Δ
cmd/frontend/graphqlbackend/settings_mutation.go	44% <0%> (-0.45%)	⬇️
cmd/frontend/graphqlbackend/site.go	2.88% <0%> (-0.06%)	⬇️
cmd/frontend/graphqlbackend/discussion_threads.go	15.62% <0%> (ø)	⬆️
cmd/frontend/graphqlbackend/external_service.go	17.64% <0%> (ø)	⬆️
cmd/frontend/graphqlbackend/settings.go	22.22% <100%> (+2.99%)	⬆️
cmd/frontend/graphqlbackend/settings_subject.go	18.26% <100%> (ø)	⬆️
cmd/frontend/graphqlbackend/json.go	41.17% <20%> (-30.26%)	⬇️
cmd/frontend/internal/httpapi/internal.go	7% <0%> (-1.41%)	⬇️
cmd/repo-updater/repos/gitlab.go	74.85% <0%> (-1.15%)	⬇️
cmd/repo-updater/repoupdater/server.go	58.54% <0%> (-0.71%)	⬇️
... and 13 more

[keegancsmith]

You need a CHANGELOG entry.

This is technically a backwards incompatible change. I worry it may break existing customers who are sending up JSONC (ie to add an external service / add a config). The graphql query will break since the variable will be declared as a string instead of a JSONC scalar. Additionally some clients may break when parsing responses (but I find this less of a concern). cc @slimsag/@beyang any idea of a customer who is sending config to Sourcegraph via our GraphQL API?

[mrnugget]

This is technically a backwards incompatible change. I worry it may break existing customers who are sending up JSONC (ie to add an external service / add a config). The graphql query will break since the variable will be declared as a string instead of a JSONC scalar.

That's a good point.

@felixfbecker In light of this: how about we only use JSONCString for the new fields we introduce?

[keegancsmith]

If you want to smoothly transition you can make the old types take JSONC | string, and document that string is deprecated and will be removed in 1.12.

[mrnugget]

If you want to smoothly transition you can make the old types take JSONC | string, and document that string is deprecated and will be removed in 1.12.

I gave this a shot with this:

# A string or a JSONC string used to update settings or configuration.
union JSONInput = String | JSONCString

But apparently GraphQL doesn't support that: graphql/graphql-spec#215

So, depending on how keen @felixfbecker is on introducing this field across the Schema, I'd say we only use it for the new fields we add.

[felixfbecker]

We can still use it existing return types, right?

[mrnugget]

We can still use it existing return types, right?

I assume so, yeah. Should I do that then?

[mrnugget]

@felixfbecker @keegancsmith: Added a CHANGELOG entry and only changed the output from String to JSONCString

[felixfbecker]

✅