🦋 Changeset detected

Latest commit: bd25c9c

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you"re a maintainer who wants to add another changeset to this PR

Not sure if we really need single because it"s too simple, just Effect.flatten

There is also these schema APIs https://effect-ts.github.io/effect/sql/SqlSchema.ts.html#findall

There is also these schema APIs https://effect-ts.github.io/effect/sql/SqlSchema.ts.html#findall

Of course i"ve seen it:) And implemented this PR because SqlQuery doesn"t fit my needs. I don"t need to parse input, because it"s already parsed. I would say that it"s a very rare case when you pass input directly to SQL, because usually SQL is not the input edge of the application.

Also, i use "ports and adapters" or "hexagonal" (call as you wish) architecture, where my persistence layer is abstracted, so my application should not know about the underlying driver. It could be SQL DB, redis, or filesystem. SqlSchema module emits SqlError, my application is not allowed to know about it. So in order to map error type i anyway do

export const FetchBotLaunchConfigPostgres = Layer.effect(
  PersistenceFetchBotLaunchConfigImpl,
  Effect.gen(function* () {
    const sql = yield* SqlClient.SqlClient

    return {
      fetchLaunchConfig: (instanceId: string) => {
        return SqlSchema.single({
          Request: Schema.String,
          Result: PersistenceFetchBotLaunchConfigRes,
          execute: (instanceId) =>
            sql`SELECT *
                FROM fxngine.instance_launch_config
                WHERE instance_id = ${instanceId}::bigint`
        })(instanceId).pipe(
          Effect.catchTags({
            SqlError: (cause) =>
              new PersistenceError({ source: "fetchBotLaunchConfig", cause }),
            NoSuchElementException: () =>
              new PersistenceFetchBotLaunchConfigNotFoundError({ instanceId })
          })
        )
      }
    }
  })
)

Which makes no sense

So in order to map error type i anyway do [...] Which makes no sense

Why not? This makes perfect sense. "ports and adapters" thats the adapting part.

export const FetchBotLaunchConfigPostgres = Layer.effect(
  PersistenceFetchBotLaunchConfigImpl,
  Effect.gen(function* () {
    const sql = yield* SqlClient.SqlClient
    const byId = SqlSchema.single({
      Request: Schema.String,
      Result: PersistenceFetchBotLaunchConfigRes,
      execute: (instanceId) =>
      sql`SELECT *
      FROM fxngine.instance_launch_config
      WHERE instance_id = ${instanceId}::bigint`
    })

    return {
      fetchLaunchConfig: (instanceId: string) => byId(instanceId).pipe(
        Effect.catchTags({
          SqlError: (cause) =>
            new PersistenceError({ source: "fetchBotLaunchConfig", cause }),
          NoSuchElementException: () =>
            new PersistenceFetchBotLaunchConfigNotFoundError({ instanceId })
        })
      )
    }
  })
)

doesn"t fit my needs. I don"t need to parse input, because it"s already parsed. I would say that it"s a very rare case when you pass input directly to SQL, because usually SQL is not the input edge of the application.

this is just a misunderstanding of the role of the Request there. It"s not to parse, its to encode. For example your api wants to force that fetching Bar must be from a known Foo. it can do

SqlSchema.single({
  Request: Foo,
  ...,
  execute: (foo) => sql`...${foo.id}`
})

Now lets say that id is a complex expression of multiple values like the shard its in + id of service that created it + it"s own unique id, so its parsed into a nice { shard: string, origin: string, uid: string }. In this case it would encode it back to just the db-represented raw string. You having Schema.String there is not because we expect the user to pass a number, it"s because thats the arbitrary input you decided for this exposed function, it"s still working as expected.

There is no question about if "it"s a very rare case when you pass input directly to SQL", as this is an implicit assumption that input that reached this level would be valid. Remember, schema also does validation, but it"s much more than that.

You should bring your ideas up to debate more often, I promise effect is very well thought out and I feel really bad when you put in this much effort..

Going to close this, I don"t think it adds enough value to be an addition to /sql.

I understand that the schema is also about encoding, not just validation. However, in my case, encoding is unnecessary for SQL queries. I deal with dozens of SQL queries, and i am forced to use S.Any as a placeholder. The example of encoding an ID is an edge case that applies only in specific, rare situations. I respectfully ask for reconsideration of the PR, as it provides a solution that is suitable for the majority of users who don’t need this kind of encoding for SQL.

Let"s view at this PR from another perspective – how would you suggest to make an SQL that does not requires input encoding?