Code cleanups and simplifications.

[donatello]

No description provided.

[balamurugana]

CI fails. Please look into that.

[donatello]

@balamurugana CI failure is fixed. It was some kind of intermittent problem in the CI, and not in the change itself. Got fixed after a force push.

[balamurugana]

👍

[harshavardhana]

Two comments..

[donatello]

@harshavardhana

This will fail on windows. We need to explicitly close the file. Can you validate ?

No it should not. The with statement was introduced a long time back (in Py 2.5 https://docs.python.org/3/whatsnew/2.6.html#pep-343-the-with-statement and more powerful in py 3 - https://docs.python.org/3/reference/compound_stmts.html#with ) and is meant to do things like close files that is alternatively done with same guarantee with try...catch...finally block (just calling f.close() after the with statement would not have the same effect - in case there was a bug in windows - because of exceptions).

I tried to find if there are any bug reports related to Windows and closing files and the with statement, but I didn't see any. If there was such a bug, it would be quite serious and there would be multiple reports.

Why a strict bytes type ?

In the case of fput_object this is not a problem, since our code opens the file in binary mode and this returns only bytes.

In put_object, the data returned by the io.RawIOBase instance, could return even str (unicode) streams, as they are user provided. How the unicode stream should be stored on disk requires us to know what encoding is intended by the user - it is not necessarily utf-8 (user may intend utf-16, etc). To avoid any unintended encoding of file-data we accept only bytes - this makes sure that the user encodes it to bytes and then calls put_object.

In other situations it is ok for us to encode str type to utf-8 and convert to bytes, as in the library, we know what data we are encoding in all other situations.

[donatello]

Added a comment explaining why we accept only strict bytes in _do_put_object.

[harshavardhana]

Why not do this inside set_content_length_range()

[donatello]

We probably could do this (and also the expiration time change/comment), but I prefer to do these changes in another refactor - the reason is that I am not sure the current PostPolicy API is cleanly done.

The current implementation has all its methods names like set_expires, set_key_startswith, etc but the name set_* implies that if we do the operation repeatedly, only the last value/call has effect - but the implementation does not satisfy this natural expectation - if we call set_bucket_name twice, it will append two conditions to the policy, possibly rendering it invalid - in the S3 spec, it seems these policy conditions are conjunctive (AND-ed together). However, the expiration condition, and the content length condition are ok to be set repeatedly to different values, and the programmer's intention is satisfied by the library implementation. I want to make this behaviour consistent in a (near) future refactor.

[harshavardhana]

The current implementation has all its methods names like set_expires, set_key_startswith, etc but the name set_* implies that if we do the operation repeatedly, only the last value/call has effect - but the implementation does not satisfy this natural expectation - if we call set_bucket_name twice, it will append two conditions to the policy, possibly rendering it invalid - in the S3 spec, it seems these policy conditions are conjunctive (AND-ed together).

To solve this problem we need to fix that first, this has to be done perhaps in all libraries FWICS.

[harshavardhana]

why not do this in set_expires?

[donatello]

See reply in comment about content-length-range.

[harshavardhana]

can you update the licence header for these files as well pointing to '2015, 2016'.

[harshavardhana]

Do it wherever you are cleaning up.

[donatello]

Done.