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

util/json: escape non-finite number values when marshaling #37331

Merged
merged 1 commit into from May 7, 2019

Conversation

Projects
None yet
3 participants
@mjibson
Copy link
Member

commented May 6, 2019

Since the resulting value is a string, not a number, it is unclear how
to test the roundtrip-ness of this, since it depends on the receiving
language. In JavaScript both of the following are true:

Infinity == "Infinity"
"Infinity" == Infinity

This explains why JSON didn't need to define +inf, -inf or NaN is special
values and instead leaves that up to the runtime. Thus for now we only
test that the output of this is parseable, not that it is equal, to
its input. This output matches postgres.

Fixes #37318

Release note (bug fix): correctly escape non-finite JSON number values
when marshaling to text.

@mjibson mjibson requested review from jordanlewis and justinj May 6, 2019

@mjibson mjibson requested a review from cockroachdb/sql-rest-prs as a code owner May 6, 2019

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented May 6, 2019

This change is Reviewable

@mjibson mjibson force-pushed the mjibson:jsonb branch from f1b1c7c to 8422bfa May 6, 2019

@jordanlewis
Copy link
Member

left a comment

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis, @justinj, and @mjibson)


pkg/sql/logictest/testdata/logic_test/json_builtins, line 594 at r1 (raw file):

# Regression for #37318
query T
SELECT jsonb_build_array('+Inf'::FLOAT8)::STRING::JSONB

Might be good to also have a NaN test?

util/json: escape non-finite number values when marshaling
Since the resulting value is a string, not a number, it is unclear how
to test the roundtrip-ness of this, since it depends on the receiving
language. In JavaScript both of the following are true:

`Infinity == "Infinity"`
`"Infinity" == Infinity`

This explains why JSON didn't need to define +inf, -inf or NaN is special
values and instead leaves that up to the runtime. Thus for now we only
test that the output of this is parseable, not that it is equal, to
its input. This output matches postgres.

Fixes #37318

Release note (bug fix): correctly escape non-finite JSON number values
when marshaling to text.

@mjibson mjibson force-pushed the mjibson:jsonb branch from 8422bfa to dea0313 May 7, 2019

@mjibson
Copy link
Member Author

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jordanlewis and @justinj)


pkg/sql/logictest/testdata/logic_test/json_builtins, line 594 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Might be good to also have a NaN test?

Done.

@mjibson

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

bors r+

craig bot pushed a commit that referenced this pull request May 7, 2019

Merge #36032 #37331 #37344
36032: sql: add a validation job status during schema validation r=vivekmenezes a=vivekmenezes

related to #32916

Release note: None

37331: util/json: escape non-finite number values when marshaling r=mjibson a=mjibson

Since the resulting value is a string, not a number, it is unclear how
to test the roundtrip-ness of this, since it depends on the receiving
language. In JavaScript both of the following are true:

`Infinity == "Infinity"`
`"Infinity" == Infinity`

This explains why JSON didn't need to define +inf, -inf or NaN is special
values and instead leaves that up to the runtime. Thus for now we only
test that the output of this is parseable, not that it is equal, to
its input. This output matches postgres.

Fixes #37318

Release note (bug fix): correctly escape non-finite JSON number values
when marshaling to text.

37344: *: ensure cluster-less RPC-related tests validate cluster IDs r=knz a=knz

Fixes  #35412.

When a test uses RPCs but does not set up a fully-fledged cluster, its
RPC context operates "without a cluster ID" throughout the test.

This creates potential for a flake if the test creates a server and
subsequently stops the server and expects further RPCs to that server
to fail: it is possible for a concurrent unrelated test to start a
server using the same TCP port as the server in the first test, in
which case the first test will then (unexpectedly, and incorrectly)
succeed in connecting to a server itself did not start. This failure
mode exists because without a valid cluster ID, the RPC context does
not validate what it's talking to.

The likelihood of TCP port reuse on a single server process is
relatively low, but given the frequency and sheer amount of tests that
start and stop servers, the probability we would observe such a flake
“in the wild” was asymptotically approaching 1, and indeed #35412 has
this scenario as likely cause.

This patch attempts to mitigate this situation by revisiting all the
tests that currently spawn a client RPC context without spawning a
cluster, in the cases where the test also checks the
availability/status/liveness of its server(s). On these tests, the
patch sets the cluster ID to be a random non-zero value.

Note that the previous fix to check expected node IDs is also a
partial mitigation, however most tests use node IDs starting from
1 (with most of them using just 1, or 1 and 2), and so the probability
that two unrelated tests running concurrently on the same machine
would run two server instances with the same node ID remains high. A
random cluster ID thus constitutes a more effective, definite mitigation.

Release note: None

Co-authored-by: Vivek Menezes <vivek@cockroachlabs.com>
Co-authored-by: Matt Jibson <matt.jibson@gmail.com>
Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
@craig

This comment has been minimized.

Copy link

commented May 7, 2019

Build succeeded

@craig craig bot merged commit dea0313 into cockroachdb:master May 7, 2019

2 of 3 checks passed

GitHub CI (Cockroach) TeamCity build started
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@mjibson mjibson deleted the mjibson:jsonb branch May 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.