Optional parameters in `query` are not really optional

[delanni]

Hi! I encountered this issue while describing my endpoints, and it can be demonstrated with the flowers api example.

Setting up the example from the wiki:

// ...
export const flowerAPI = defineAPI({
    listFlowers: GET `/flowers`
        .query({
            'sortBy': FlowerIndexedAttribute,
            'filterBy': FlowerIndexedAttribute
        })
        .response(rt.Array(Flower))
/ /...
});

// if you create a client
import axios from 'axios';
const driver = axios.create(config);
const flowers = createConsumer(flowerAPI, driver);

// and when you try to call it with the supposedly optional query params:
flowers.listFlowers({
  query: {
    sortBy: 'color'
    // filterBy: 'color' // without this parameter it will not pass typecheck
  }
});

As you can see, in the example, I'm calling the created API endpoint with an optional query param (I would like to think all query params are optional by default as stated here: https://github.com/hmil/rest.ts/blob/master/packages/rest-ts-core/src/builder-kit.ts#L136) it fails.

The error comes from the type calculations made on the inferred incoming type in defineAPI will see every Runtype defined type requirements as present, non-undefined, so none of them can be left out ultimately. (I hope this makes sense).

For now I don't know what would be a good solution, but I feel that if we actually want to allow ALL query params as optional (as they usually should be in RESTful design) then maybe the type calculations shouldn't be removing undefineable fields, but rather making the whole query type Partial.

My workaround, btw with the current code is this:

const identity: <T>(o: T) => T = o => o;
const partial: <T extends object>(o: T) => Partial<T> = identity;
const optional: <T>(o: T) => T | undefined = identity;

export const flowerAPI = defineAPI({
    listFlowers: GET `/flowers`
        .query(partial({
            'sortBy': FlowerIndexedAttribute,
            'filterBy': FlowerIndexedAttribute
        }))
        .response(rt.Array(Flower))
/ /...
});

What do you think? Is this a legit concern?

[hmil]

Good find. Yes this is legit.

Actually this is pretty bad because in the server all parameters are typed as string and not string | undefined, but the route will match regardless of whether the parameter was provided or not... So potential for crash right here.

The prototype of query should be changed to something like this:

public query<U extends QueryParams>(query: U): EndpointBuilder<RemoveKey<T, 'query'> & { query: Partial<ExtractRuntimeType<U>> }>

This is a breaking change.

[geekflyer]

Just a comment - I agree that in RESTful design it's usually good practice to have all query params as optional but there are some legit cases where you want query params to be mandatory, so I think the framework should allow for that too. So there should be either a flag to mark properties as mandatory, or a flag to mark them as optional or use some Partial runtimes wrapper.

[delanni]

It's also a legit solution, to update the docs, and javadocs, and let the authors make stuff optional with some util function.

[hmil]

Maybe there should be a runtime type check for request query parameters, similar to the one available for request body with runtypes.

The thing is: query params are user input (as seen by the server). We can make rest.ts enforce mandatory query params on the client, but the server might still receive bad data.