Revisit last_modified in StreamResponse

[greshilov]

What do these changes do?

Bug fixes related to last_modified attribute in StreamResponse object.

Are there changes in behavior for the user?

Nope.

Related issue number

#5303, #5304

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • The format is <Name> <Surname>.
  • Please keep alphabetical order, the file is sorted by names.
• Add a new news fragment into the CHANGES folder
  • name it <issue_id>.<type> for example (588.bugfix)
  • if you don't have an issue_id change it to the pr id after creating the pr
  • ensure type is one of the following:
    • .feature: Signifying a new feature.
    • .bugfix: Signifying a bug fix.
    • .doc: Signifying a documentation improvement.
    • .removal: Signifying a deprecation or removal of public API.
    • .misc: A ticket has been closed, but it is not of interest to users.
  • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.15%. Comparing base (4121c94) to head (a7072ec).
Report is 4152 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5313      +/-   ##
==========================================
- Coverage   97.16%   97.15%   -0.02%     
==========================================
  Files          41       41              
  Lines        8739     8742       +3     
  Branches     1402     1403       +1     
==========================================
+ Hits         8491     8493       +2     
  Misses        129      129              
- Partials      119      120       +1     

Flag	Coverage Δ
unit	97.03% <100.00%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[asvetlov]

There is no easy way to create tz-aware local current time.
In turn, datetime.now() is widely used as local naive time.
datetime.utcnow() usage is discouraged for the sake of datetime.now(tz=timezone.utc).

So, I suggest dropping this warning but replacing value.utctimetuple() with value.timetuple() a few lines below.

https://docs.aiohttp.org/en/stable/web_reference.html?highlight=last_modified#aiohttp.web.StreamResponse.last_modified should describe how the library works with naive time objects.

[greshilov]

So, I guess there are 3 cases in new behaviour:

• Naive datetime is used.
  Must be considered as UTC. Documentation clearly describes this case.

• tz-aware datetime is used and timezone is utc.
  All is ok, user gave us correct object.

• tz-aware datetime is used and timezone is not utc.
  That's definitely wrong input to use with value.timetuple(). ValueException should be raised.

Is that correct?

[asvetlov]

I know, the change is not backward compatible but I don't see a smooth transition.
@serhiy-storchaka what do you think?

[serhiy-storchaka]

To make the change more gradual we would need to emit FutureWarning, and change the behavior only after passing some period (a year or two). But if most of uses of naive datetime objects are datetime.now() this change will actually fix currently broken user code.

[Dreamsorcerer]

The floor is unneeded, gmtime() already does that.

Suggested change

	"%a, %d %b %Y %H:%M:%S GMT", time.gmtime(math.floor(value))
	"%a, %d %b %Y %H:%M:%S GMT", time.gmtime(value)