sukovanej / effect-http

Declarative HTTP API library for effect-ts
https://sukovanej.github.io/effect-http
MIT License
218 stars 16 forks source link

setRequestQuery, setRequestPath should only allow schemas where `From` is a Struct (or Record) with all values being strings #611

Open adrian-gierakowski opened 4 days ago

adrian-gierakowski commented 4 days ago

for example, this compiles:

import { NodeRuntime } from '@effect/platform-node'
import { Schema } from '@effect/schema'
import { pipe } from 'effect'
import { Api, ExampleServer, RouterBuilder } from 'effect-http'
import { NodeServer } from 'effect-http-node'

const Query = Schema.Struct({
  queryParam: Schema.Number,
})

const api = Api.make().pipe(
  Api.addEndpoint(
    Api.get('hello', '/hello').pipe(
      Api.setRequestQuery(Query),
    ),
  ),
)

pipe(
  ExampleServer.make(api),
  RouterBuilder.build,
  NodeServer.listen({ port: 3000 }),
  NodeRuntime.runMain,
)

but fails at runtime:

> curl 'http://localhost:3000/hello?queryParam=1'
{"error":"Request validation error","location":"query","message":"queryParam must be a number, received \"1\""}⏎   

As far as I can tell, there is no valid way to call this route.

Same applies to the following:

import { NodeRuntime } from '@effect/platform-node'
import { Schema } from '@effect/schema'
import { pipe } from 'effect'
import { Api, ExampleServer, RouterBuilder } from 'effect-http'
import { NodeServer } from 'effect-http-node'

const Path = Schema.Struct({
  param: Schema.Number,
})

const api = Api.make().pipe(
  Api.addEndpoint(
    Api.get('hello', '/hello/:param').pipe(
      Api.setRequestPath(Path),
    ),
  ),
)

pipe(
  ExampleServer.make(api),
  RouterBuilder.build,
  NodeServer.listen({ port: 3000 }),
  NodeRuntime.runMain,
)

which fails as follows:

> curl 'http://localhost:3000/hello/1'
{"error":"Request validation error","location":"path","message":"param must be a number, received \"1\""}

To fix it I can use Schema.NumberFromString but ideally we'd want the compiler to enforce schemas to be valid.

datner commented 4 days ago
export declare const schemaBodyUrlParams: <
  A,
  I extends Readonly<Record<string, string>>,
  R,
>(
  schema: Schema.Schema<A, I, R>,
  options?: ParseOptions | undefined,
) => Effect.Effect<A, any, HttpServerRequest | R>

^taken from @effect/platform, this shows exactly this thing. Though it should be Record<string, string | string[]> I think

sukovanej commented 4 days ago

Though it should be Record<string, string | string[]> I think

Yeah that's the thing. On the wire, query parameters are always (string, string) pairs, but it is actually possible to serialise strings, array of strings or even objects so the situation is not trivial. Tho, the way to approach this is to probably start with string | string[] as a type constraint of the value and wait for user feedback.

adrian-gierakowski commented 3 days ago

but it is actually possible to serialise strings, array of strings or even objects so the situation is not trivial

How would you deserialise this without user providing a schema from string | string[] to whatever they want to get on the other side? It’s the job of the user provided schema (unless you want to do some guess work) so I don’t see how any other would make sense as input.

sukovanej commented 3 days ago

but it is actually possible to serialise strings, array of strings or even objects so the situation is not trivial

How would you deserialise this without user providing a schema from string | string[] to whatever they want to get on the other side? It’s the job of the user provided schema (unless you want to do some guess work) so I don’t see how any other would make sense as input.

yeah, the user needs to use something like Schema.Union(Schema.String, Schema.Array(Schema.String)) (see also https://github.com/sukovanej/effect-http/issues/441)