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

Allow JSON with unquoted field names by enabling system property #17801

Merged
merged 1 commit into from Apr 19, 2016

Conversation

Projects
None yet
5 participants
@dakrone
Copy link
Member

commented Apr 15, 2016

In Elasticsearch 5.0.0, by default unquoted field names in JSON will be
rejected. This can cause issues, however, for documents that were
already indexed with unquoted field names. To alleviate this, a system
property has been added that can be enabled so migration can occur.

This system property will be removed in Elasticsearch 6.0.0

Resolves #17674

@nik9000

View changes

core/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContent.java Outdated
jsonFactory.configure(JsonParser.Feature.ALLOW_UNQUOTED_FIELD_NAMES, false);
// TODO: Remove the system property configuration for this in Elasticsearch 6.0.0
jsonFactory.configure(JsonParser.Feature.ALLOW_UNQUOTED_FIELD_NAMES,
"true".equals(System.getProperty(JSON_ALLOW_UNQUOTED_FIELD_NAMES)) ? true : false);

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 15, 2016

Contributor

Probably don't need the ternary?

This comment has been minimized.

Copy link
@dakrone

dakrone Apr 15, 2016

Author Member

Not sure what you mean by not needing it?

This comment has been minimized.

Copy link
@jasontedor

jasontedor Apr 15, 2016

Member

I don't like leniency. Can it be "true", "false" or null with the former parsing to the right boolean and null giving the default? A typo of "tru" will parse to false and that makes me 😢.

This comment has been minimized.

Copy link
@dakrone

dakrone Apr 15, 2016

Author Member

"tru" parsing to false is less leniency, the value must be "true" and ONLY "true" to set this option. All other values are false

This comment has been minimized.

Copy link
@jasontedor

jasontedor Apr 15, 2016

Member

And I think that's bad because with "tru" the user meant "true" and the system silently swallowed their error. I want "true" -> true, "false" -> false and null -> false and all other values are rejected.

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 15, 2016

Contributor

I think that you could leave off the ternary and it'd be the same code. I think maybe Jason's point about tru means this should throw an exception or otherwise blow up if this isn't true or false?

This comment has been minimized.

Copy link
@javanna

javanna Apr 16, 2016

Member

can we use Booleans#parseBooleanExact?

This comment has been minimized.

Copy link
@jasontedor

jasontedor Apr 16, 2016

Member

No, that allows 0, off, and no for false, and 1, on, and yes for true.

This comment has been minimized.

Copy link
@javanna

javanna Apr 16, 2016

Member

oh I was hoping that was gone already. seems like parsing booleans is very complicated for us....

This comment has been minimized.

Copy link
@jasontedor

jasontedor Apr 16, 2016

Member

The worst is how on and no both parse to legitimate values, very dangerous for transposing typos.

#
# WARNING: This option will be removed in Elasticsearch 6.0.0 and is provided
# only for migration purposes.
#-Delasticsearch.json.allow_unquoted_field_names=true

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 15, 2016

Contributor

Is this the right place for this? I don't know where things go anymore but this feels like it should be another place? I dunno.

This comment has been minimized.

Copy link
@dakrone

dakrone Apr 15, 2016

Author Member

This is the correct place as of #17675

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 15, 2016

Contributor

Cool.

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2016

Left some comments but otherwise fine by me.

I suspect we can also have a unit test for this? Maybe it can fail if the current major version is 6?

@@ -115,6 +115,24 @@ setup() {
export ES_JAVA_OPTS=$es_java_opts
}

@test "[TAR]" start Elasticsearch with unquoted JSON option {

This comment has been minimized.

Copy link
@jasontedor

jasontedor Apr 15, 2016

Member

This is fine, I wanted to do something like this the other day too but ultimately passed on it. I think it shows a shortcoming of our rest tests. We need the ability to start up a node with arbitrary JVM options/settings, execute requests and have them match a pattern. Bashing them into the bats tests is the best we have right now.

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 15, 2016

Contributor

We can totally make a new qa project for this. smoke-test-ingest-disabled does that.

@jasontedor

This comment has been minimized.

Copy link
Member

commented Apr 15, 2016

I think this needs a note in the migration docs. Did the original issue that added the strict parsing on the JSON parser add a note? If so, can we add one along side that note? Otherwise, can we add a full note describing the issue and the solution here? Otherwise, looks great.

@jasontedor

This comment has been minimized.

Copy link
Member

commented Apr 16, 2016

I wonder if this should receive deprecation logging immediately out of the box in 5.0.0?

@dakrone

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2016

@jasontedor I pushed a couple of commits about making the option parsing extremely strict and extended the blurb in the migration guide about this

@jasontedor

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

Can we sprinkle some TODO in JsonXContent and Node? Otherwise, LGTM.

Allow JSON with unquoted field names by enabling system property
In Elasticsearch 5.0.0, by default unquoted field names in JSON will be
rejected. This can cause issues, however, for documents that were
already indexed with unquoted field names. To alleviate this, a system
property has been added that can be enabled so migration can occur.

This system property will be removed in Elasticsearch 6.0.0

Resolves #17674

@dakrone dakrone force-pushed the dakrone:allow-bad-json branch to a1e8fb7 Apr 19, 2016

@dakrone dakrone merged commit a1e8fb7 into elastic:master Apr 19, 2016

1 check passed

CLA Commit author has signed the CLA
Details

@dakrone dakrone deleted the dakrone:allow-bad-json branch Apr 19, 2016

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.