Change regex, add remove method, and edit qeury_timeout #375
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@phantomcosmonaut The change looks good. I suggest to merge the change to the master branch instead - dev branch is where development by Tableau happens. It may take some time before dev is merged to master and a new release is created. |
0golovatyi
approved these changes
Dec 16, 2019
|
I originally cloned the dev branch thinking I'd push changes to that :/ so if I submit a new pull request on your master branch it will include other modifications on that file I did not make. Do you want me to go ahead with that or clone the master branch and make my changes? |
|
It is fine, we can bring the change to dev branch. The only thing - I can't tell when the fix will make it to a release and package. |
WillAyd
reviewed
Dec 16, 2019
tabpy/tabpy_tools/client.py
Outdated
| @@ -22,14 +22,13 @@ def _check_endpoint_type(name): | |||
|
|
|||
| def _check_hostname(name): | |||
| _check_endpoint_type(name) | |||
| hostname_checker = compile( | |||
| r'^http(s)?://[a-zA-Z0-9-_\.]+(/)?(:[0-9]+)?(/)?$') | |||
| hostname_checker = compile(r'^http(s)?://[\w.-]+(/)?(:\d+)?(/)?$') | |||
There was a problem hiding this comment.
Just out of curiosity is there a reason to use this regex instead of urllib.parse?
https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urlparse
There was a problem hiding this comment.
That's a good idea. It could add more functionality if needed but it's probably just regex under the hood right?
There was a problem hiding this comment.
I don't know the details but can find here:
unless needed to roll separately here I just think a better idea to use built-ins. One gap in this implementation is for ipv6 addresses, but ultimately up to the Tableau team
There was a problem hiding this comment.
I think we should prefer a dedicated URL parser/validator even if it is implemented with exactly the same regex.
0golovatyi
added a commit
that referenced
this pull request
Jan 14, 2020
* added anvoa to supported pre-deployed models in tabpy (#350) * added anvoa to supported pre-deployed models in tabpy * fixed pep8 issue * fixed md * Add Ctrl+C handler (#348) * Add Ctrl+C handler * Fix unit tests warnings for genson * Add test to increase code coverage * Add * Change default from 10Mb to 100Mb for request size * Increase code coverage * Increase code coverage * Convert buffer size to int * Add Ctrl+C test * Delete test added to the wrong folder * Update CHANGELOG * Update test_app.py * Remove dead code * Don't count coverage for multiline expressions * Add test case for invalid protocol * Add test case for _check_endpoint_name * Remove dead code * Fix vulnerabilities found by LGTM (#361) * Fix vulnerabilities found by LGTM * Fix test failures * Dev improvements (#384) * Fix flake8 warnings * Merge from master * Fix pycodestyle * Fix more flake8 warnings * Fix tests to pass again * Create test_coveralls_codestyle.yml (#382) * Use github actions * Documentation improvements (#385) * Delete .travis.yml * Fix Ctrl+C failing on extra parameter in signal handler * Remove outdated documentation for how to configure connection * tabpy.py to use docopt * Update tabpy-user with docopt * Update CHANGELOG * Fix code style * Change regex, add remove method, and edit qeury_timeout (#375) * Added Client.remove method to delete deployed model * Fix bug for query_timeout types * Update CHANGELOG * Fix missing script result/return bug * Fix github workflow for push * Code improvement: app._parse_config (#391) * Update app.py * v0.8.10 (#392) * Collect test coverage with scrutinizer instead of coveralls * Restore coverage collecting with coveralls * Update scrutinizer settings * Add support for Python 3.8 * Fix static page and add unit test for it * Delete obsolete test * Dev cov (#394) * Collect test coverage with scrutinizer instead of coveralls * Restore coverage collecting with coveralls * Update scrutinizer settings * Add support for Python 3.8 * Fix static page and add unit test for it * Delete obsolete test * Restore scrutinizer configuration * Linting as separate build step * Restore scrutinizer configuration * Update .scrutinizer.yml * Update .scrutinizer.yml * Restore scrutinizer configuration * Update pull_request.yml * Code style improvements * Code style improvements * Code style improvements Co-authored-by: sbabayan <34922408+sbabayan@users.noreply.github.com> Co-authored-by: Brennan Bugbee <Bugbeeb@gmail.com>
0golovatyi
added a commit
that referenced
this pull request
Jul 29, 2020
* added anvoa to supported pre-deployed models in tabpy (#350) * added anvoa to supported pre-deployed models in tabpy * fixed pep8 issue * fixed md * Add Ctrl+C handler (#348) * Add Ctrl+C handler * Fix unit tests warnings for genson * Add test to increase code coverage * Add * Change default from 10Mb to 100Mb for request size * Increase code coverage * Increase code coverage * Convert buffer size to int * Add Ctrl+C test * Delete test added to the wrong folder * Update CHANGELOG * Update test_app.py * Remove dead code * Don't count coverage for multiline expressions * Add test case for invalid protocol * Add test case for _check_endpoint_name * Remove dead code * Fix vulnerabilities found by LGTM (#361) * Fix vulnerabilities found by LGTM * Fix test failures * Dev improvements (#384) * Fix flake8 warnings * Merge from master * Fix pycodestyle * Fix more flake8 warnings * Fix tests to pass again * Create test_coveralls_codestyle.yml (#382) * Use github actions * Documentation improvements (#385) * Delete .travis.yml * Fix Ctrl+C failing on extra parameter in signal handler * Remove outdated documentation for how to configure connection * tabpy.py to use docopt * Update tabpy-user with docopt * Update CHANGELOG * Fix code style * Change regex, add remove method, and edit qeury_timeout (#375) * Added Client.remove method to delete deployed model * Fix bug for query_timeout types * Update CHANGELOG * Fix missing script result/return bug * Fix github workflow for push * Code improvement: app._parse_config (#391) * Update app.py * v0.8.10 (#392) * Collect test coverage with scrutinizer instead of coveralls * Restore coverage collecting with coveralls * Update scrutinizer settings * Add support for Python 3.8 * Fix static page and add unit test for it * Delete obsolete test * Dev cov (#394) * Collect test coverage with scrutinizer instead of coveralls * Restore coverage collecting with coveralls * Update scrutinizer settings * Add support for Python 3.8 * Fix static page and add unit test for it * Delete obsolete test * Restore scrutinizer configuration * Linting as separate build step * Restore scrutinizer configuration * Update .scrutinizer.yml * Update .scrutinizer.yml * Restore scrutinizer configuration * Update pull_request.yml * Code style improvements * Code style improvements * Code style improvements * Add coverall workflow * Initial checkin, working tests. * Made common base class for server info tests. * pep8 checks. * Added documentation for TABPY_AUTH_INFO * Fix spacing. * Refactor config parsing to allow custom parsers. (#412) * Refactor config parsing to allow custom parsers. * Fix pep8 * Update version and changelog. * Changed default for tabpy_auth_info to a boolean. * Secure info (#414) * Update README.md * Doc update (#402) * Fixed broken link * Linked to install doc. * Dev fix spelling (#408) * Add spelling fix workflow * Refactor config parsing to allow custom parsers. * Fix pep8 * Update version and changelog. * Changed default for tabpy_auth_info to a boolean. * Remove configuration for securing the info API and make it secure by default. * Revert "Merge branch 'master' into secureInfo" This reverts commit 3688561, reversing changes made to a4acc65. * Removed auth config from uni test. Co-authored-by: nmannheimer <nathanmannheimer@gmail.com> Co-authored-by: Oleksandr Golovatyi <olekg@outlook.com> * Secure info (#417) * Update README.md * Doc update (#402) * Fixed broken link * Linked to install doc. * Dev fix spelling (#408) * Add spelling fix workflow * Refactor config parsing to allow custom parsers. * Fix pep8 * Update version and changelog. * Changed default for tabpy_auth_info to a boolean. * Remove configuration for securing the info API and make it secure by default. * Revert "Merge branch 'master' into secureInfo" This reverts commit 3688561, reversing changes made to a4acc65. * Removed auth config from uni test. * Removed example for removed setting. * Remove unused example config. Co-authored-by: nmannheimer <nathanmannheimer@gmail.com> Co-authored-by: Oleksandr Golovatyi <olekg@outlook.com> * Clean up API documentation (#420) * Update README.md * Doc update (#402) * Fixed broken link * Linked to install doc. * Dev fix spelling (#408) * Add spelling fix workflow * Update LICENSE update to Tableau Software LLC * v 1.1.0: Secure /info with auth (#415) - Authorization is now required for the /info API method. This method did not check authentication previously. This change is backwards compatible with Tableau clients. - Improved config parsing flexibility. Previously the TABPY_EVALUATE_TIMEOUT setting would be set to a default if tabpy couldn't parse the value. Now it will throw an exception at startup. * Clean up API documentation * Clean up API documentation * Clean up API documentation Co-authored-by: nmannheimer <nathanmannheimer@gmail.com> Co-authored-by: lriggs <lriggs@tableau.com> Co-authored-by: Olek Golovatyi <ogolovatyi@tableau.com> * Code improvements (#431) * Rename tabpy_server and tabpy_tools to server and tools (breaking change) * fix flake8 warnings * Clean up code to reduce number of conditions * Remove pypi publishing instructions - those are Tableau specific * Restore tabpy_tools and tabpy_server names * Restore tabpy_tools and tabpy_server names * Restore tabpy_tools and tabpy_server names * Unit and integration tests passing * Update .gitignore * do not track settings.json for VSCode * Fix server -> tabpy_server * more cleaning for tabpy_server names * make "python setup.py test" work * add coverage module as required * delete tests node for scrutinizer run Co-authored-by: sbabayan <34922408+sbabayan@users.noreply.github.com> Co-authored-by: ogolovatyi <ogolovatyi@OGOLOVATYI.tsi.lan> Co-authored-by: Brennan Bugbee <Bugbeeb@gmail.com> Co-authored-by: Logan Riggs <lriggs@tableau.com> Co-authored-by: Olek Golovatyi <ogolovatyi@tableau.com> Co-authored-by: nmannheimer <nathanmannheimer@gmail.com>
0golovatyi
added a commit
that referenced
this pull request
Aug 14, 2020
* added anvoa to supported pre-deployed models in tabpy (#350) * added anvoa to supported pre-deployed models in tabpy * fixed pep8 issue * fixed md * Add Ctrl+C handler (#348) * Add Ctrl+C handler * Fix unit tests warnings for genson * Add test to increase code coverage * Add * Change default from 10Mb to 100Mb for request size * Increase code coverage * Increase code coverage * Convert buffer size to int * Add Ctrl+C test * Delete test added to the wrong folder * Update CHANGELOG * Update test_app.py * Remove dead code * Don't count cover A874 age for multiline expressions * Add test case for invalid protocol * Add test case for _check_endpoint_name * Remove dead code * Fix vulnerabilities found by LGTM (#361) * Fix vulnerabilities found by LGTM * Fix test failures * Dev improvements (#384) * Fix flake8 warnings * Merge from master * Fix pycodestyle * Fix more flake8 warnings * Fix tests to pass again * Create test_coveralls_codestyle.yml (#382) * Use github actions * Documentation improvements (#385) * Delete .travis.yml * Fix Ctrl+C failing on extra parameter in signal handler * Remove outdated documentation for how to configure connection * tabpy.py to use docopt * Update tabpy-user with docopt * Update CHANGELOG * Fix code style * Change regex, add remove method, and edit qeury_timeout (#375) * Added Client.remove method to delete deployed model * Fix bug for query_timeout types * Update CHANGELOG * Fix missing script result/return bug * Fix github workflow for push * Code improvement: app._parse_config (#391) * Update app.py * v0.8.10 (#392) * Collect test coverage with scrutinizer instead of coveralls * Restore coverage collecting with coveralls * Update scrutinizer settings * Add support for Python 3.8 * Fix static page and add unit test for it * Delete obsolete test * Dev cov (#394) * Collect test coverage with scrutinizer instead of coveralls * Restore coverage collecting with coveralls * Update scrutinizer settings * Add support for Python 3.8 * Fix static page and add unit test for it * Delete obsolete test * Restore scrutinizer configuration * Linting as separate build step * Restore scrutinizer configuration * Update .scrutinizer.yml * Update .scrutinizer.yml * Restore scrutinizer configuration * Update pull_request.yml * Code style improvements * Code style improvements * Code style improvements * Add coverall workflow * Initial checkin, working tests. * Made common base class for server info tests. * pep8 checks. * Added documentation for TABPY_AUTH_INFO * Fix spacing. * Refactor config parsing to allow custom parsers. (#412) * Refactor config parsing to allow custom parsers. * Fix pep8 * Update version and changelog. * Changed default for tabpy_auth_info to a boolean. * Secure info (#414) * Update README.md * Doc update (#402) * Fixed broken link * Linked to install doc. * Dev fix spelling (#408) * Add spelling fix workflow * Refactor config parsing to allow custom parsers. * Fix pep8 * Update version and changelog. * Changed default for tabpy_auth_info to a boolean. * Remove configuration for securing the info API and make it secure by default. * Revert "Merge branch 'master' into secureInfo" This reverts commit 3688561, reversing changes made to a4acc65. * Removed auth config from uni test. Co-authored-by: nmannheimer <nathanmannheimer@gmail.com> Co-authored-by: Oleksandr Golovatyi <olekg@outlook.com> * Secure info (#417) * Update README.md * Doc update (#402) * Fixed broken link * Linked to install doc. * Dev fix spelling (#408) * Add spelling fix workflow * Refactor config parsing to allow custom parsers. * Fix pep8 * Update version and changelog. * Changed default for tabpy_auth_info to a boolean. * Remove configuration for securing the info API and make it secure by default. * Revert "Merge branch 'master' into secureInfo" This reverts commit 3688561, reversing changes made to a4acc65. * Removed auth config from uni test. * Removed example for removed setting. * Remove unused example config. Co-authored-by: nmannheimer <nathanmannheimer@gmail.com> Co-authored-by: Oleksandr Golovatyi <olekg@outlook.com> * Clean up API documentation (#420) * Update README.md * Doc update (#402) * Fixed broken link * Linked to install doc. * Dev fix spelling (#408) * Add spelling fix workflow * Update LICENSE update to Tableau Software LLC * v 1.1.0: Secure /info with auth (#415) - Authorization is now required for the /info API method. This method did not check authentication previously. This change is backwards compatible with Tableau clients. - Improved config parsing flexibility. Previously the TABPY_EVALUATE_TIMEOUT setting would be set to a default if tabpy couldn't parse the value. Now it will throw an exception at startup. * Clean up API documentation * Clean up API documentation * Clean up API documentation Co-authored-by: nmannheimer <nathanmannheimer@gmail.com> Co-authored-by: lriggs <lriggs@tableau.com> Co-authored-by: Olek Golovatyi <ogolovatyi@tableau.com> * Code improvements (#431) * Rename tabpy_server and tabpy_tools to server and tools (breaking change) * fix flake8 warnings * Clean up code to reduce number of conditions * Remove pypi publishing instructions - those are Tableau specific * Restore tabpy_tools and tabpy_server names * Restore tabpy_tools and tabpy_server names * Restore tabpy_tools and tabpy_server names * Unit and integration tests passing * Update .gitignore * do not track settings.json for VSCode * Fix server -> tabpy_server * more cleaning for tabpy_server names * make "python setup.py test" work * add coverage module as required * delete tests node for scrutinizer run * Update postman collection * remove print from error handling code (#439) * remove print from error handling code * remove & for linux cmd Co-authored-by: Olek Golovatyi <ogolovatyi@tableau.com> * Return HTTP 400 status when receiving a request with authentication credentials and authN is not configured (#440) * added 400 bad request response to event when authorization is not set up but user sends username and password * added unit tests to test what happens when there are credentials, but no authentication is required * renamed not_authorized flag to authentication_error flag * changed line formatting * changed the way auth error is handled * Version to 2.0.0 * Version to 2.0.0 * Fix codystyle warnings * Restore scrutinizer settings * Fix codestyle Co-authored-by: sbabayan <34922408+sbabayan@users.noreply.github.com> Co-authored-by: ogolovatyi <ogolovatyi@OGOLOVATYI.tsi.lan> Co-authored-by: Brennan Bugbee <Bugbeeb@gmail.com> Co-authored-by: Logan Riggs <lriggs@tableau.com> Co-authored-by: Olek Golovatyi <ogolovatyi@tableau.com> Co-authored-by: nmannheimer <nathanmannheimer@gmail.com> Co-authored-by: harold-xi <69485232+harold-xi@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Unit tests all passed. I modified your regex to be more compact and made some updates to docstrings. It also looked like the query_timeout attribute was not being set correctly because it was initially set to self but in the property wrapper it was being set to self._service and the latter seems correct. I also made the query_timeout parameter more robust by filtering for argument type before setting. In a previous issue, someone brought up that there was no remove endpoint function so I added one. It works: '[INFO] (web.py:web:2106): 204 DELETE /endpoints/Foo (127.0.0.1) 9.97ms', but it appears to cause a server-side error: '[ERROR] (base_handler.py:base_handler:120): Error submitting update model request: error=AttributeError : 'ContextLoggerWrapper' object has no attribute 'info''. However, client.get_status() is resolved when the server is restarted.