elastic / elasticsearch

Free and Open Source, Distributed, RESTful Search Engine
https://www.elastic.co/products/elasticsearch
Other
1.26k stars 24.86k forks source link

XContentParser Behaves Differently for Stream and Byte Array #61489

Open original-brownbear opened 4 years ago

original-brownbear commented 4 years ago

Optimizing BytesReference parsing in #61447 revealed a subtle bug in x-content parsing.

The following test:

    public void testIncompatibleStreamTypes() throws Exception {
        final Map<String, Object> map = Map.of("foo", "bar");
        final BytesReference jsonBytes = BytesReference.bytes(JsonXContent.contentBuilder().map(map));
        final Map<String, Object> deserialized = SmileXContent.smileXContent.createParser(
                NamedXContentRegistry.EMPTY, DeprecationHandler.IGNORE_DEPRECATIONS, jsonBytes.streamInput()).map();
        assertEquals(deserialized, Collections.emptyMap());

        final BytesRef bytes = jsonBytes.toBytesRef();
        final Map<String, Object> deserialized2 = SmileXContent.smileXContent.createParser(
                NamedXContentRegistry.EMPTY, DeprecationHandler.IGNORE_DEPRECATIONS, bytes.bytes, bytes.offset, bytes.length).map(); // this throws
    }

will actually pass until the last line which will throw:

com.fasterxml.jackson.core.JsonParseException: Input does not start with Smile format header (first byte = 0x7b) -- rather, it starts with '{' (plain JSON input?) -- can not parse
 at [Source: (byte[])"{"foo":"bar"}"; line: -1, column: 0]

So for reading from streams, we simply read empty map when the type is off but when reading from a byte array we (in my opinion correctly) throw an exception. I think we should throw that same exception when reading from a stream of the wrong content type shouldn't we?

elasticmachine commented 4 years ago

Pinging @elastic/es-core-infra (:Core/Infra/Core)

original-brownbear commented 4 years ago

Relates https://github.com/elastic/elasticsearch/pull/61485 which was caused by this subtle difference where it resulted in some pointless test.

rjernst commented 4 years ago

+1 to a consistent error

rjernst commented 3 years ago

I'm adding the help wanted since this is still an issue, and should be relatively easy to fix, but has not been a priority since it is a minor inconsistency.

Kiriakos1998 commented 1 year ago

Hello @rjernst @jaymode @original-brownbear , can I work on this issue?