Skip to content

#587 pass HTTP headers for WMS,WFS,WMTS,TMS + fix WFS3 test#613

Merged
cehbrecht merged 4 commits into
geopython:masterfrom
justb4:issue-587-headers
Oct 17, 2019
Merged

#587 pass HTTP headers for WMS,WFS,WMTS,TMS + fix WFS3 test#613
cehbrecht merged 4 commits into
geopython:masterfrom
justb4:issue-587-headers

Conversation

@justb4
Copy link
Copy Markdown
Member

@justb4 justb4 commented Oct 9, 2019

  • WMS was missing HTTP header passing for v 1.1.1.
  • added WFS,WMTS,TMS HTTP header passing (was absent)
  • WFS3 Test needed small fix because of recent commit for limit param

@justb4 justb4 mentioned this pull request Oct 10, 2019
@cehbrecht
Copy link
Copy Markdown
Contributor

Fix for issue #587.

@cehbrecht cehbrecht requested a review from tomkralidis October 17, 2019 11:29
@cehbrecht
Copy link
Copy Markdown
Contributor

@justb4 I'm ok with this PR. Please solve the conflict due to updates.

@tomkralidis Any objections? Can I merge when conflict is solved?

Copy link
Copy Markdown
Member

@tomkralidis tomkralidis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we able to update also the CSW and WPS support with this functionality?

assert lakes['title'] == 'Large Lakes'
assert lakes['description'] == 'lakes of the world, public domain'

# Minimum of limit param is 1
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note these fixes are already in master via 9f7bf60

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, will integrate.

@justb4
Copy link
Copy Markdown
Member Author

justb4 commented Oct 17, 2019

Are we able to update also the CSW and WPS support with this functionality?

Yes, I am aware that these and others also need the same fix:

  • CSW: implementation is quite intricate. A central _invoke() though where we should pass headers to openURL() and http_post() (latter needs to merge headers).
  • WPS: looks from the code that headers are passed in OK.
  • SOS: needs similar fix as WFS
  • WCS: needs similar fix as WFS (in 5 WCS-versions!)

IMO 2 options:

  1. merge this PR and open new for WPS (if needed), SOS WCS
  2. add WPS (if needed), SOS WCS to this PR

I would opt for 1.
NB above checks have failed because of WMTS test.

@justb4
Copy link
Copy Markdown
Member Author

justb4 commented Oct 17, 2019

Above Checks have failed because of a WMTS test failure, could this be lxml related, I use lxml==4.4.1? See below, did not see this error before, did not touch any of the involved code.
@tomkralidis ' commit from yesterday seems unrelated as well, strange. UPDATE: also fails without lxml installed.

=================================================================== short test summary info ====================================================================
SKIPPED [1] tests/test_wmts.py:46: ARCGIS WMTS service is unreachable
=========================================================== 1 failed, 1 skipped, 1 warnings in 6.00s ===========================================================
(owslib-3.7.1) nusa:justb4.git just$ python -m pytest tests/test_wmts.py 
===================================================================== test session starts ======================================================================
platform darwin -- Python 3.7.1, pytest-5.2.1, py-1.8.0, pluggy-0.13.0 -- /Users/just/.pyenv/versions/owslib-3.7.1/bin/python
cachedir: .pytest_cache
rootdir: /Users/just/project/owslib/justb4.git, inifile: tox.ini
plugins: cov-2.8.1
collected 2 items                                                                                                                                              

tests/test_wmts.py::test_wmts FAILED
tests/test_wmts.py::test_wmts_without_serviceprovider_tag SKIPPED

=========================================================================== FAILURES ===========================================================================
__________________________________________________________________________ test_wmts ___________________________________________________________________________
Traceback (most recent call last):
  File "/Users/just/project/owslib/justb4.git/tests/test_wmts.py", line 15, in test_wmts
    wmts = WebMapTileService(SERVICE_URL)
  File "/Users/just/project/owslib/justb4.git/owslib/wmts.py", line 188, in __init__
    self._buildMetadata(parse_remote_metadata)
  File "/Users/just/project/owslib/justb4.git/owslib/wmts.py", line 239, in _buildMetadata
    gather_layers(caps, None)
  File "/Users/just/project/owslib/justb4.git/owslib/wmts.py", line 232, in gather_layers
    parse_remote_metadata=parse_remote_metadata)
  File "/Users/just/project/owslib/justb4.git/owslib/wmts.py", line 716, in __init__
    tile_matrix_set_links = TileMatrixSetLink.from_elements(link_elements)
  File "/Users/just/project/owslib/justb4.git/owslib/wmts.py", line 655, in from_elements
    raise KeyError(msg)
KeyError: 'TileMatrixLimits with tileMatrix "1" already exists'

@cehbrecht
Copy link
Copy Markdown
Contributor

@justb4 the latest merge let to a conflict again.

The wps module has already the headers parameter. I'm fine with follow up PRs to add headers to the CSW module etc ...

The current test failures are not related to this PR. Need to treat them separately.

@justb4
Copy link
Copy Markdown
Member Author

justb4 commented Oct 17, 2019

Ok, conflict resolved (was due to my own PR #614, merged). Also added test_tms.py (was missing) with basic tests on URL/main layer from Dutch National SDI, so URL should be stable.
Still failing tests but test_tms.py should be ok.

@cehbrecht cehbrecht merged commit e96e5ea into geopython:master Oct 17, 2019
@cehbrecht
Copy link
Copy Markdown
Contributor

@justb4 merged :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants