geopython / pywps

PyWPS is an implementation of the Web Processing Service standard from the Open Geospatial Consortium. PyWPS is written in Python.
https://pywps.org
MIT License
178 stars 117 forks source link

Fix deserialization #456

Closed davidcaron closed 5 years ago

davidcaron commented 5 years ago

Overview

I had trouble with the serialization-deserialization of a ComplexInput being a url link to a file.

One of the issue is that the file property should be saved for a ComplexInput that has a prop attribute of url.

I'm not 100% sure that it should be fixed this way, but it fixes the case of the url type and I don't think that other types are impacted.

Also, when deserializing the WPSRequest, I believe the inputs should be from pywps.inout.inputs instead of pywps.inout.basic. The docstring mentions that the latter are abstract types.

Related Issue / Discussion

Additional Information

Contribution Agreement

(as per https://github.com/geopython/pywps/blob/master/CONTRIBUTING.rst#contributions-and-licensing)

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.7%) to 75.226% when pulling baf3972b12daa881f37132bfb043afb06b5bd048 on davidcaron:fix-deserialization into 034b185c5e3b521d4a6a30fef118c6ab368567a6 on geopython:master.

cehbrecht commented 5 years ago

@davidcaron Could you add a test to show the "serialization-deserialization" issue of a ComplexInput?

davidcaron commented 5 years ago

@cehbrecht sure, I will add tests when I get some time.

davidcaron commented 5 years ago

I moved json serialization methods for the inputs from WPSRequest.json to inout.inputs so that they are close to each other. I put them in inout.inputs instead of inout.basic because ComplexInput.json was already there and json methods for outputs are in inout.outputs, not in inout.basic.

I had to fix a circular dependency between inout.formats and validator.complexvalidator. In all cases where Format is instantiated, the validate property is set. So this shouldn't break existing code. I prefer this solution to having a delayed import...

I fixed other things I found along the way, making this PR larger that it was originally.

@huard , please check https://github.com/geopython/pywps/pull/456/commits/ced88fbcb4bd082de9c8aa91f2158e5a67e91013 I think this would cover all cases of ComplexInput.

davidcaron commented 5 years ago

Codacy is giving me "Dangerous default value [] as argument" but this happens a lot elsewhere. I agree this should be changed, but maybe not in this PR...

I think this should really be changed, because this can lead to very hard to find bugs:

class Test:
    def __init__(self, some_list=[]):
        self.some_list = some_list

t1 = Test()
t2 = Test()

t2.some_list.append(1)
print(t1.some_list)  # prints [1]
huard commented 5 years ago

Does the Emu test suite also pass with those changes ? I've catched some issues in the past that we're missed by the pywps test suite.

cehbrecht commented 5 years ago

I wasn't really aware of this issue ... thought it would be just coding style. We should open an issue and fix it in a new PR, maybe like this:

def __init__(self, some_list=None):
    self.some_list = some_list or []

Codacy is giving me "Dangerous default value [] as argument" but this happens a lot elsewhere. I agree this should be changed, but maybe not in this PR...

I think this should really be changed, because this can lead to very hard to find bugs:

class Test:
    def __init__(self, some_list=[]):
        self.some_list = some_list

t1 = Test()
t2 = Test()

t2.some_list.append(1)
print(t1.some_list)  # prints [1]
cehbrecht commented 5 years ago

@davidcaron @huard I have checked this PR branch with Emu and there are issues at least with the bbox:

'BoundingBoxOutput' object has no attribute 'json'

Just run the getcapabilities request.

Does the Emu test suite also pass with those changes ? I've catched some issues in the past that we're missed by the pywps test suite.

davidcaron commented 5 years ago

@cehbrecht I think this should fix it. I had not touched the code for the outputs, but inputs.BoundingBoxOutput was inheriting from basic.BBoxInput.

cehbrecht commented 5 years ago

@huard @davidcaron If I heare a "go" by you and no complains by others I can merge :)

davidcaron commented 5 years ago

I'm done, feel free to @ me if you notice any related bug after the merge.

davidcaron commented 5 years ago

Sorry, I just realized the Metadata class needed serialization methods also

cehbrecht commented 5 years ago

@davidcaron done. Thanks :)

huard commented 5 years ago

@davidcaron I believe this PR introduced a bug with the describeprocess response. Below is an excerpt of a LiteralData element. Note that the value is a string representation of a property, instead of the property value.

<LiteralData xmlns:wps="http://www.opengis.net/wps/1.0.0" xmlns:ows="http://www.opengis.net/ows/1.1" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
                <ows:DataType ows:reference="http://www.w3.org/TR/xmlschema-2/#float">float</ows:DataType>
                <ows:AllowedValues>
                    <ows:Value>&lt;property object at 0x7febfb256ea8&gt;</ows:Value>
                </ows:AllowedValues>
                <DefaultValue>2.0</DefaultValue>
                </LiteralData>

I've hit this using the birdy test suite.