Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

schemas: add geojson support #21

Merged
merged 2 commits into from Nov 10, 2020
Merged

Conversation

ppanero
Copy link
Member

@ppanero ppanero commented Oct 26, 2020

[ci skip]

Skipped tests until reviewed cuz Travis...

As discussed with @slint this PR adds support for Point, MultiPoint and Polygon. Since DataCite supports geoLocationPoint, geoLocationBox, geoLocationPolygon.

These schemas have been implemented using python geojson library. In order to provide support for multiple schemas (i.e. the coordinates is different depending on the object type) marshmallow-oneofschema is used. marshmallow-polyschema was evaluated.

Enables the start of inveniosoftware/invenio-rdm-records#231 in Invenio-RDM-Records

Copy link
Member

@slint slint left a comment

Choose a reason for hiding this comment

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

LGTM overall, good job 🔥

Some discussions to be made regarding which parts of GeoJSON to be included and how to integrate extra metadata fields better.

optional third element.
"""
if not data:
raise ValidationError({"geojson": "coordinates must be present."})
Copy link
Member

Choose a reason for hiding this comment

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

Isn't that handled by the required=True above on the coordinates field?

Also I think passing a dict to the ValidationError hides the real field that has the error (unless we want to establish a convention for exposing GeoJSON error under a specific key)

Copy link
Member Author

@ppanero ppanero Oct 27, 2020

Choose a reason for hiding this comment

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

Isn't that handled by the required=True above on the coordinates field?

Correct, it was legacy code. First I tried to have a single "validates_schema" on the GeoJSONSchema, and a simple attr geojson_cls. Because as you can see the validation function is the same except for the geojson object they invoke for validation. In this case, this check was needed. However, I did not manage to make it work, for some reason the validation decorator was not triggered (even though OneOfSchema inherits from Schema...). Some issues are open also about post_load and company not being triggered.

Proposal: Open an issue, and address this (refactor validation) when we add more fields/schemas. (Already removed the unnecessary check).

Also I think passing a dict to the ValidationError hides the real field that has the error (unless we want to establish a convention for exposing GeoJSON error under a specific key)

My idea was to actually expose GeoJSON since we are validating with it... would be nice to tell the user that in a meaningful way.

"type": "point",
"coordinates": [-32.94682]
}
pytest.raises(ValidationError, GeoJSONSchema().load, invalid_input)
Copy link
Member

Choose a reason for hiding this comment

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

I would also test in general for the shape/kind of data the ValidationErrors have

Copy link
Member Author

Choose a reason for hiding this comment

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

Can do that once we agreed on the above point. Was also thinking if we should capture/re-raise exceptions to enable translation on strings... (would make this module dependent on babel and company). Maybe it's better to do it on rdm-records level?

@ppanero
Copy link
Member Author

ppanero commented Oct 27, 2020

Working on adding dump tests Done.
Letting travis run

@ppanero ppanero closed this Oct 27, 2020
InvenioRDM October Board automation moved this from In Review to Done Oct 27, 2020
@ppanero ppanero reopened this Oct 27, 2020
@ppanero ppanero moved this from Done to In Review in InvenioRDM October Board Oct 27, 2020
@lnielsen lnielsen moved this from In Review to Pending merge in InvenioRDM October Board Nov 3, 2020
@lnielsen lnielsen merged commit ddec07b into inveniosoftware:master Nov 10, 2020
InvenioRDM October Board automation moved this from Pending merge to Done Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants