Skip to content
This repository has been archived by the owner on Dec 21, 2020. It is now read-only.

Force Points Encoded = false #1

Open
boldtrn opened this issue Jan 10, 2017 · 24 comments
Open

Force Points Encoded = false #1

boldtrn opened this issue Jan 10, 2017 · 24 comments

Comments

@boldtrn
Copy link
Member

boldtrn commented Jan 10, 2017

Currently, points_encoded has to be manually set to false, which is not optimal.

@karussell
Copy link
Member

karussell commented Jan 10, 2017

Related to this here? http://swagger.io/unlocking-the-spec-the-default-keyword/ or this swagger-api/swagger-core#1844

Will this work?

- name: calc_points    
   type: boolean  
   default: false

@karussell
Copy link
Member

Ah, ok. It won't work: Also, I think you're missing the point of the default value. The default value is one that's assumed by the receiving end (either the server or the client) if the sender does not provide a value for it - it's not what automatically should be filled.

@boldtrn
Copy link
Member Author

boldtrn commented Jan 10, 2017

Will this work?

Ah, ok. It won't work

Exactly :) That was my first try, as well. I also experimented with required=true and default=false, which does not work either. So right now I have the feeling, that swagger does not allow adding fixed values to the query, but maybe we find something.

@karussell
Copy link
Member

Maybe this answer somehow via enum: [false]?

@karussell
Copy link
Member

karussell commented Jan 10, 2017

And then there is “defaultValue”: “something” for the swagger UI ...

@boldtrn
Copy link
Member Author

boldtrn commented Jan 10, 2017

Thanks, I will check if either of your proposed approaches work.

@boldtrn
Copy link
Member Author

boldtrn commented Jan 11, 2017

Maybe this answer somehow via enum: [false]?

The enum seems to only work for the swagger UI. It did not changed the generated code, I was able to still put true as value. The client send true to the server.

And then there is “defaultValue”: “something” for the swagger UI ...

The same applies to the defaultValue.

@boldtrn
Copy link
Member Author

boldtrn commented Jan 11, 2017

I added a question to Stackoverflow.

Also I just had another idea. We could allow points_encoded=true and return the encoded string. That way a user has to decode the point on his/her own.

@karussell
Copy link
Member

Also I just had another idea. We could allow points_encoded=true and return the encoded string. That way a user has to decode the point on his/her own.

Well, both should be possible but I would like to avoid the necessity to decode it for a fresh user. Maybe we should just document that per default you get the encoded polyline and how to decode it in JS/Java, and if the user does not want this he can switch to the array variant (this "either/or" should be possible in swagger)

@boldtrn
Copy link
Member Author

boldtrn commented Jan 18, 2017

I've researched for a couple of while now and it seems like handling the points_encoded will be quite difficult. The reason is that swagger expects for a certain format for a certain key. In this example for points it would expect an array of coordinates:

      points:
        $ref: '#/definitions/GHResponseCoordinates'

Swagger supports the concept of polymorphism, but polymorphism requires a field with a string that contains the name of the of the type. See their examples and search for Models with Polymorphism Support. However, they announced to improve that with swagger spec version 3.0.

@karussell
Copy link
Member

What do you mean? Isn't there an 'optional'-like possibility?

@boldtrn
Copy link
Member Author

boldtrn commented Jan 19, 2017

What do you mean? Isn't there an 'optional'-like possibility?

Yes optional is allowed per default. What's not allowed easiy is to state something like:
points are either string or array.

@karussell
Copy link
Member

Hmmh, strange. Thought have seen this somewhere.

@karussell
Copy link
Member

karussell commented Jan 19, 2017

Swagger supports the concept of polymorphism, but polymorphism requires a field with a string that contains the name of the of the type

I've no swagger spec knowledge :) ...but that looks exactly what we need - why would the following not work?

..
allOf:
    - $ref: '#/definitions/PointsGeoJson'
    - type: string
...
PointsGeoJson:
    properties:
       coordinates:
           type: array
           ...

@boldtrn
Copy link
Member Author

boldtrn commented Jan 20, 2017

why would the following not work?

I think the idea of allOf is something different. I think it's about defining a common set of properties. For example: every response contains a message of type string. This would allow you to add the allOf to every Response and therefore need only to define a "CommonResponseData" object.

I tested it like this:

      points:
        allOf:
          - $ref: '#/definitions/GHResponseCoordinates'
          - type: string

The editor validates this as valid swagger, but when I generate the client the points entry is completely missing. So at least the swagger-codegen does not understand it.

I think what we look for is oneOf, which will be supported by OpenAPI 3.0

The current plan is for oneOf to be supported in OpenAPI 3.0 #741

See for example this issue. There are tons of requests for that.

I think we might be able to use Polymorphism. There is a swagger spec for GeoJson. I haven't tested it, but this should be what we need. But Polymorphism needs a type string, like: point, polygon etc. Swagger maps the object to the object that equals the type string. Not really elegant and we don't have something like this right now. We only have points_encoded: true|false. Not sure if we should name the objects true and false, and if this would even work. So eventually it would work if would return a point_type: PointsEncoded|PointsNotEncoded or something like this. Should I test this?

@karussell
Copy link
Member

Thanks for digging into this!

Should I test this?

Yes, please test if an additional helper string or boolean would work.

Or finding a solution (or an issue) for the default parameter could also help.

@boldtrn
Copy link
Member Author

boldtrn commented Jan 23, 2017

Ok I tested Polymorphism. After I worte the swagger I found out that Polymorphism is not supported for Java clients, yet (not released see my comment below). I think Polymorphism is supported for other clients. The problem is, we could not use the Routing API from certain languages.

Not supported languages:

It seems to be supported for PHP, Swift and ObjC.

I opened another issue to see if we can get an overview of the supported languages to make a reasonable decision: swagger-api/swagger-codegen#4622.

So in short: If we use this polymorphism approach not all clients will work. So the question is if we should follow this road or just support points_encoded=false? However, I still think there is no solution to force the parameter to false, which might lead to misuse of the client...

Some related issues:
swagger-api/swagger-codegen#3904
swagger-api/swagger-codegen#1253
swagger-api/swagger-codegen#1686
swagger-api/swagger-codegen#2449
swagger-api/swagger-codegen#1869

Anyway the swagger is like that:

  GHRouteResponsePath:
    type: object
    description: A found path
    discriminator: type
    required:
    - type
    properties:
      type:
        type: string
        enum:
        - PointsEncoded
        - Points
      distance:
        description: The total distance of the route, in meter
        type: number
        format: double
      time:
        description: The total time of the route, in ms
        type: integer
        format: int64
      ascend:
        type: number
        format: double
      descend:
        description: The total descend (downhill) of the route, in meter
        type: number
        format: double
      points_encoded:
        description: Is true if the points are encoded, if not paths[0].points contains the geo json of the path (then order is lon,lat,elevation), which is easier to handle but consumes more bandwidth compared to encoded version
        type: boolean
      bbox:
        description: The bounding box of the route, format <br> minLon, minLat, maxLon, maxLat
        type: array
        items:
          type: number
          format: double
      #snapped_waypoints:
      #  $ref: '#/definitions/GHResponseCoordinates'
      instructions:
        $ref: '#/definitions/GHResponseInstructions'
  Points:
    description: Unencoded points
    allOf:
    - $ref: "#/definitions/GHRouteResponsePath"
    - type: object
      properties:
        points:
          $ref: '#/definitions/GHResponseCoordinates'
      required:
      - points
  PointsEncoded:
    description: Encoded points
    allOf:
    - $ref: "#/definitions/GHRouteResponsePath"
    - type: object
      properties:
        points:
          type: string

@karussell
Copy link
Member

I opened another issue to see if we can get an overview of the supported languages to make a reasonable decision: swagger-api/swagger-codegen#4622.

Nice - thanks!

The problem is, we could not use the Routing API from certain languages.

Nice finding. Then let us for now go without polymorphism to bundle all APIs and support as many languages as possible. And only document the ugliness for using the Routing API...

@boldtrn
Copy link
Member Author

boldtrn commented Jan 23, 2017

Then let us for now go without polymorphism to bundle all APIs and support as many languages as possible

Ok, I would wait one or two days to see what languages are not supported with polymorphism (assuming that we get a fast answer to the issue :)). If it would be only Python and some almost not used languages I think it would be better to go with polymorphism. But if we cannot support 2-3 major languages we shouldn't use polymorphism.

@boldtrn
Copy link
Member Author

boldtrn commented Jan 24, 2017

Ok, it seems that there are a couple of languages that miss the polymorphism support. Thus, I will model the swagger to only support unencoded points.

@karussell
Copy link
Member

karussell commented Jan 27, 2017

How again was GeoJSON handled? Could we introduce a new points_type string entry in the response json to make it working?

@boldtrn
Copy link
Member Author

boldtrn commented Jan 27, 2017

How again was GeoJSON handled? Could we introduce a new points_type string entry in the response json to make it working?

GeoJson used the Polymorphism concept. The point_type would be the discriminator. See my code posted here. That might work, but only for some languages. Others don't support it.

@michaz
Copy link
Member

michaz commented Apr 23, 2018

enum: [false]

That seems exactly right, and we should put it in there.
Maybe the code generators will pick this up at some point, or maybe some of them already have.

See here, scroll down to Constant Parameters.

@boldtrn
Copy link
Member Author

boldtrn commented Apr 23, 2018

That seems exactly right, and we should put it in there

Yes I think so too, the issue was that it didn't work when I tried it. it would be nice if it would be working now :).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants