Fixing the null-unsafe bug in JsonPrinter #10365

[landreev]

What this PR does / why we need it:

This was a simple bug in jsonprinter, recently merged (#10322). This is killing the /addFiles api that the Direct Upload and Globus apis rely on.

Which issue(s) this PR closes:

Closes #10365

Special notes for your reviewer:

trivial?

Suggestions on how to test this:

As of now, an attempt to use DvUploader in direct upload mode is failing when used w/ the develop branch build, with the following error message:

Error response when processing files in Individual files on command line : Bad Request
{"status":"ERROR","message":"Cannot invoke \"java.lang.Long.longValue()\" because the return value of \"edu.harvard.iq.dataverse.FileMetadata.getVersion()\" is null"}

Should be working again with this fix applied.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

Is there a release notes update needed for this change?:

Additional documentation:

[pdurbin]

I didn't test this but the fix looks good.

[github-actions]

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:10365-jsonprinter-nullsafejsonbuilder

ghcr.io/gdcc/configbaker:10365-jsonprinter-nullsafejsonbuilder

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.

[qqmyers]

I don't think that can be it. Instead, I think the problem is that https://github.com/IQSS/dataverse/blame/6a26e6315834a7f26de2c00ec78df7752feccd5e/src/main/java/edu/harvard/iq/dataverse/util/json/JsonPrinter.java#L419 doesn't check for a null dsv. That's called by the new line at https://github.com/IQSS/dataverse/blame/6a26e6315834a7f26de2c00ec78df7752feccd5e/src/main/java/edu/harvard/iq/dataverse/util/json/JsonPrinter.java#L678. W.r.t. to the casting of the Null builder - the cast is just to an interface, there is no underlying class that has an add method, and we use this cast elsewhere w/o problem.

[landreev]

Hmm. The change above does fix it though.

doesn't check for a null dsv

note that it's not a dsv in the null exception, it's FileMetadata.getVersion() (the Postgres object version)
Also, yes, it is an interface - but see the method definition in the interface:

public JsonObjectBuilder add(String string, long l);

i.e., it's long, not Long, hence the exception:

"Cannot invoke \"java.lang.Long.longValue()\" because the return value of \"edu.harvard.iq.dataverse.FileMetadata.getVersion()\" is null"

(and NullSafeJsonBuilder provides a Long version of the method, in addition to the long that immediately implements the interface; and that's why it works!)

[landreev]

(as always, corrected some typos in the last comment after the fact)

[qqmyers]

Ah - I see. I was looking at the wrong version method. The interface doesn't have a method with Long, hence this issue.

So it seems any of the calls in this whole class where a null Long after that cast would fail.

As would any call with null values for int, double, boolean - as the NullSafeJsonBuilder doesn't have methods equivalent to the add Long method for Integer, Double, or Boolean?

Is it worth fixing or noting these somewhere?

[landreev]

I did notice that there were a few (but only a few) places in the class where NullSafeJsonBuilder was explicitly used in declarations. Could have been for this very reason?
It would make sense to check and see if there are other places where this could happen... But since this PR was meant as a quick fix for this specific issue, it may be prudent to keep it this way...
But then it should at least be fair to ask the reviewers to take another quick look and check if no other such issue was introduced in that same specific PR (#10322). I didn't think so, but just in case.

From a very quick look, it appears that in most places the notation jsonObjectBuilder().add(...).add(...)... is used - and that should work with any such null cases. There are quite a few places with explicit null checks, like

JsonObjectBuilder bld = jsonObjectBuilder();
bld.add("name", wf.getName());
if ( wf.getId() != null ) {
   bld.add("id", wf.getId());
}

(these seem to be old, likely predating the NullSafeJsonBuilder class).
Idk, it may be safe-ish to assume that all the other practical cases where null values were a problem had already been addressed in the past (?).

[qqmyers]

Looks good. Verified that the problem is resolved when using DVUploader to call the API.

[landreev]

Please note that I removed the milestone "6.1.1" and replaced it with "6.2". The definition of "6.1.1." was that it was something that needed to be included in the prod. patch of 6.1 deployed at IQSS, or in the 6.1 patch distributed to the community. I realized that this PR fixed something that was only broken in the develop branch - i.e., it was a new bug that was not in 6.1, so patching it there was not necessary.