Skip to content

Conversation

mongkok
Copy link
Contributor

@mongkok mongkok commented Feb 4, 2018

the PR convert Geometry fields to well-known text (WKT):

{
    "id": "1",
    "location": "SRID=4326;POINT (0 1)"
}

@coveralls
Copy link

coveralls commented Feb 4, 2018

Coverage Status

Coverage increased (+0.01%) to 94.215% when pulling 1d3bdad on mongkok:convert-geometry-field into c585982 on graphql-python:master.

@coveralls
Copy link

coveralls commented Feb 4, 2018

Coverage Status

Coverage increased (+0.01%) to 94.366% when pulling ec73e91 on mongkok:convert-geometry-field into 6ce59ae on graphql-python:master.

@zbyte64
Copy link
Collaborator

zbyte64 commented Oct 15, 2018

In my own project using graphql I prefer having GeometryField to not serialize as a string but a generic scalar because we're using geojson and the client doesn't bother (de)serializing json but gets direct objects.

Aside from that, we probably shouldn't be importing django.contrib.gis by default.

Here is what I have for reference:

class GeoJSON(GenericScalar):
    @staticmethod
    def geos_to_json(value):
        return json.loads(GEOSGeometry(value).geojson)

    @staticmethod
    def json_to_geos(value):
        return GEOSGeometry(value)

    serialize = geos_to_json
    deserialize = json_to_geos


@convert_django_field.register(models.PolygonField)
@convert_django_field.register(models.MultiPolygonField)
def convert_geofield_to_string(field, registry=None):
     return GeoJSON(description=field.help_text, required=not field.null)


@convert_form_field.register(fields.PolygonField)
@convert_form_field.register(fields.MultiPolygonField)
def convert_geofield_to_string(field, registry=None):
     return GeoJSON(description=field.help_text, required=field.required)

Copy link
Collaborator

@danpalmer danpalmer left a comment

Choose a reason for hiding this comment

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

I think I'm inclined to agree with @zbyte64. In general I think custom scalars for complex data like this are more useful. Converting this to a string would require more processing on the client, and if the client is a web frontend using a mapping tool, that might be unnecessarily complex.

Serialising to some generic JSON, or even to a custom type that is represented "on the wire" with an object structure feels like it will be simpler for more clients to use, and also plays to GraphQL's strengths with its use of the type system, particularly with a custom type.

If we do go down one of these two routes, which I am strongly in favour of, I think we should document this somewhere, perhaps in a new document describing how Graphene treats complex type (such as the IP addresses, File fields, etc that have default converters).

If we don't think that specific types are the correct way to handle this, I'd suggest that we may want to not provide this at all, forcing users to implement custom scalars. Our documentation around this could be better, but in general I think more custom scalars create more type safety, and are a good thing.

@jkimbo
Copy link
Member

jkimbo commented Mar 31, 2019

I don't know enough about GeometryField's in Django to be able to contribute here but I would say that (as @danpalmer mentions) generally creating customer scalar fields that are typed to match the underlying data model is a good thing. I'm not sure whether it is up to graphene-django to define what that type should look like this case but I don't see why not since GeometryField is part of Django.

@jkimbo jkimbo removed their request for review March 31, 2019 11:24
@stale
Copy link

stale bot commented Jun 11, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jun 11, 2019
@stale stale bot closed this Jun 18, 2019
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.

6 participants