DbClient and Webserver - wrong result on JSON-P when library missing

[tomas-langer]

Environment Details

• Helidon Version: 2.0.0

• Helidon SE

Problem Description

When the following dependency is not on the classpath

<dependency>
        <groupId>io.helidon.dbclient</groupId>
        <artifactId>helidon-dbclient-jsonp</artifactId>
</dependency>

and Json processing for WebServer is on the classpath, calling

Multi<JsonObject> dbMulti = dbClient.execute(exec -> exec.namedQuery("select-all-pokemons"))
                .map(it -> it.as(JsonObject.class);
response.send(dbMulti, JsonObject.class);

Fails and only returns [, instead of a usual exception. This should fail, as DbClient cannot find the mapper to JsonObject for rows.

Reproducer

Go to Pokemon example in dbclient and remove the dependency from pom.xml. Then try to get list of Pokemon.

[romain-grecourt]

I'm getting [ even when the helidon-dbclient-jsonp in the classpath.

[romain-grecourt]

JsonpBodyStreamWriter is implemented like this:

Single.just(DataChunk.create(ARRAY_JSON_BEGIN_BYTES))
    .onCompleteResumeWith(Multi.create(publisher)
        .map(jsonToChunks)
        .flatMap(it -> {
            if (first.getAndSet(false)) {
                // first record, do not prepend a comma
                return Single.just(it);
            } else {
                // any subsequent record starts with a comma
                return Multi.just(DataChunk.create(COMMA_BYTES), it);
            }
        }))
    .onCompleteResume(DataChunk.create(ARRAY_JSON_END_BYTES));

If the upstream publisher holds an error, it is not propagated and it produces a stream that contains just [.

Single.just("[")
    .onCompleteResumeWith(Single.error(new Exception("booh!")))
    .onCompleteResume("]")
     .forEach(System.out::println)
// prints '['

[romain-grecourt]

The example requires a database to be running, if the database is not running the publisher holds an exception:

io.helidon.dbclient.DbClientException: Failed to create a connection to jdbc:oracle:thin:@localhost:1521

If the helidon-dbclient-jsonp dependency is missing, the publisher holds an exception:

io.helidon.common.mapper.MapperException: Failed to map interface io.helidon.dbclient.DbRow to interface javax.json.JsonObject: Failed to find DB mapper.

[danielkec]

Well its a stream, you can get correct item n-times and then receive the error signal. There is no bug here in the stream, its just un-handled error signal, this will end up in the same manner, because stream is prefixed with item [ by JsonStreamWriter:

res.send(Multi.error(new Exception("booh!")), JsonObject.class);

Question is how should we behave with http status code 200 already sent. I believe status code is not one of the headers allowed in the trailer(not sure about that), so only way to signal the client something is wrong is custom trailer header(ignored by the browser) or unexpectedly close the connection.

[romain-grecourt]

Response headers and status are sent before the first item is received from the stream. If the stream does not produce any item but holds an error, it should be possible to propagate the error correctly.

I.e, Can we change the implementation of JsonStreamWriter to use the publisher from dbclient as the first publisher in the pipeline ? I'm assuming this would allow us to signal onError and zero item.

If this change can be implemented, we still have to handle the error, currently if one sends a publisher with an error it produces an empty response:

response.send(Multi.error(new Exception("boo!")));

I think BareResponseImpl (the subscriber) can be updated to detect that an onError signal is received and the headers are not sent. Perhaps we can also delegate the error handling to the routing error handlers.

[danielkec]

Problem goes even deeper, there is no guarantee that first item/error/complete will arrive immediately. And because of reactive streams needs to assure serial execution of onSubscribe->onNext->onError, there is no way to detect "immediate" arrival of anything after onSubscribe with one thread(assuming waiting for timeout in different thread is too expensive).

So there are basically three options:

1. Send headers with the first subsequent signal after onSubscribe

   • means risking timeout with Empty reply from server
   • allows sending status 500 in case onError signal as first signal after onSubscribe

2. Send headers with the first flush

   • means risking timeout with Empty reply from server
   • allows sending status 500 in case onError signal arrives before first flush
   • allows fixing issue with JsonStreamWriter by making it possible to drive the mechanism by flushing eg. prepended item is not flushed while all the other are, see #2129

3. Send headers with onSubscribe signal

   • means being stuck with current problem(always status 200)
   • should assure Empty reply from server doesn't happen

Personally I am for number 2. as it gives us a way to control the thing from higher layers of code.

[romain-grecourt]

The change required for 2 to send the headers with the first flush feels a little uncertain, isn't it a big change compared to the current implementation ?

I'd vote for 1 as it doesn't require as many changes as 2.

[danielkec]

We are going for option 1. enhanced with custom trailing header for subsequent error

[danielkec]

Fixed in #2129 https://github.com/oracle/helidon/pull/2129#issuecomment-656208308