Add new method to set/get object retention & Legal Hold #836

sinhaashish · 2020-01-27T08:02:10Z

Add new method to set/get object retention and Legal Hold

Function test not added as it will make the object WORM enabled and cant be deleted after successful run of test.

api/src/main/java/io/minio/messages/ObjectLockRetentionConfiguration.java

api/src/main/java/io/minio/messages/ObjectLockRetention.java

api/src/main/java/io/minio/DateFormat.java

api/src/main/java/io/minio/MinioClient.java

sinhaashish · 2020-02-02T18:05:06Z

@balamurugana Need one confirmation .
I am using public static final DateTimeFormatter RETENTION_DATE_FORMAT = EXPIRATION_DATE_FORMAT; for date format.
this is leading to an exception against MinIo but works fine against S3.

The exception is :

Exception in thread "main" java.lang.IllegalArgumentException: Invalid format: "2020-02-03T22:30:00Z" is malformed at "Z"
	at org.joda.time.format.DateTimeFormatter.parseDateTime(DateTimeFormatter.java:899)
	at io.minio.messages.ObjectRetentionConfiguration.retainUntil(ObjectRetentionConfiguration.java:80)
	at SetGetObjectRetentionConfig.main(SetGetObjectRetentionConfig.java:66)

As per S3 the retainUntil Date is of this form 2020-01-05T00:00:00.000Z Refer

But on Minio Server it is stored as 2020-01-05T00:00:00.00Z . Notice one 0 is less in the date format when stored in Minio. This leads to exception while parsing against Minio.

Can you please suggest that is it a server side bug , or do i need to have some change in client side.

balamurugana · 2020-02-02T21:46:56Z

@balamurugana Need one confirmation .
I am using public static final DateTimeFormatter RETENTION_DATE_FORMAT = EXPIRATION_DATE_FORMAT; for date format.

You don't need to have RETENTION_DATE_FORMAT, instead use EXPIRATION_DATE_FORMAT directly.

this is leading to an exception against MinIo but works fine against S3.

The exception is :
Exception in thread "main" java.lang.IllegalArgumentException: Invalid format: "2020-02-03T22:30:00Z" is malformed at "Z"
	at org.joda.time.format.DateTimeFormatter.parseDateTime(DateTimeFormatter.java:899)
	at io.minio.messages.ObjectRetentionConfiguration.retainUntil(ObjectRetentionConfiguration.java:80)
	at SetGetObjectRetentionConfig.main(SetGetObjectRetentionConfig.java:66)
As per S3 the retainUntil Date is of this form 2020-01-05T00:00:00.000Z Refer

But on Minio Server it is stored as 2020-01-05T00:00:00.00Z . Notice one 0 is less in the date format when stored in Minio. This leads to exception while parsing against Minio.

Can you please suggest that is it a server side bug , or do i need to have some change in client side.

Server needs to comply with the spec, else its server bug.

balamurugana · 2020-02-02T21:55:07Z

api/src/main/java/io/minio/MinioClient.java

+          String[] hashes = Digest.sha256Md5Hashes(data, len);
+          sha256Hash = hashes[0];
+          md5Hash = hashes[1];
+          headerMap.remove("retention","");


Not a good approach to pass and remove headers or params for sake of checksum calculation. You could pass a flag from higher level calls whether MD5 checksum calculation is required or not. You could add this improvement and remove these nested if guesses.

balamurugana · 2020-02-02T22:02:23Z

api/src/main/java/io/minio/MinioClient.java

+
+    if (!versionId.isEmpty()) {
+      queryParamMap.put("versionId", versionId);
+    }


Treat null versionId as empty string to support minio server and add it into query param i.e.

if (versionId == null) { queryParamMap.put("versionId", ""); } else { queryParamMap.put("versionId", versionId); }

You have to use this logic in all methods accepting versionId

balamurugana · 2020-02-02T22:03:36Z

api/src/main/java/io/minio/MinioClient.java

+   * @throws InvalidResponseException    upon a non-xml response from server
+   */
+  public void setObjectRetention(String bucketName, String objectName, String versionId, 
+          boolean bypassGovernanceRetention, ObjectRetentionConfiguration config)


move this boolean as last param looks like its optional.

balamurugana · 2020-02-02T22:08:53Z

api/src/main/java/io/minio/MinioClient.java

+      headerMap.put("x-amz-bypass-governance-retention", "True");
+    }
+
+    HttpResponse response = executePut(bucketName, objectName, headerMap, queryParamMap, config, 0);


config must be non-null and Mode and RetainUntilDate are mandatory configuration. You would need to add this validation properly.

balamurugana · 2020-02-02T22:39:00Z

api/src/main/java/io/minio/MinioClient.java

+   * @throws InvalidResponseException    upon a non-xml response from server
+   */
+  public void setObjectLegalHold(String bucketName, String objectName, String versionId, 
+          boolean legalHold)


You would need to have below more friendlier APIs than boolean flag

public void enableObjectLegalHold(String bucketName, String objectName, String versionId) public void disableObjectLegalHold(String bucketName, String objectName, String versionId) public boolean isObjectLegalHoldEnabled(String bucketName, String objectName, String versionId)

balamurugana · 2020-02-02T22:55:42Z

api/src/main/java/io/minio/messages/ObjectLockLegalHold.java

+   * Indicates whether the specified object has a Legal Hold in place.
+   */
+  public String getStatus() {
+    return status;


You need to retrurn boolean depending on status; not string

balamurugana · 2020-02-02T22:59:51Z

api/src/main/java/io/minio/MinioClient.java

+      stream = new BufferedInputStream(stream);
+    }
+    if (objectLockRetention) {
+      headerMap.put("retention","");


Two things you can do at the time of put object.

retention (Mode and RetainUntilDate are required)

legal hold (boolean to say ON or OFF)

You could send these changes in put object as separate PR.

Will send a separate PR for this

sinhaashish · 2020-02-06T06:49:19Z

@balamurugana PTAL

balamurugana · 2020-02-06T10:01:19Z

api/src/main/java/io/minio/messages/ObjectLockLegalHold.java

+   * Indicates whether the specified object has a Legal Hold in place or not.
+   */
+  public Boolean getStatus() {
+    return Boolean.parseBoolean(status);


have status(), not getStatus()

I don't see javadoc of Boolean parses "ON"/"OFF" string. Are you sure it works?

Return boolean not Boolean

balamurugana · 2020-02-06T10:02:50Z

api/src/main/java/io/minio/messages/ObjectLockLegalHold.java

+  /**
+   * Constructs a new CustomRetention object with given retention.
+   */
+  public ObjectLockLegalHold(boolean legalHold) throws  XmlPullParserException {


argument is status not legalHold

balamurugana · 2020-02-06T10:03:56Z

api/src/main/java/io/minio/messages/ObjectRetentionConfiguration.java

+  public ObjectRetentionConfiguration(RetentionMode mode,  DateTime retainUntilDate) throws XmlPullParserException,
+          InvalidArgumentException {
+    super();
+    super.name = "Retention";


call this() than repeating these two lines

balamurugana · 2020-02-06T10:04:38Z

api/src/main/java/io/minio/messages/ObjectLockLegalHold.java

+   */
+  public ObjectLockLegalHold(boolean legalHold) throws  XmlPullParserException {
+    super();
+    super.name = "LegalHold";


use this() and don't repeat logic

balamurugana · 2020-02-06T10:05:56Z

api/src/main/java/io/minio/messages/ObjectRetentionConfiguration.java

+    super();
+    super.name = "Retention";
+    if (mode == null) {
+      throw new InvalidArgumentException("null is not allowed in mode. Valid values are 'COMPLIANCE' and 'GOVERNANCE'");


change error message as null mode is not allowed

balamurugana · 2020-02-06T10:07:01Z

api/src/main/java/io/minio/messages/ObjectRetentionConfiguration.java

+    }
+    this.mode = mode.toString();
+    if (retainUntilDate == null) {
+      throw new InvalidArgumentException("retain until date cant be null, it must be a valid date.");


change error message as null retainUntilDate is not allowed

balamurugana · 2020-02-06T10:08:45Z

api/src/main/java/io/minio/messages/ObjectRetentionConfiguration.java

+  /**
+   * Returns retain until date.
+   */
+  public Date retainUntil() {


Method name should be same as member field retainUntilDate() and return DateTime not Date i.e. should be same as argument type in the constructor

balamurugana · 2020-02-06T10:12:07Z

api/src/main/java/io/minio/MinioClient.java

@@ -1402,11 +1400,12 @@ private HttpResponse executeDelete(String bucketName, String objectName, Map<Str
   * @param data           HTTP request body data.
   */
  private HttpResponse executePost(String bucketName, String objectName, Map<String,String> headerMap,
-                                   Map<String,String> queryParamMap, Object data)
+                                   Map<String,String> queryParamMap, Object data, boolean md5Required)


Have overloaded executePost() i.e. with/without md5Required argument.

balamurugana · 2020-02-06T10:12:50Z

api/src/main/java/io/minio/MinioClient.java

@@ -1439,13 +1438,13 @@ private HttpResponse executePost(String bucketName, String objectName, Map<Strin
   * @param length         Length of HTTP request body data.
   */
  private HttpResponse executePut(String bucketName, String objectName, Map<String,String> headerMap,
-                                  Map<String,String> queryParamMap, String region, Object data, int length)
+                  Map<String,String> queryParamMap, String region, Object data, int length, boolean md5Required)


Have overloaded executePut() i.e. with/without md5Required

balamurugana · 2020-02-06T10:13:43Z

api/src/main/java/io/minio/MinioClient.java

@@ -1461,11 +1460,12 @@ private HttpResponse executePut(String bucketName, String objectName, Map<String
   * @param length         Length of HTTP request body data.
   */
  private HttpResponse executePut(String bucketName, String objectName, Map<String,String> headerMap,
-                                  Map<String,String> queryParamMap, Object data, int length)
+                                  Map<String,String> queryParamMap, Object data, int length,  boolean md5Required)


Same as above

balamurugana · 2020-02-06T10:15:07Z

api/src/main/java/io/minio/MinioClient.java

@@ -2388,7 +2388,7 @@ public void copyObject(String bucketName, String objectName, Map<String,String>
      headerMap.putAll(copyConditions.getConditions());
    }

-    HttpResponse response = executePut(bucketName, objectName, headerMap, null, "", 0);
+    HttpResponse response = executePut(bucketName, objectName, headerMap, null, "", 0, false);


Use overloaded executePut()/executePost() accepting md5Required argument to avoid unnecessary diffs.

balamurugana · 2020-02-08T13:35:55Z

api/src/main/java/io/minio/messages/ObjectRetentionConfiguration.java

+   * Returns mode.
+   */
+  public RetentionMode mode() {
+    return RetentionMode.fromString(mode);


mode can be null. You need to return null accordingly

balamurugana · 2020-02-08T13:37:29Z

api/src/main/java/io/minio/messages/ObjectLockLegalHold.java

+    if (status.equals("ON")) {
+      return true;
+    }
+    return false;


return status != null && status.equals("ON"); is correct and sufficient

balamurugana · 2020-02-08T13:45:26Z

api/src/main/java/io/minio/MinioClient.java

+
+    if ( !(versionId == null || versionId.isEmpty())) {
+      queryParamMap.put("versionId",versionId);
+    }


I don't know why you are missing out of versionId handling. Are you deferring in other places?

balamurugana · 2020-02-08T13:48:48Z

api/src/main/java/io/minio/MinioClient.java

+   * @throws InsufficientDataException   upon getting EOFException while reading given
+   */
+  public void putObject(String bucketName, String objectName, String fileName, Long size, Map<String, String> headerMap,
+                        ServerSideEncryption sse, String contentType, boolean objectLockRetention)


As we agreed previously, you are going to send separate PR to have putObject() with retention and legal hold. Are you still considering to add in this PR?

Yes, that i will send as another PR.

If so, why are doing this change here?

If so, why are doing this change here?

This change is needed .
There are two cases to this flow:
User creates a bucket with lock function enabled.
Case 1 : User can put the object on the bucket and then at later point of time, set the retenetion. This case is handled in this PR.
Case 2 : User asks for retention/legalhold at the time of putting object. This will be handled in another PR.

If we dont have the below overloaded putObject then we wont be able to put the object on
lock enabled buckets and hence not able to test the PR.

public void putObject(String bucketName, String objectName, String fileName, Long size, Map<String, String> headerMap, ServerSideEncryption sse, String contentType, boolean objectLockRetention)

If you think that case 2 should also be present in this PR , then i can come up with that .

You cannot add any changes to putObject() if you plan for separate PR.

sinhaashish · 2020-02-12T04:49:44Z

@balamurugana PTAL

sinhaashish mentioned this pull request Jan 27, 2020

Set retention namespace for ObjectRetention minio/minio#8896

Closed

7 tasks

sinhaashish force-pushed the get-set-object-retention branch 2 times, most recently from 4546da1 to b5c1c80 Compare January 27, 2020 17:21

balamurugana reviewed Jan 28, 2020

View reviewed changes

api/src/main/java/io/minio/messages/ObjectLockRetentionConfiguration.java Outdated Show resolved Hide resolved

balamurugana reviewed Jan 28, 2020

View reviewed changes

api/src/main/java/io/minio/messages/ObjectLockRetentionConfiguration.java Outdated Show resolved Hide resolved

balamurugana reviewed Jan 28, 2020

View reviewed changes

api/src/main/java/io/minio/messages/ObjectLockRetentionConfiguration.java Outdated Show resolved Hide resolved

balamurugana reviewed Jan 28, 2020

View reviewed changes

api/src/main/java/io/minio/messages/ObjectLockRetentionConfiguration.java Outdated Show resolved Hide resolved

balamurugana reviewed Jan 28, 2020

View reviewed changes

api/src/main/java/io/minio/messages/ObjectLockRetention.java Outdated Show resolved Hide resolved

balamurugana reviewed Jan 28, 2020

View reviewed changes

api/src/main/java/io/minio/messages/ObjectLockRetention.java Outdated Show resolved Hide resolved

balamurugana reviewed Jan 28, 2020

View reviewed changes

api/src/main/java/io/minio/messages/ObjectLockRetention.java Outdated Show resolved Hide resolved

balamurugana reviewed Jan 28, 2020

View reviewed changes

api/src/main/java/io/minio/DateFormat.java Outdated Show resolved Hide resolved

balamurugana reviewed Jan 28, 2020

View reviewed changes

api/src/main/java/io/minio/MinioClient.java Outdated Show resolved Hide resolved

sinhaashish force-pushed the get-set-object-retention branch from b5c1c80 to 3dbb344 Compare February 2, 2020 14:59

harshavardhana previously approved these changes Feb 2, 2020

View reviewed changes

sinhaashish dismissed harshavardhana’s stale review via c6c316b February 2, 2020 17:54

sinhaashish force-pushed the get-set-object-retention branch from 3dbb344 to c6c316b Compare February 2, 2020 17:54

sinhaashish changed the title ~~Add new method to set/get object retention~~ Add new method to set/get object retention & Legal Hold Feb 2, 2020

balamurugana reviewed Feb 2, 2020

View reviewed changes

sinhaashish force-pushed the get-set-object-retention branch 3 times, most recently from 47d23da to e6668cb Compare February 6, 2020 06:23

balamurugana reviewed Feb 6, 2020

View reviewed changes

sinhaashish force-pushed the get-set-object-retention branch from e6668cb to 1462c63 Compare February 6, 2020 19:28

sinhaashish requested review from balamurugana and harshavardhana February 6, 2020 19:46

sinhaashish added 2 commits February 7, 2020 09:49

Add new method to set/get object retention and legal hold

a96dde3

Legal Hold methods modified

29bdecd

sinhaashish force-pushed the get-set-object-retention branch from 0f3cd29 to bb595dd Compare February 7, 2020 04:20

balamurugana reviewed Feb 8, 2020

View reviewed changes

Add argument to calculate md5 if needed

fc431eb

sinhaashish force-pushed the get-set-object-retention branch from bb595dd to fc431eb Compare February 9, 2020 02:07

Remove overloaded putObject

2c5206f

sinhaashish force-pushed the get-set-object-retention branch from fffaeec to 2c5206f Compare February 12, 2020 14:02

balamurugana approved these changes Feb 13, 2020

View reviewed changes

harshavardhana approved these changes Feb 13, 2020

View reviewed changes

harshavardhana merged commit 8427afc into minio:master Feb 13, 2020

sinhaashish deleted the get-set-object-retention branch April 1, 2020 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new method to set/get object retention & Legal Hold #836

Add new method to set/get object retention & Legal Hold #836

sinhaashish commented Jan 27, 2020 •

edited

Loading

sinhaashish commented Feb 2, 2020 •

edited

Loading

balamurugana commented Feb 2, 2020

balamurugana Feb 2, 2020

balamurugana Feb 2, 2020

balamurugana Feb 6, 2020

balamurugana Feb 2, 2020

balamurugana Feb 2, 2020

balamurugana Feb 2, 2020

balamurugana Feb 2, 2020

balamurugana Feb 2, 2020

sinhaashish Feb 6, 2020

sinhaashish commented Feb 6, 2020

balamurugana Feb 6, 2020 •

edited

Loading

balamurugana Feb 6, 2020

balamurugana Feb 6, 2020

balamurugana Feb 6, 2020

balamurugana Feb 6, 2020

balamurugana Feb 6, 2020

balamurugana Feb 6, 2020

balamurugana Feb 6, 2020

balamurugana Feb 6, 2020

balamurugana Feb 6, 2020

balamurugana Feb 6, 2020

balamurugana Feb 8, 2020

balamurugana Feb 8, 2020 •

edited

Loading

balamurugana Feb 8, 2020

balamurugana Feb 8, 2020

sinhaashish Feb 9, 2020

balamurugana Feb 12, 2020

sinhaashish Feb 12, 2020

balamurugana Feb 12, 2020

sinhaashish commented Feb 12, 2020

Add new method to set/get object retention & Legal Hold #836

Add new method to set/get object retention & Legal Hold #836

Conversation

sinhaashish commented Jan 27, 2020 • edited Loading

sinhaashish commented Feb 2, 2020 • edited Loading

balamurugana commented Feb 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinhaashish commented Feb 6, 2020

balamurugana Feb 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balamurugana Feb 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinhaashish commented Feb 12, 2020

sinhaashish commented Jan 27, 2020 •

edited

Loading

sinhaashish commented Feb 2, 2020 •

edited

Loading

balamurugana Feb 6, 2020 •

edited

Loading

balamurugana Feb 8, 2020 •

edited

Loading