Effect-TS / effect

An ecosystem of tools for building production-grade applications in TypeScript.
https://effect.website
MIT License
7.1k stars 227 forks source link

sql: Silently swallows errors #2730

Open TheDevMinerTV opened 4 months ago

TheDevMinerTV commented 4 months ago

What version of Effect is running?

3.1.3

What steps can reproduce the bug?

  1. Set up a PostgreSQL container
  2. Run the following code (make sure that the connection credentials are incorrect)
    
    import * as Sql from "@effect/sql";
    import * as Pg from "@effect/sql-pg";
    import { Config, Effect } from "effect";

const SqlLive = Pg.client.layer({ database: Config.succeed("inventory") });

const program = Effect.gen(function () { yield Effect.log("Connecting to SQL..."); const sql = yield Sql.client.Client; yield Effect.log("Connected to SQL!");

const people = yield* sql<{ readonly id: number; readonly name: string; }>SELECT id, name FROM people; // Hangs here ^

yield* Effect.log(Got ${people.length} results!); }).pipe(Effect.scoped);

program.pipe(Effect.provide(SqlLive), Effect.runPromise);


### What is the expected behavior?

The error gets logged, or gets returned up.

### What do you see instead?

Nothing from the Effect code, but my PostgreSQL server is spitting out authentication errors.

### Additional information

```json
    "@effect/sql": "^0.2.5",
    "@effect/sql-pg": "^0.2.5",
TheDevMinerTV commented 4 months ago

Apparently it's also happening with the migrator which also silently hangs the application when the migrations folder doesn't exist.

tim-smart commented 4 months ago

I tried your example and got this error:

image
TheDevMinerTV commented 4 months ago

Is there a environment variable to change the log level?

mikearnaldi commented 3 months ago

@TheDevMinerTV is this related to the discord discussion? if yes then this is not an issue, if not we need a full repro

thanhnguyen2187 commented 1 week ago

Hi. I'm having the same issue for Migrator. Here is a minimal reproduction:

import { SqlClient } from "@effect/sql";
import { SqliteClient, SqliteMigrator } from "@effect/sql-sqlite-node";
import { Config, Effect, pipe, Layer } from "effect";
import { fileURLToPath } from "node:url";
import { NodeRuntime, NodeContext } from "@effect/platform-node";

const SqlLive = SqliteClient.layer({
  filename: Config.succeed(":memory:"),
});

const MigratorLive = pipe(
  SqliteMigrator.layer({
    loader: SqliteMigrator.fromFileSystem(
      fileURLToPath(new URL("src/lib/data/migrations", import.meta.url)),
    ),
    schemaDirectory: "src/lib/data/migrations",
  }),
  Layer.provide(SqlLive),
  Layer.provide(NodeContext.layer),
);

const program = Effect.gen(function* () {
  const sql = yield* SqlClient.SqlClient;

  const rows = yield* sql<{
    count: number;
  }>`SELECT COUNT(*) AS count FROM site_configs`;
  const count = rows[0].count;

  yield* Effect.log(`Got ${count} results!`);
});

pipe(
  program,
  Effect.provide(SqlLive),
  Effect.provide(MigratorLive),
  NodeRuntime.runMain,
);

Where within migrations, there is a file named 0000_create_table_site_configs.ts:

import { Effect } from "effect";
import { SqlClient } from "@effect/sql";

export default Effect.map(
  SqlClient.SqlClient,
  (sql) => sql`
    ERROR SQL;
    CREATE TABLE site_configs (
      id SERIAL PRIMARY KEY,
      url TEXT NOT NULL,
      request JSON NOT NULL,
      response JSON NOT NULL
    )
  `,
);
thanhnguyen2187 commented 1 week ago

Side note: a bit of digging led me to FileSystem.ts and loadFromFileSystem:

export const fromFileSystem = (directory: string): Loader<FileSystem> =>
  FileSystem.pipe(
    Effect.flatMap((FS) => FS.readDirectory(directory)),
    Effect.mapError((error) => new MigrationError({ reason: "failed", message: error.message })),
    Effect.map((files): ReadonlyArray<ResolvedMigration> =>
      files
        .map((file) => Option.fromNullable(file.match(/^(?:.*\/)?(\d+)_([^.]+)\.(js|ts)$/)))
        .flatMap(
          Option.match({
            onNone: () => [],
            onSome: ([basename, id, name]): ReadonlyArray<ResolvedMigration> =>
              [
                [
                  Number(id),
                  name,
                  Effect.promise(
                    () =>
                      import(
                        /* @vite-ignore */
                        `${directory}/${basename}`
                      )
                  )
                ]
              ] as const
          })
        )
        .sort(([a], [b]) => a - b)
    )
  )

I think we should have some docs for naming convention of migration files :sweat_smile:

thanhnguyen2187 commented 1 week ago

EDIT: if the file is named 0000_something.ts, then it'll get ignored.


I tried putting invalid TypeScript syntax within a migration file, and it does not get picked up either. Seems like Effect.promise from the file above is the problem? The docs said that:

promise

This constructor is similar to a regular Promise, where you're confident that the asynchronous operation will always succeed. It allows you to create an Effect that represents successful completion without considering potential errors. However, it's essential to ensure that the underlying Promise never rejects.

thanhnguyen2187 commented 1 week ago

I dug through the code and found that the default migration table is effect_sql_migrations. I got

SqliteError: no such table: effect_sql_migrations

From querying the table, which means MigratorLive doesn't run, even after it was provided? It seems weird, as when I provide a non-existent migration folder path, it did raise an error

MigrationError: ENOENT: no such file or directory, scandir '...'
thanhnguyen2187 commented 1 week ago

I found the problem: I was using Effect.map in my .ts migration file, while Effect.flatMap is needed :sweat:. My case is done here then


EDIT: another gotcha is the usage of :memory:. Somehow switching to a real file works


UPDATE: apparently, :memory: would point to a new in-memory database for each new connection, which creates the above issue. There is no fix yet because of better-sqlite3's implementation only accepts :memory:.

References:

bountyhub-bot commented 1 week ago

πŸš€ Bounty Alert!

πŸ’° A bounty of $12.00 has been created by omarsoufiane on BountyHub.

πŸ”— Claim this bounty by submitting a pull request that solves the issue!

Good luck, and happy coding! πŸ’»