Optional query param should not be sent

ouertani commented 1 month ago

Prerequisites

[ X] I have checked the existing issues to ensure this is not a duplicate.
[ X] I am running the latest version of Tailcall.

Describe the bug

A clear and concise description of what the bug is.

Seems to be a regression as of now optional query param are part of the url

Steps to reproduce

use this schema
```
scalar DateTime
```

schema @server( port: 8000 hostname: "0.0.0.0" ) @upstream( baseURL: "http://localhost:8083/graphql" allowedHeaders: ["Authorization"] )

{ query: Query }

type NodeA { name : String } input AFilter { p1 : String p3 : String p4 : String }

type Query { queryNodeA(filter: AFilter, p2: Int):[NodeA] @http(method: GET , path: "/find", query: [ {key: "p1", value: "{{.args.filter.p1}}"} {key: "p2", value: "{{.args.p2}}"} {key: "p3", value: "{{.args.filter.p3}}"} {key: "p4", value: "{{args.filter.p4}}"} ]) }````

send this query

query {
queryNodeA ( filter : { p1 : "a"}) {
name
}
}

check the response

Expected behavior

/graphql/find?p1=a

Actual behavior

/graphql/find?p1=a&p2&p3&p4

Screenshots

If applicable, add screenshots to help explain your problem.

Environment information:

Operating System: (e.g., Ubuntu 20.04, macOS Big Sur, Windows 10)
Tailcall Version: (e.g., v1.0.2)
Rust Version: (e.g., rustc 1.58.0)
GraphQL Client/version (e.g., curl/8.1.2)

Additional context

Add any other context about the problem here, such as relevant logs, error messages, or stack traces.

Logs

Paste any relevant logs here.

karatakis commented 1 month ago

Proposed solutions that require development and design.

Solution 1

Introduce skip_null: {true/false}, and if the value is empty/null (this is ambiguous), we skip sending it.

type Query {
    queryNodeA(filter: AFilter, p2: Int):[NodeA]  @http(method: GET , path: "/find",
    query: [
          {key: "p1", value: "{{.args.filter.p1}}", skip_null: true}
        ])
}

Solution 2

Introduce skip_default: {true/false}; if the value is Default, we skip sending it.

Discussion

In the future, people might have other scenarios to skip, like edge cases or special values. One way to tackle this problem is by introducing skip_js, where we execute javascript code and let users decide. Another way is to provide an expressive DSL that allows users to write custom skip rules @skip(negative, zero, empty, null) / @skip(len_gt(5) / etc..

ouertani commented 1 month ago

It still the same behavior:

scalar DateTime

schema
  @server(
    port: 8000
    hostname: "0.0.0.0"
  )
  @upstream( 
    baseURL: "http://localhost:8083/graphql"
    allowedHeaders: ["Authorization"]
  )

  {
  query: Query
}

type NodeA {
    name : String
}
input AFilter {
    p1 :  String
    p3 :  String
    p4 :  String
}

type Query {
    queryNodeA(filter: AFilter, p2: Int):[NodeA]  @http(method: GET , path: "/find",
    query: [
          {key: "p1", value: "{{.args.filter.p1}}" , skip_null: true}
          {key: "p2", value: "{{.args.p2}}" , skip_null: true}
          {key: "p3", value: "{{.args.filter.p3}}", skip_null: true}
          {key: "p4", value: "{{args.filter.p4}}", skip_null: true}
        ])
}

tusharmath commented 1 month ago

@ouertani skipNull is WIP — https://github.com/tailcallhq/tailcall/pull/2475

ouertani commented 1 month ago

@ouertani skipNull is WIP — #2475

The point is that this feature was the default the skip optional and seems a breaking change/ regression .

I suggest to skip null is the default.

github-actions[bot] commented 2 weeks ago

Action required: Issue inactive for 30 days. Status update or closure in 7 days.

github-actions[bot] commented 1 week ago

Issue closed after 7 days of inactivity.

tailcallhq / tailcall