Fixes for list object versions result

[mdedetrich]

This PR fixes a few changes that was introduced in my previous PR at #2747. The easiest way to navigate the PR is to look at the commits which are described below.

Make Owner fields optional for ListObjectVersionsResult

This commit is mandatory since it appears that in some circumstances you do not get the Owner field in the S3 response. Ideally and originally I wanted to make a test to replicate this but it seems like you need a specific AWS account with a certain set of permissions in order to have the missing fields. i.e. on one AWS account I get this as a response

<ListVersionsResult xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
<Name>wzlfljq6</Name>
<Prefix/>
<KeyMarker/>
<VersionIdMarker/>
<MaxKeys>1000</MaxKeys>
<IsTruncated>false</IsTruncated>
<Version>
    <Key>1970-01-01T00:00:00.179Z.json</Key>
    <VersionId>null</VersionId>
    <IsLatest>true</IsLatest>
    <LastModified>2021-10-11T18:09:50.000Z</LastModified>
    <ETag>&quot;436e1b9fdef26ffe3f6c9f20524f7cfe-1&quot;</ETag>
    <Size>8845</Size>
    <StorageClass>STANDARD</StorageClass>
</Version>
<Version>
    <Key>1970-01-01T00:00:00.341Z.json</Key>
    <VersionId>null</VersionId>
    <IsLatest>true</IsLatest>
    <LastModified>2021-10-11T18:09:50.000Z</LastModified>
    <ETag>&quot;dc997d6c24774e24e7efb2b027761be4-1&quot;</ETag>
    <Size>3796</Size>
    <StorageClass>STANDARD</StorageClass>
</Version>
</ListVersionsResult>

where as with another AWS account I get this as a response

<ListVersionsResult xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
    <Name>alpakka-test-bucket-1</Name>
    <Prefix/>
    <KeyMarker/>
    <VersionIdMarker/>
    <MaxKeys>1000</MaxKeys>
    <IsTruncated>false</IsTruncated>
    <Version>
        <Key>deleteBucketContentsTest</Key>
        <VersionId>null</VersionId>
        <IsLatest>true</IsLatest>
        <LastModified>2021-10-11T18:23:19.000Z</LastModified>
        <ETag>&quot;83beb8c4fa4596c8f7b565d390f494e2&quot;</ETag>
        <Size>11</Size>
        <Owner>
            <ID>REDACTED</ID>
            <DisplayName>awstest</DisplayName>
        </Owner>
        <StorageClass>STANDARD</StorageClass>
    </Version>
</ListVersionsResult>

I have a suspicion that this may be due to how AWS permissions are set up on the account which means that I can't really write a test for this (changing/creating actual AWS accounts is out of scope for tests).

Make VersionId optional by detecting null

When you call S3.listObjectVersions on a bucket that doesn't have versioning enabled (which is the default) you get versionId=null in the XML response (see above XML response snippets as well as https://docs.aws.amazon.com/AmazonS3/latest/userguide/AddingObjectstoVersionSuspendedBuckets.html). This commit makes the versionId field an Option where None is the case where we get null from AWS as a quality of life improvement.

Deleting versions with S3.deleteBucketContents is now optional

Upon the realization that S3.listObjectVersions completely fails if you are using Minio I deemed it appropriate to make the behavior of deleting all object versions in S3.deleteBucketContents/S3.deleteObjectsByPrefix optional with a default of NOT deleting object versions.

A test was added to verify this behavior.

This is mainly done as a safety precaution (i.e. the default behavior is now the same at the point in time before #2747 was merged) and also as a means of allowing Minio users to actually use S3.deleteBucketContents/S3.deleteObjectsByPrefix (otherwise this method will always fail because Minio doesn't even seem to support the S3.listObjectVersions call).

[mdedetrich]

I had another thought regarding the Make VersionId optional by detecting null change, it can also be further improved by checking for null when you try to construct/modify the associated data structures, i.e.

def withVersionId(value: String): ListObjectVersionsResultVersions = copy(versionId = Option(value))

can be changed to

def withVersionId(value: String): ListObjectVersionsResultVersions =
  if (value.trim.toLowerCase == "null")
    copy(versionId = None)
  else
    copy(versionId = Option(value)    

Same applies for apply. This can prevent users of the API accidentally putting in "null".

EDIT: closing of PR was accident, I must have hit some hotkey.....

[johanandren]

LGTM with some small suggestions

[mdedetrich]

@jochenschneider Thanks for the comments, I have addressed them, what are your thoughts on #2751 (comment) ?

[johanandren]

Not sure I get when a user would call withVersionId() with a null string, isn't all cases when it comes from the S3 API covered?

[mdedetrich]

Not sure I get when a user would call withVersionId() with a null string, isn't all cases when it comes from the S3 API covered?

I guess if some people are mixing the official S3 SDK with Alpakka (i.e. you get a value from the S3 SDK where versionId == "null" and you put that value into a DeleteMarker.withVersionId for example) or manually constructing the various data structures.

Ultimately I think its more of a consistency thing, in essence the versionId values from S3 are a union type of <some_Id> | "null" and what the commit does is it formalizes it to Scala's Option which also has the same properties but people may get interesting surprises if it doesn't act consistently this way.

Alternately I can just drop the commit if its too confusing.

[johanandren]

Yeah, I guess it can't hurt either, feel free to do as you like (it's not in here now as far as I can see).

[mdedetrich]

Okay I have just rebased the PR with the changes mentioned in #2751 (comment)

[mdedetrich]

@johanandren The tests have failed but its due to getting rate limited by docker, i.e.

ERROR: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit

So its unrelated.

[johanandren]

LGTM but let's kick the ci again tomorrow and see that it goes green

[mdedetrich]

@johanandren I have just added a commit that fixes [[java.lang.Void]] documentation issue in the entire S3 file for Java, this also triggered a new CI pass and everything is passing now

[johanandren]

LGTM, thanks!