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

Missing validation in INSERT JSON of simple_date, time, uuid, inet_addr #10115

Closed
avelanarius opened this issue Feb 21, 2022 · 2 comments · Fixed by #10101
Closed

Missing validation in INSERT JSON of simple_date, time, uuid, inet_addr #10115

avelanarius opened this issue Feb 21, 2022 · 2 comments · Fixed by #10101
Milestone

Comments

@avelanarius
Copy link
Member

Scylla version (or git commit hash): f292d3d

When using INSERT JSON and inserting values to columns with types represented by simple_date_type_impl, time_type_impl, uuid_type_impl, inet_addr_type_impl, you are supposed to specify those values as JSON string values. However, this condition is not validated:

cqlsh:ks> CREATE TABLE t(pk int primary key, v uuid);
cqlsh:ks> INSERT INTO t JSON '{"pk": 1, "v": 123.456}';
ServerError: JSON error: condition not met: IsString()

The error is not that bad, but it doesn't originate from Scylla code, but from RapidJSON assertion. The error message does not include which value caused the problem, which makes debugging the problem more difficult.

@nyh
Copy link
Contributor

nyh commented Feb 21, 2022

Good catch. I believe I don't have a test for this yet in test_json.py (as if there aren't already enough open bugs listed there ;-)).

A small comment: In Alternator, we catch those RapidJSON errors (this "JSON error" here) and wrap them into a syntax error response - so the application knows the problem was an error in the query so it is pointless to retry it (even if the string of the message is not too descriptive), not some sort of internal server error which can be retried.

But you're right that it's even better to print a special error in each case (e.g., here say that "uuid must be a string" or something like that). The generic JSON-error-into-syntax-error thing is just in case we forgot those special error messages.

nyh added a commit that referenced this issue Feb 22, 2022
…otr Grabowski

Add support for specifing integers in scientific format (for example 1.234e8) in INSERT JSON statement:

```
INSERT INTO table JSON '{"int_column": 1e7}';
```

Before the JSON parsing library was switched to RapidJSON from JsonCpp, this statement used to work correctly, because JsonCpp transparently casts double to integer value.

Inserting a floating-point number ending with .0 is allowed, as the fractional part is zero. Non-zero fractional part (for example 12.34) is disallowed. A new test is added to test all those behaviors.

This behavior differs from Cassandra, which disallows those types of numbers (1e7, 123.0 and 12.34), however some users rely on that behavior and JSON specification itself does not distinct between floating-point numbers and integer numbers (only a single "number" type is defined).

This PR also fixes two minor issues I noticed while looking at the code: wrong blob validation and missing `IsString()` checks that could result in assertion error.

Fixes #10100
Fixes #10114
Fixes #10115

Closes #10101

* github.com:scylladb/scylla:
  type_json: support integers in scientific format
  type_json: add missing IsString() checks
  type_json: fix wrong blob JSON validation
xemul pushed a commit that referenced this issue Feb 22, 2022
…otr Grabowski

Add support for specifing integers in scientific format (for example 1.234e8) in INSERT JSON statement:

```
INSERT INTO table JSON '{"int_column": 1e7}';
```

Before the JSON parsing library was switched to RapidJSON from JsonCpp, this statement used to work correctly, because JsonCpp transparently casts double to integer value.

Inserting a floating-point number ending with .0 is allowed, as the fractional part is zero. Non-zero fractional part (for example 12.34) is disallowed. A new test is added to test all those behaviors.

This behavior differs from Cassandra, which disallows those types of numbers (1e7, 123.0 and 12.34), however some users rely on that behavior and JSON specification itself does not distinct between floating-point numbers and integer numbers (only a single "number" type is defined).

This PR also fixes two minor issues I noticed while looking at the code: wrong blob validation and missing `IsString()` checks that could result in assertion error.

Fixes #10100
Fixes #10114
Fixes #10115

Closes #10101

* github.com:scylladb/scylla:
  type_json: support integers in scientific format
  type_json: add missing IsString() checks
  type_json: fix wrong blob JSON validation
@avikivity
Copy link
Member

Not backporting. The fix changes a bad error message to a good error message, so it's not something that people usually encounter.

@DoronArazii DoronArazii added this to the 5.1 milestone May 7, 2023
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 a pull request may close this issue.

5 participants