search

embulk / embulk-base-restclient

Base class library for Embulk plugins to access RESTful services
https://www.embulk.org/
Apache License 2.0
6 stars 7 forks source link

Set NullNode when JsonNode value in record is null #55

Closed sakama closed 7 years ago

sakama commented 7 years ago

I got following failure and fixed it. This failure happens because Web API doesn't returns record itself when its value is null.

Models

private JacksonServiceResponseMapper model = JacksonServiceResponseMapper.builder()
            .add("id", Types.STRING)
            .add("column_abcdefg", Types.BOOLEAN)
            .build();

API response

{
  "id": "aaa_1234567"
   # API doesn't returns 'column_abcdefg' record itself when its value is null
}

Stacktrace

Caused by: org.embulk.spi.DataException: Failed to import a value for column: column_abcdefg (boolean)
    at org.embulk.base.restclient.record.values.BooleanValueImporter.findAndImportValue(BooleanValueImporter.java:40)
    at org.embulk.base.restclient.record.RecordImporter.importRecord(RecordImporter.java:17)
    at org.embulk.input.xxxxx.xxxxxInputPluginDelegate.ingestServiceData(xxxxxInputPluginDelegate.java:389)
    at org.embulk.input.xxxxx.xxxxxInputPluginDelegate.ingestServiceData(xxxxxInputPluginDelegate.java:65)
    at org.embulk.base.restclient.RestClientInputPluginBaseUnsafe.run(RestClientInputPluginBaseUnsafe.java:125)
    at org.embulk.base.restclient.RestClientInputPluginBase.run(RestClientInputPluginBase.java:123)
    at org.embulk.exec.LocalExecutorPlugin$ScatterExecutor.runInputTask(LocalExecutorPlugin.java:294)
    at org.embulk.exec.LocalExecutorPlugin$ScatterExecutor.access$000(LocalExecutorPlugin.java:212)
    at org.embulk.exec.LocalExecutorPlugin$ScatterExecutor$1.call(LocalExecutorPlugin.java:257)
    at org.embulk.exec.LocalExecutorPlugin$ScatterExecutor$1.call(LocalExecutorPlugin.java:253)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.NullPointerException
    at org.embulk.base.restclient.jackson.JacksonServiceValue.isNull(JacksonServiceValue.java:41)
    at org.embulk.base.restclient.record.values.BooleanValueImporter.findAndImportValue(BooleanValueImporter.java:29)
    ... 13 more

I found that BooleanValueImport.findAndImportValue() has null check logic, but always returns false and JacksonServiceValue.isNull() throws NullPointerException.

// this value type is 'ServiceValue'
// 'value==null' always returns false
// Then 'value.isNull()' throws NullPointerException
if (value == null || value.isNull()) {
    pageBuilder.setNull(getColumnToImport());
}

BooleanValueImport.java

So I added additional null check logic to JacksonServiceValue.isNull() method.

dmikurube commented 7 years ago

@sakama Thanks!

Looked into that part again, and finally I thought it could be better to guarantee that JacksonServiceValue does not contain Java's null directly. (Sorry to change my mind.)

What do you think to have the following in the constructor?

        if (value == null) {
            this.value = new NullNode();
        } else {
            this.value = value;
        }

https://github.com/embulk/embulk-base-restclient/blob/ac76108004e586608ee9fd98cd53b95e26ee8f20/src/main/java/org/embulk/base/restclient/jackson/JacksonServiceValue.java#L32-L35

sakama commented 7 years ago

@dmikurube Looks good. It's much better than null check.

Minor comment but NullNode() has private access. So NullNode.getInstance() instead of new NullNode()?

Above one works fine with my model&data.

And I added commits ac384ad.