search

fgpv-vpgf / rcs

RAMP Configuration Service
http://fgpv-vpgf.github.io/rcs
1 stars 8 forks source link

feat(rcs): Added v2 Update endpoint #58

Closed dan-bowerman closed 7 years ago

dan-bowerman commented 7 years ago

RCS v2 now supports the ability to update specific fields in a previous registration, without re-registering wholesale.

Closes #9

This change is Reviewable

alyec commented 7 years ago

Reviewed 5 of 5 files at r1, 1 of 1 files at r2. Review status: all files reviewed at latest revision, 3 unresolved discussions.

_services/update.py, line 28 at r2 (raw file):_

                return '{"msg":"Record not found in database"}', 404
        except Exception as e:
            msg = {'msg': 'Error: {0}'.format(e.message)}

This is probably not something the client has control over since it's a general exception (e.g. database is down). 4xx errors should be used if the client can change something to get a proper response but this should probably be a 500 error and maybe we don't want to pass the message along.

services/update.py, line 34 at r2 (raw file):

            payload = json.loads(request.data)
            for x in langs:
                # The generic fields to update

Might be easier to add a small helper:

request_seg = dbdata['request'][x]
payload_seg = payload[x]

def replace_if_set(params):
  for p in params:
    request_segment[p] = payload_seg[p]

replace_if_set('service_url service_name metadata'.split())
if payload_seg['service_type'] in ['esriFeature', 'esriImage', 'esriTile']:
  replace_if_set(['display_field'])
elif payload_seg['service_type'] == 'esriMapServer':
  replace_if_set(['scrape_only', 'recursive'])
elif payload_seg['service_type'] == 'ogcWms':
  replace_if_set(['scrape_only', 'recursive', 'legend_format', 'feature_info_format'])

services/update.py, line 64 at r2 (raw file):

            db.put_doc(key, payload[x]["service_type"], dbdata["request"], layer_config=v2_node, v1_config=v1_node)
        except Exception as e:
            msg = {'msg': 'Error: {0}'.format(e.message)}

Same issue as above with respect to the exception. If there is a way we can identify genuine bad requests (400s as opposed to 500s) we should try.

Comments from Reviewable

james-rae commented 7 years ago

Reviewed 4 of 5 files at r1, 4 of 4 files at r3. Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

alyec commented 7 years ago

Reviewed 3 of 4 files at r3. Review status: all files reviewed at latest revision, 4 unresolved discussions.

[services/regparse/ogc.py, line 72 at r3](https://reviewable.io:443/reviews/fgpv-vpgf/rcs/58#-KUSBWX1YFOF4BlkyMB:-KUSBWX2J2V8v9vNyB3S:ba2mfg) (raw file):_

    :type str
    """
    xmldoc = minidom.parseString(query_service)

Since XML parsing is expensive it's probably better to scrape all the data upfront.

def parseCapabilities(capabilties_xml_string):
  ret = {}
  xmldoc = minidom.parseString(capabilties_xml_string)
  for layer in xmldoc.getElementsByTagName('Layer'):
    id = layer..getElementsByTagName('Name')[0].firstChild.data
    title = layer.getElementsByTagName('Title')[0].firstChild.data
    queryable = str2bool(layer.getAttribute('queryable'))
    ret[id] = dict(title=title, queryable=queryable)
  return ret

in make_wms_node you could call this immediately after the capabilities call and use if for both scrape_only and recursive

Comments from Reviewable

dan-bowerman commented 7 years ago

Review status: 7 of 8 files reviewed at latest revision, 4 unresolved discussions.

_services/update.py, line 28 at r2 (raw file):_

Previously, alyec (Aly Merchant) wrote… > This is probably not something the client has control over since it's a general exception (e.g. database is down). `4xx` errors should be used if the client can change something to get a proper response but this should probably be a `500` error and maybe we don't want to pass the message along. >

Done.

services/update.py, line 34 at r2 (raw file):

Previously, alyec (Aly Merchant) wrote… > Might be easier to add a small helper: > > ``` python > request_seg = dbdata['request'][x] > payload_seg = payload[x] > > def replace_if_set(params): > for p in params: > request_segment[p] = payload_seg[p] > > replace_if_set('service_url service_name metadata'.split()) > if payload_seg['service_type'] in ['esriFeature', 'esriImage', 'esriTile']: > replace_if_set(['display_field']) > elif payload_seg['service_type'] == 'esriMapServer': > replace_if_set(['scrape_only', 'recursive']) > elif payload_seg['service_type'] == 'ogcWms': > replace_if_set(['scrape_only', 'recursive', 'legend_format', 'feature_info_format']) > ``` > >

Done.

services/update.py, line 64 at r2 (raw file):

Previously, alyec (Aly Merchant) wrote… > Same issue as above with respect to the exception. If there is a way we can identify genuine bad requests (`400`s as opposed to `500`s) we should try. >

Done.

[services/regparse/ogc.py, line 72 at r3](https://reviewable.io:443/reviews/fgpv-vpgf/rcs/58#-KUSBWX1YFOF4BlkyMB:-KUSVyIizJ3mx0gHhXN:b-896fix) (raw file):

Previously, alyec (Aly Merchant) wrote… > Since XML parsing is expensive it's probably better to scrape all the data upfront. > > ``` python > def parseCapabilities(capabilties_xml_string): > ret = {} > xmldoc = minidom.parseString(capabilties_xml_string) > for layer in xmldoc.getElementsByTagName('Layer'): > id = layer..getElementsByTagName('Name')[0].firstChild.data > title = layer.getElementsByTagName('Title')[0].firstChild.data > queryable = str2bool(layer.getAttribute('queryable')) > ret[id] = dict(title=title, queryable=queryable) > return ret > ``` > > in `make_wms_node` you could call this immediately after the capabilities call and use if for both `scrape_only` and `recursive` >

Done.

Comments from Reviewable

alyec commented 7 years ago

Reviewed 1 of 1 files at r4. Review status: all files reviewed at latest revision, 1 unresolved discussion.

services/update.py, line 34 at r2 (raw file):

Previously, dan-bowerman (Dan Bowerman) wrote… > Done. >

I don't see this change, did you forget a commit?

Comments from Reviewable

dan-bowerman commented 7 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion.

services/update.py, line 34 at r2 (raw file):

Previously, alyec (Aly Merchant) wrote… > I don't see this change, did you forget a commit? >

I will roll this into another issue and take care of it later. I need to get this release out soon so I can focus on ECDMP and JOSM for the next few weeks.

Comments from Reviewable

alyec commented 7 years ago

Reviewed 1 of 4 files at r3. Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable

james-rae commented 7 years ago

Reviewed 1 of 1 files at r4. Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable