search

palantir / conjure

Strongly typed HTTP/JSON APIs for browsers and microservices
https://palantir.github.io/conjure/
Apache License 2.0
417 stars 65 forks source link

Spec for query parameters does not match behavior of IR compiler #92

Closed nmiyake closed 5 years ago

nmiyake commented 6 years ago

BLUF: Conjure spec states that header and query parameters can only be primitives or optional of primitives. However, the IR generator allows list<string> query parameters (but not header parameters). The spec or generator should be updated so they are in agreement on behavior.

In the https://github.com/palantir/conjure/blob/develop/docs/spec/source_files.md#argumentdefinition section of the spec, the documentation for the type field states:

The type MUST be a primitive if the argument is a path parameter and primitive or optional of primitive if the argument is header or query parameter.

This seems to be enforced for header parameters, as specifying list<string> as a type fails:

services:
  TestService:
    name: Test Service
    package: com.palantir.another
    endpoints:
      headerParam:
        http: GET /headerParam
        args:
          foo:
            param-type: header
            param-id: FOO
            type: list<string>

Fails with:

Exception in thread "main" java.lang.IllegalStateException: Header parameters must be primitives, aliases or optional primitive: "foo" is not allowed

However, it is possible to generate query parameters that are collection types.

The input:

services:
  TestService:
    name: Test Service
    package: com.palantir.another
    endpoints:
      headerParam:
        http: GET /headerParam
        args:
          foo:
            param-type: query
            type: list<string>

Produces the IR:

{
  "version" : 1,
  "errors" : [ ],
  "types" : [ ],
  "services" : [ {
    "serviceName" : {
      "name" : "TestService",
      "package" : "com.palantir.another"
    },
    "endpoints" : [ {
      "endpointName" : "headerParam",
      "httpMethod" : "GET",
      "httpPath" : "/headerParam",
      "args" : [ {
        "argName" : "foo",
        "type" : {
          "type" : "list",
          "list" : {
            "itemType" : {
              "type" : "primitive",
              "primitive" : "STRING"
            }
          }
        },
        "paramType" : {
          "type" : "query",
          "query" : {
            "paramId" : "foo"
          }
        },
        "markers" : [ ]
      } ],
      "markers" : [ ]
    } ]
  } ]
}

This does not match what is outlined in the spec -- the spec should be updated or the implementation should be changed to match it.

nmiyake commented 5 years ago

@iamdanfox @ferozco do you have a sense of what is actually correct here (the spec or the implementation)?

@uschi2000 for SA

iamdanfox commented 5 years ago

Good spot @nmiyake! I'd like to resolve this inconsistency by changing the current codebase to match the spec.

I think it's preferable to be intentionally restrictive with all of (query, header, path-params). The reasons being:

uschi2000 commented 5 years ago

Do we need to verify that nobody uses collection types in header or query params before making this change?

// from mobile

From: iamdanfox notifications@github.com Sent: Monday, September 24, 2018 5:58:56 AM To: palantir/conjure Cc: Robert Fink; Mention Subject: Re: [palantir/conjure] Spec for query parameters does not match behavior of IR compiler (#92)

Good spot @nmiyake [github.com]https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_nmiyake&d=DwMFaQ&c=izlc9mHr637UR4lpLEZLFFS3Vn2UXBrZ4tFb6oOnmz8&r=UfcWCaR4ui50AFap-gezrx5XYtPtH-9JpazU7tbRW-4&m=hCU-IjHLwn-m5xr9XOl0tvoI4jmYmFERHmejU6_2ZTM&s=UPxQrdafp6bxkRBt3c2arkEo6asOJy2HmAaPdSHZ654&e=! I'd like to resolve this inconsistency by changing the current codebase to match the spec.

I think it's preferable to be intentionally restrictive with all of (query, header, path-params). The reasons being:

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub [github.com]https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_palantir_conjure_issues_92-23issuecomment-2D423965749&d=DwMFaQ&c=izlc9mHr637UR4lpLEZLFFS3Vn2UXBrZ4tFb6oOnmz8&r=UfcWCaR4ui50AFap-gezrx5XYtPtH-9JpazU7tbRW-4&m=hCU-IjHLwn-m5xr9XOl0tvoI4jmYmFERHmejU6_2ZTM&s=Mjl-PmXoQeC0IsTu-OXwOM64IpMqBvoP3LG2h7_SYfg&e=, or mute the thread [github.com]https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AGOdwXpDZewhEF5P4P24Q3jhZBcOsJ-5F-2Dks5ueNcQgaJpZM4Wy6fL&d=DwMFaQ&c=izlc9mHr637UR4lpLEZLFFS3Vn2UXBrZ4tFb6oOnmz8&r=UfcWCaR4ui50AFap-gezrx5XYtPtH-9JpazU7tbRW-4&m=hCU-IjHLwn-m5xr9XOl0tvoI4jmYmFERHmejU6_2ZTM&s=noXS9BNwQlZ3d1hdwxRQtE0hbHmOnDgjC9RORQdctQQ&e=.

iamdanfox commented 5 years ago

That's only really feasible right now because Conjure is only use internally. If there were Conjure definitions out in the wild, would the response to that question change how we approach this? (ie we could add a warning log line to tell people this is deprecated, then do a soft-break by putting it behind a feature flag, then eventually a full break with a major rev?)

uschi2000 commented 5 years ago

If nobody uses multiplicity then it's a no-brainer. Maybe it's even impossible to use given the current implementations. And yes, we'd have to be more careful if there are known or unknown uses out there, of course.

ferozco commented 5 years ago

It seems like there is usage internally of the collection types in query parameters. I think we can add a deprecation warning, and track this as something we would like to change whenever a major rev comes

uschi2000 commented 5 years ago

We can also update the spec to match the IR: allow lists for query params, but not for headers. @nmiyake 's request to make spec+impl match, which I think is fair :)