Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Concurrent inserts with json result in some being written in string format #937

Closed
alexcouper opened this issue Sep 4, 2024 · 13 comments
Closed

Comments

@alexcouper
Copy link

alexcouper commented Sep 4, 2024

When run concurrently, this lib seems to insert jsonb data as a string rather than an object.

I've been able to reproduce down to a minimal example.

Steps to reproduce

  1. Create a table named "dodgy_json"
create table "dodgy_json" (
    "id" uuid not null default gen_random_uuid(),
    "created_at" timestamp with time zone not null default now(),
    "dodgy" jsonb not null
);
  1. Use this test and ensure it works:
import postgres from "postgres";

let sql: postgres.Sql;

describe("dodgy sql json 1", () => {
  beforeAll(async () => {
    sql = postgres(process.env.DATABASE_URL!, { ssl: true });
  });
  afterAll(async () => {
    sql.end();
  });

  for (let i = 0; i < 20; i++) {
    it("tests dodgy json", async () => {
      const jsonReady = sql.json({
        foo: "bar",
      });
      const obj =
        await sql`INSERT INTO dodgy_json(dodgy) VALUES (${jsonReady}) RETURNING id;`;
      const check =
        await sql`SELECT jsonb_typeof(dodgy) from dodgy_json WHERE id=${obj[0]!.id};`;
      expect(check[0]!.jsonb_typeof).toBe("object");
    });
  }
});
jest sql.test.ts
  1. Copy this test file into 3 different files and run them concurrently using jest
cp sql.test.ts sql2.test.ts
cp sql.test.ts sql3.test.ts
jest sql*
  1. See that you have errors
dodgy sql json 1 › tests dodgy json

    expect(received).toBe(expected) // Object.is equality

    Expected: "object"
    Received: "string"

      24 |         await sql`SELECT jsonb_typeof(dodgy) from dodgy_json WHERE id=${obj[0]!.id};`;
    > 25 |       expect(check[0]!.jsonb_typeof).toBe("object");
         |                                      ^
      26 |     });
      27 |   }
      28 | });

      ```
@porsager
Copy link
Owner

porsager commented Sep 4, 2024

don't use sql.json, it's deprecated and only for very specific cases

@alexcouper
Copy link
Author

alexcouper commented Sep 4, 2024

What's the correct way to insert into a jsonb field using template syntax?

Option 1: sql.json

What I was doing, due to issues like #838

Not deprecated: ❌
Works in single case: ✅
Works in concurrent case: ❌

Option 2: Directly

      const jsonReady = {
        foo: "bar",
      };
      const obj =
        await sql`INSERT INTO dodgy_json(dodgy) VALUES (${jsonReady}) RETURNING id;`;

fails with:

sql.test.ts:19:69 - error TS2769: No overload matches this call.
      Argument of type '[{ foo: string; }]' is not assignable to parameter of type 'never'.
      Overload 2 of 2, '(template: TemplateStringsArray, ...parameters: readonly ParameterOrFragment<never>[]): PendingQuery<Row[]>', gave the following error.
        Argument of type '{ foo: string; }' is not assignable to parameter of type 'ParameterOrFragment<never>'.

    19         await sql`INSERT INTO dodgy_json(dodgy) VALUES (${jsonReady}) RETURNING id;`;

Not deprecated: ✅
Works in single case: ❌
Works in concurrent case: ❌

Option 3: Directly, with any typing

#587 (comment) Made me try as any, which runs.

      const jsonReady = {
        foo: "bar",
      };
      const obj =
        await sql`INSERT INTO dodgy_json(dodgy) VALUES (${jsonReady as any}) RETURNING id;`;

But has the same issue that some are string values

Not deprecated: ✅
Works in single case: ✅
Works in concurrent case: ❌

@alexcouper
Copy link
Author

Option 4: Directly, with any typing and manual casting:

      const jsonReady = {
        foo: "bar",
      };
      const obj =
        await sql`INSERT INTO dodgy_json(dodgy) VALUES (${jsonReady as any}::jsonb) RETURNING id;`;
      const check =
        await sql`SELECT jsonb_typeof(dodgy) from dodgy_json WHERE id=${obj[0]!.id};`;
      expect(check[0]!.jsonb_typeof).toBe("object");

Not deprecated: ✅
Works in single case: ✅
Works in concurrent case: ❌

@alexcouper
Copy link
Author

alexcouper commented Sep 4, 2024

An aside fact, seems to fail approx 50% of the time in concurrent runs. Eg at the moment running with 10 tests in each of 3 concurrent test files, and consistently getting 14:16, 15:15 failed to passes when running concurrently

@alexcouper
Copy link
Author

Option 4 and 3 both work against a non-ssl local in docker database.

Failing case is SSL against Supabase backend.

@porsager
Copy link
Owner

porsager commented Sep 4, 2024

Right, just insert it directly. Do note that simple values won't work as expected - check #392

@porsager
Copy link
Owner

porsager commented Sep 4, 2024

Can you describe the concurrent test?

@porsager
Copy link
Owner

porsager commented Sep 4, 2024

Would be nice with a json specific docs section also

@alexcouper
Copy link
Author

Concurrent test is 3 files identical to this:

import postgres from "postgres";

let sql: postgres.Sql;

describe("dodgy sql json 1", () => {
  beforeAll(async () => {
    sql = postgres(process.env.DATABASE_URL!, { ssl: true });
  });
  afterAll(async () => {
    sql.end();
  });

  for (let i = 0; i < 20; i++) {
    it("tests dodgy json", async () => {
      const jsonReady = sql.json({
        foo: "bar",
      });
      const obj =
        await sql`INSERT INTO dodgy_json(dodgy) VALUES (${jsonReady}) RETURNING id;`;
      const check =
        await sql`SELECT jsonb_typeof(dodgy) from dodgy_json WHERE id=${obj[0]!.id};`;
      expect(check[0]!.jsonb_typeof).toBe("object");
    });
  }
});

Being all run with a single jest command.

Attempting to get an SSL postgres up to test against. If that succeeds then this may be a supabase issue

@porsager
Copy link
Owner

porsager commented Sep 4, 2024

Are you using their load balancer? Could there be a cache inbetween? When it fails is it because the row didn't exist or because the object was a string?

@alexcouper
Copy link
Author

SSL in docker worked fine, so I think this is a supabase thing.

Are you using their load balancer?

Yes

When it fails is it because the row didn't exist or because the object was a string?

Object is a string

I'm going to try skipping the loadbalancer

@porsager
Copy link
Owner

porsager commented Sep 4, 2024

Yeah, interesting what's going on! Not a nice behaviour 😬!

@alexcouper
Copy link
Author

I can't connect directly due to ipv6 issues.

However, connecting to 5432 which is for session based pooling, everything works as expected.

image

Default is 6543 which is transaction based.

I'll file an issue with the team there.

Thanks for the help @porsager

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants