add append_version feature into StaticResource url resolver #2158

sky-code · 2017-08-02T14:10:54Z

Are there changes in behavior for the user?

Fully backward compatible feature (has no impact on exists code)

Related issue number

Please goto #2157 for description and discussion

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.bug)
- 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."

asvetlov · 2017-08-02T14:27:38Z

Please mention new parameter and behavior in docs

sky-code · 2017-08-02T16:05:53Z

I have added documentation for url_for but not for url because it's deprecated

asvetlov · 2017-08-02T16:24:26Z

The PR only generates new version tag by url_for() call.

Should it return redirection response on version mismatch?

Should we move append_version parameter to constructor?

It might simplify the usage: just add static files handler with parameter set (app.router.add_static('/prefix', path_to_static, append_version=True)), no need to pass append_version=True to every url_for() call.

sky-code · 2017-08-02T16:58:18Z

Should it return redirection response on version mismatch?
Not necessarily, no at this time. If someone asks to make it and provide good explanation of advantages then it will be added

Should we move append_version parameter to constructor?
maybe, set default value in constructor than override it in each call if necessary

codecov-io · 2017-08-02T18:38:41Z

Codecov Report

Merging #2158 into master will increase coverage by 0.02%.
The diff coverage is 97.14%.

@@            Coverage Diff             @@
##           master    #2158      +/-   ##
==========================================
+ Coverage   97.09%   97.11%   +0.02%     
==========================================
  Files          39       39              
  Lines        7783     7878      +95     
  Branches     1357     1366       +9     
==========================================
+ Hits         7557     7651      +94     
  Misses        101      101              
- Partials      125      126       +1

Impacted Files	Coverage Δ
aiohttp/web_urldispatcher.py	`99.34% <97.14%> (-0.08%)`	⬇️
aiohttp/helpers.py	`97.32% <0%> (+0.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6d88d8...e218008. Read the comment docs.

asvetlov

Well, let's don't redirect but add append_version to router.add_static

asvetlov · 2017-08-03T18:18:36Z

aiohttp/web_urldispatcher.py

+        # with_query remove previous query options
+        url = self.url_for(filename=filename, append_version=append_version)
+        if query is not None:
+            return str(url.with_query(query))


Should be update_query

asvetlov · 2017-08-03T18:20:49Z

aiohttp/web_urldispatcher.py

+                return url
+        return url
+
+    def get_file_hash(self, byte_array):


Add _ prefix to method name: it is not a part of public API

asvetlov · 2017-08-03T18:28:00Z

aiohttp/web_urldispatcher.py

+                return url
+            except Exception as error:
+                # perm error or other kind!
+                # TODO log error here?


Not sure if we should catch the exception.
url is useless: next GET will raise an error.
Moreover next open call might fail too.

asvetlov · 2017-08-03T18:30:22Z

aiohttp/web_urldispatcher.py

+                filepath = self._directory.joinpath(filename).resolve()
+                if not self._follow_symlinks:
+                    filepath.relative_to(self._directory)
+            except (ValueError, FileNotFoundError) as error:


What case could raise ValueError?

I don't know, this is copied code from _handle method, anyway i remove it

… method docs and tests is updated to reflect changes

asvetlov

Document append_version parameter for add_staticin web_reference.rst too

asvetlov · 2017-08-04T21:57:52Z

aiohttp/web_urldispatcher.py

+        # with_query remove previous query options
+        url = self.url_for(filename=filename, append_version=append_version)
+        if query is not None:
+            return str(url.update_query(query))


Please add a test for this case

asvetlov · 2017-08-04T21:58:44Z

aiohttp/web_urldispatcher.py

+        url = URL(url)
+        if append_version is True:
+            try:
+                if filename.startswith('/'):


Please add a test if filename doesn't start with /

asvetlov · 2017-08-04T21:59:24Z

aiohttp/web_urldispatcher.py

+                if filename.startswith('/'):
+                    filename = filename[1:]
+                filepath = self._directory.joinpath(filename).resolve()
+                if not self._follow_symlinks:


Test for _follow_symlink needed

asvetlov · 2017-08-05T14:14:48Z

Thanks!

lock · 2019-10-28T19:04:45Z

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

sky-code added 4 commits August 2, 2017 01:23

add StaticResource.url append_version arg

127188e

add 2157.feature

fabd128

2157.feature contributor

e23feff

sort contributors.txt

b510c1f

add documentation for StaticResource.url_for append_version arg #2157

56817fa

update read file code to be compatible with windows python 3.4

e204474

fix flake8 line len issues

cdd2257

asvetlov requested changes Aug 3, 2017

View reviewed changes

add append_version param to StaticResource constructor and add_static…

fc743bf

… method docs and tests is updated to reflect changes

asvetlov requested changes Aug 4, 2017

View reviewed changes

sky-code added 3 commits August 5, 2017 05:32

add more tests and docs for StaticRoute append_version

b076eab

removed outdated comments

d229c53

add ValueError exception catching for follow_symlinks and more tests

e218008

asvetlov approved these changes Aug 5, 2017

View reviewed changes

asvetlov merged commit ee69c40 into aio-libs:master Aug 5, 2017

asvetlov mentioned this pull request Aug 6, 2017

StaticResource url versioning #2157

Closed

lock bot added the outdated label Oct 28, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add append_version feature into StaticResource url resolver #2158

add append_version feature into StaticResource url resolver #2158

sky-code commented Aug 2, 2017 •

edited

Loading

asvetlov commented Aug 2, 2017

sky-code commented Aug 2, 2017

asvetlov commented Aug 2, 2017

sky-code commented Aug 2, 2017

codecov-io commented Aug 2, 2017 •

edited

Loading

asvetlov left a comment

asvetlov Aug 3, 2017

sky-code Aug 3, 2017

asvetlov Aug 3, 2017

sky-code Aug 3, 2017

asvetlov Aug 3, 2017

sky-code Aug 3, 2017

asvetlov Aug 3, 2017

sky-code Aug 3, 2017

asvetlov left a comment

asvetlov Aug 4, 2017

sky-code Aug 5, 2017

asvetlov Aug 4, 2017

sky-code Aug 5, 2017

asvetlov Aug 4, 2017

sky-code Aug 5, 2017

asvetlov commented Aug 5, 2017

lock bot commented Oct 28, 2019

add append_version feature into StaticResource url resolver #2158

add append_version feature into StaticResource url resolver #2158

Conversation

sky-code commented Aug 2, 2017 • edited Loading

Are there changes in behavior for the user?

Related issue number

Checklist

asvetlov commented Aug 2, 2017

sky-code commented Aug 2, 2017

asvetlov commented Aug 2, 2017

sky-code commented Aug 2, 2017

codecov-io commented Aug 2, 2017 • edited Loading

Codecov Report

asvetlov left a 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

asvetlov left a 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

asvetlov commented Aug 5, 2017

lock bot commented Oct 28, 2019

sky-code commented Aug 2, 2017 •

edited

Loading

codecov-io commented Aug 2, 2017 •

edited

Loading