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

Add Null check at JacksonAllInObjectScope to avoid NullPointerException #87

Merged
merged 1 commit into from
May 31, 2017

Conversation

sakama
Copy link
Contributor

@sakama sakama commented May 30, 2017

TODO

Changes

I was trying to fix ArrayIndexOutOfBoundsException that happens with embulk-output-elasticsearch at #86 and revised it.
I tried embulk/embulk#654 and got NullPointerException instead of ArrayIndexOutOfBoundsException.
This should valid behavior. Then, I added Null check to JacksonAllInObjectScope#{string|timestamp|json}Column to avoid this failure.

Stacktrace

Caused by: java.lang.NullPointerException
	at org.embulk.spi.time.TimestampFormatter.format(TimestampFormatter.java:93)
	at org.embulk.base.restclient.jackson.scope.JacksonAllInObjectScope$1.timestampColumn(JacksonAllInObjectScope.java:74)
	at org.embulk.spi.Column.visit(Column.java:60)
	at org.embulk.spi.Schema.visitColumns(Schema.java:81)
	at org.embulk.base.restclient.jackson.scope.JacksonAllInObjectScope.scopeObject(JacksonAllInObjectScope.java:36)
	at org.embulk.base.restclient.jackson.scope.JacksonObjectScopeBase.scopeEmbulkValues(JacksonObjectScopeBase.java:17)
	at org.embulk.base.restclient.jackson.scope.JacksonObjectScopeBase.scopeEmbulkValues(JacksonObjectScopeBase.java:9)
	at org.embulk.base.restclient.record.ValueExporter.exportValueToBuildRecord(ValueExporter.java:14)
	at org.embulk.base.restclient.record.RecordExporter.exportRecord(RecordExporter.java:18)
	at org.embulk.base.restclient.RestClientPageOutput.add(RestClientPageOutput.java:43)
	at org.embulk.exec.LocalExecutorPlugin$ScatterTransactionalPageOutput$OutputWorker.call(LocalExecutorPlugin.java:394)
	at org.embulk.exec.LocalExecutorPlugin$ScatterTransactionalPageOutput$OutputWorker.call(LocalExecutorPlugin.java:319)
	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)
	Suppressed: java.lang.NullPointerException
		... 16 more
	Suppressed: java.lang.NullPointerException
		... 16 more
	Suppressed: java.lang.NullPointerException
		... 16 more

@sakama
Copy link
Contributor Author

sakama commented May 30, 2017

Ah, this PR works without waiting embulk/embulk#654

No need to wait embulk/embulk#654

if (!singlePageRecordReader.isNull(column)) {
    resultObject.put(column.getName(),
                     singlePageRecordReader.getString(column));
}

Need to wait embulk/embulk#654

if (singlePageRecordReader.getString(column) != null) {
    resultObject.put(column.getName(),
                     singlePageRecordReader.getString(column));
}

@sakama sakama changed the title [WIP] Add Null check at JacksonAllInObjectScope to avoid NullPointerException Add Null check at JacksonAllInObjectScope to avoid NullPointerException May 30, 2017
@sakama sakama requested a review from dmikurube May 30, 2017 09:15
@sakama
Copy link
Contributor Author

sakama commented May 30, 2017

@dmikurube How do you think about?

Copy link
Member

@dmikurube dmikurube left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sakama Sorry to be late. lgtm!

We may have an option to add null JSON values there as below, but it's not trivial which is better. Starting from this sounds good to me.

"some_empty_value": null

@dmikurube
Copy link
Member

Merging this, and releasing a new version soon...

@dmikurube dmikurube merged commit 65256bf into master May 31, 2017
@dmikurube dmikurube mentioned this pull request May 31, 2017
@sakama sakama deleted the fix-null-pointer-exception branch May 31, 2017 07:14
@sakama
Copy link
Contributor Author

sakama commented May 31, 2017

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants