search

SpectraLogic / ds3_java_sdk

Apache License 2.0
7 stars 15 forks source link

Starting to modernize Gradle #595

Closed scribe closed 2 years ago

scribe commented 2 years ago

This is a draft because I still don't know what all the dependencies need to be modernized, and I need to figure out why upgrading causes 3 tests to fail.

lesserwhirls commented 2 years ago

It looks like the failures are due to the mocks returning empty values rather than null values?

scribe commented 2 years ago

It looks like the failures are due to the mocks returning empty values rather than null values?

I think so? But I worry swapping something else has changed the real behavior

lesserwhirls commented 2 years ago

Looks like the upgrade from jackson 2.9.0 to 2.13.3 triggered the new failures. We can go up to 2.11.x, but the failure begins in 2.12.x due to this change: https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.12#xml-module.

So the question is, do we want to keep the behavior of mapping empty xml elements to null?

scribe commented 2 years ago

I'm heavily tempted to say we go with the newest behavior, but I also don't feel like that's my call to make? Empty elements are easier to deal with than nulls most of the time?

lesserwhirls commented 2 years ago

Yeah, I'm +1 for empty objects over nulls. @RachelTucker, what do you think?

scribe commented 2 years ago

@blitt How much should we try to preserve the current behavior of the SDK vs modernizing it?

RachelTucker commented 2 years ago

Would an empty Boolean be false and an empty Integer be 0 with the new behavior? The reason why some values are nullable to begin with is because a value not being returned in a response payload has different meaning than a default value being returned. One example of the difference in meaning is written up in GOSDK-36.

lesserwhirls commented 2 years ago

Would an empty Boolean be false and an empty Integer be 0 with the new behavior? The reason why some values are nullable to begin with is because a value not being returned in a response payload has different meaning than a default value being returned. One example of the difference in meaning is written up in GOSDK-36.

In this case, the issue is howjackson-dataformat-xml handles <tag/> vs <tag></tag> prior to coercion, so the question would be if BPs XML treats those as having a different meaning or not. Prior to version 2.12.0, <tag/> would be treated as null, but <tag></tag> would be treated as "". Starting with 2.12.0, the behavior is that both are treated as "" (unless xsi:nil is specified (with proper namespace) on the element, and possibly that the xsd indicates that it is nillable, I think). A little more info can be found here.

If I tell the XmlMapper to treat empty as null, the three failing tests in this PR pass. I think the thing to do is to keep the behavior as-is, but maybe However, as long as BP does not make a distinction between the two different empty element forms, I would suggest we consider changing it in 5.5.x.

RachelTucker commented 2 years ago

Edit: got that backwords

It looks like BP currently returns <tag/> when things are null, not <tag></tag>.

Example: <Data><Activated>true</Activated><AllowNewJobRequests>true</AllowNewJobRequests><AutoActivateTimeoutInMins>44</AutoActivateTimeoutInMins><AutoInspect>MINIMAL</AutoInspect><CacheAvailableRetryAfterInSeconds>60</CacheAvailableRetryAfterInSeconds><DefaultVerifyDataAfterImport/><DefaultVerifyDataPriorToImport>true</DefaultVerifyDataPriorToImport><Id>d9a3a38b-1dfe-4a63-8d79-fc8efd5a8da6</Id><InstanceId>d9a3a38b-1dfe-4a63-8d79-fc8efd5a8da6</InstanceId><IomCacheLimitationPercent>0.5</IomCacheLimitationPercent><IomEnabled>true</IomEnabled><LastHeartbeat>2022-08-17T22:54:35.514Z</LastHeartbeat><MaxAggregatedBlobsPerChunk>20000</MaxAggregatedBlobsPerChunk><PartiallyVerifyLastPercentOfTapes/><PoolSafetyEnabled>true</PoolSafetyEnabled><UnavailableMediaPolicy>DISALLOW</UnavailableMediaPolicy><UnavailablePoolMaxJobRetryInMins>20</UnavailablePoolMaxJobRetryInMins><UnavailableTapePartitionMaxJobRetryInMins>20</UnavailableTapePartitionMaxJobRetryInMins><VerifyCheckpointBeforeRead>true</VerifyCheckpointBeforeRead></Data>

lesserwhirls commented 2 years ago

Ok, so by BP convention the empty element in the form of <tag/> means nil value, as opposed to something like:

<?xml version="1.0" encoding="UTF-8"?>
<root xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
   ...
   <ele xsi:nil="true"/> 
   ...
</root>

We'll make sure to keep the prior behavior and not look to change it in the future.