graphhopper / directions-api-clients

API clients for various languages for the GraphHopper Directions API
https://docs.graphhopper.com/
Apache License 2.0
30 stars 36 forks source link

How to specify "details" in response? #21

Closed michaz closed 6 years ago

michaz commented 6 years ago

((What I'm writing here is assuming that path details are dynamic -- that the server can invent new types of path details, and the client does not have to be rebuilt to use them. I think that was the intention. If not, ignore me.))

I'm afraid we may have managed to produce an API which is unspecifiable in Swagger.

Path details look like this:

"details":{"r5_edge_id":[[0,1,2772083],[1,2,397628],[2,3,397635],[3,4,397632],[4,5,999487],[5,6,999485],[6,7,999482],[7,8,2772090]]}

This is an object which consists of dynamic properties. This is unsupported in Swagger, except if the object is entirely free-form, meaning that also the schema of the list of arrays int[3] is lost:

        "details": {
          "type": "object"
        }

Any suggestions? Shall we try and be more like "Swagger first" to prevent this?

michaz commented 6 years ago

Or am I getting this wrong and it was never meant to be dynamic, and all currently implemented path details should be listed in the spec as properties of details in the response, and as allowed values of details in the request?

karussell commented 6 years ago

Yes, it is intended to by dynamic at some point (for now we have a limited list)

Any suggestions? Shall we try and be more like "Swagger first" to prevent this?

I wouldn't bind us to limitations of Swagger (still a 'young' project with many limitations). Instead we should fix swagger (or at least create an issue) if this gets critical (gets dynamic).

boldtrn commented 6 years ago

I agree with @karussell. I think, accepting this as a swagger limitation (and maybe raise an issue there as well) is acceptable. If I understand you correctly, you can still use the client if we specify this as a free object. The client would then only return a map (or a similar structure) for the PathDetails, which I find acceptable for now.

michaz commented 6 years ago

So we are using Swagger not as a specification language, or as a framework that may contain useful guidelines about what may be usual or non-surprising ways to do something, but just as a tool to auto-generate clients if that happens to be possible, and we want to keep it that way? Okay.

michaz commented 6 years ago

Because I was already dreaming of using it to actually design the API, and generate the (only, real, live) documentation and such.

michaz commented 6 years ago

But if it is too limited for us, probably not.

boldtrn commented 6 years ago

Because I was already dreaming of using it to actually design the API, and generate the (only, real, live) documentation and such.

Yes I was dreaming of the same thing :). To be honest I find it easier to write normal documentation and to use swagger only to generate clients, but this does not mean that we shouldn't use Swagger as our documentation tool. I haven't followed the latest developments from Swagger, maybe the documentation part became a lot better?

The biggest problem for me is, if we cannot do something properly in Swagger, we cannot document it properly and then we cannot generate clients from it properly as well. This however does not mean that we shouldn't built a certain feature, if we believe that the way we specified it is good. Swagger cannot simply update their specification, so if we find an issue, we might have to wait for the new Swagger version to be released and is stable to implement a feature, etc.

I 100% support the idea of Swagger and I think these guys are doing an awesome job. And I think Swagger works perfectly well for simple CRUD APIs (maybe even to generate the server code). But some of our optimizations are just really hard to model in Swagger, for example "points_encoded" has been a big issue and I think it's still not possible to do this properly (See #1, and also the related issue here).

michaz commented 6 years ago

But some of our optimizations are just really hard to model in Swagger, for example "points_encoded" has been a big issue and I think it's still not possible to do this properly

I see. But couldn't that suggest that for, say, version 2, we should give Swagger (or another specification tool) a bigger role while designing the API?

Maybe if we would do that, the next version of the points_encoded feature would be realized differently. Say, with differently-named response fields for the two different data-types.

I am still not completely convinced that we know good practices about these kinds of things better than the tools.

boldtrn commented 6 years ago

But couldn't that suggest that for, say, version 2, we should give Swagger (or another specification tool) a bigger role while designing the API?

Yes I think it would be good to consider a tool like Swagger when we design the version 2 of the API. If we can create a compatible API.

Maybe if we would do that, the next version of the points_encoded feature would be realized differently. Say, with differently-named response fields for the two different data-types.

Yes, I think this also makes more sense from a consumer-perspective. One key that can contain different content might be more complex to grasp. On the other hand, recent trends with for example NoSQL databases also show that there is a use-case for this.

I am still not completely convinced that we know good practices about these kinds of things better than the tools.

Me neither :).