Improve recursive object restore

[lavigne958]

When restoring a complete bucket stop and raise an error only when the configuration is set to stop on the first error. otherwise keep trying to restore the next objects.

closes #1369

[fviard]

Thank you for your PR, it looks good to me.

But, regarding the following behavior that you suggested in the issue:
"if the storage class is already the right one -> nothing to do just keep iterating on the next object"

We will still have an issue if there are "non glacier" files in the folder that you want to restore:
ERROR: S3 error: 403 (InvalidObjectState): Restore is not allowed for the object's current storage class

Is it expected in the way that you see your change?
Or, alternatively, should we also silently skip that?

[lavigne958]

Thank you for your PR, it looks good to me.

thanks 🙃

But, regarding the following behavior that you suggested in the issue: "if the storage class is already the right one -> nothing to do just keep iterating on the next object"

From AWS documentation an object already in the right storage class will return a 200 HTTP code. I'll run a check anyway just in case

We will still have an issue if there are "non glacier" files in the folder that you want to restore: ERROR: S3 error: 403 (InvalidObjectState): Restore is not allowed for the object's current storage class

Is it expected in the way that you see your change? Or, alternatively, should we also silently skip that?

I'll suggest we skip them too, if the object is concerned by a restore request (because it's already in standard class, it cannot be restored etc) we should just skip it.

[fviard]

From AWS documentation an object already in the right storage class will return a 200 HTTP code. I'll run a check anyway just in case

There is a subtile detail that is not well documented in my opinion.
If you have an object that is on glacier but that was already restored, and so already available, then you will get a 200.
But, if the object has a standard storage class and so was not in Glacier anyway, you will get the 403 error in response.

I'll suggest we skip them too, if the object is concerned by a restore request (because it's already in standard class, it cannot be restored etc) we should just skip it.

I agree that it looks like to be a reasonable behavior. Let me know if you want to update this PR with this change or prefer to do another one for that.

[lavigne958]

There is a subtile detail that is not well documented in my opinion. If you have an object that is on glacier but that was already restored, and so already available, then you will get a 200. But, if the object has a standard storage class and so was not in Glacier anyway, you will get the 403 error in response.

agreed, I tried it: when trying to restore an object in standard class, that is not concerned by GLACIER we receive as you said it the error InvalidObjectState .
To me we should skip them exactly like the RestoreAlreadyInProgress . these objects are not concerned by the given request.
The user should be able to control how the error should raise (from first error or not) for objects concerned by the request and so object from GLACIER storage class, not the other objects.

I agree that it looks like to be a reasonable behavior. Let me know if you want to update this PR with this change or prefer to do another one for that.

I would put it in the same PR, it belongs to the same improvement.

[lavigne958]

I reworked the code so we ignore both errors (already in progress and object already in correct storage class.

[fviard]

Merged, thank you very much